[Mesa-stable] [PATCH 7/8] i965: Fix {src, dst}_pitch alignment check for XY_SRC_COPY_BLT

[Anuj Phogat]

On Mon, Aug 17, 2015 at 12:24 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Fri, Aug 14, 2015 at 04:51:58PM -0700, Anuj Phogat wrote:
>> Current code checks the alignment restrictions only for Y tiling.
>> From Broadwell PRM vol 10:
>>
>>  "pitch is of 512Byte granularity for Tile-X: This means the tiled-x
>>   surface pitch can be (512, 1024, 1536, 2048...)/4 (in Dwords)."
>>
>> This patch adds the restriction for X tiling as well.
>>
>> Cc: Ben Widawsky <ben at bwidawsk.net>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>
> I don't think you need to Cc stable because the other tiling modes aren't used,
> right? (also, the backport you've left is kinda sucky, you should probably
> manually send the backport to stable if you feel it's applicabale).
>
I can drop the Cc stable because we had enough checks earlier in the
code to make sure we get the tile aligned pitch.

>> ---
>>  src/mesa/drivers/dri/i965/intel_blit.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_blit.c b/src/mesa/drivers/dri/i965/intel_blit.c
>> index d15a64d..729ffb0 100644
>> --- a/src/mesa/drivers/dri/i965/intel_blit.c
>> +++ b/src/mesa/drivers/dri/i965/intel_blit.c
>> @@ -531,6 +531,8 @@ intelEmitCopyBlit(struct brw_context *brw,
>>     bool dst_y_tiled = dst_tiling == I915_TILING_Y;
>>     bool src_y_tiled = src_tiling == I915_TILING_Y;
>>     bool use_fast_copy_blit = false;
>> +   uint32_t src_tile_w, src_tile_h;
>> +   uint32_t dst_tile_w, dst_tile_h;
>>
>>     if ((dst_y_tiled || src_y_tiled) && brw->gen < 6)
>>        return false;
>> @@ -559,6 +561,11 @@ intelEmitCopyBlit(struct brw_context *brw,
>>         src_buffer, src_pitch, src_offset, src_x, src_y,
>>         dst_buffer, dst_pitch, dst_offset, dst_x, dst_y, w, h);
>>
>> +   intel_miptree_get_tile_dimensions(src_tiling, src_tr_mode, cpp,
>> +                                     &src_tile_w, &src_tile_h);
>> +   intel_miptree_get_tile_dimensions(dst_tiling, dst_tr_mode, cpp,
>> +                                     &dst_tile_w, &dst_tile_h);
>> +
>>     use_fast_copy_blit = can_fast_copy_blit(brw,
>>                                             src_buffer,
>>                                             src_x, src_y,
>> @@ -597,8 +604,8 @@ intelEmitCopyBlit(struct brw_context *brw,
>>                          cpp, use_fast_copy_blit);
>>
>>     } else {
>> -      assert(!dst_y_tiled || (dst_pitch % 128) == 0);
>> -      assert(!src_y_tiled || (src_pitch % 128) == 0);
>> +      assert(src_tiling == I915_TILING_NONE || (src_pitch % src_tile_w) == 0);
>> +      assert(dst_tiling == I915_TILING_NONE || (dst_pitch % dst_tile_w) == 0);
>>
>>        /* For big formats (such as floating point), do the copy using 16 or
>>         * 32bpp and multiply the coordinates.
>
>
> I like the generalization again (like the last patch). Do you actually need the
> check for TILING_NONE after all the plumbing you've done? I believe it's just:
> assert(src_pitch % src_tile_w == 0);
> assert(dst_pitch % dst_tile_w == 0);
>
drm_intel_gem_bo_tile_pitch() aligns the pitch to 64 in case of no tiling.
That makes this assert to fail. So, we need to exclude the no tiling case here.

> (since I don't think pitch can be anything but a multiple of cpp).
>
> Kill the cc stable unless you prove it's needed and it's:
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>