[Mesa-dev] [PATCH] [v2] i965: Make sure we blit a full compressed block

[Ian Romanick]

On 02/09/2016 11:58 AM, Ben Widawsky wrote:
> On Mon, Feb 08, 2016 at 09:48:54PM -0800, Jason Ekstrand wrote:
>> On Sat, Feb 6, 2016 at 6:11 PM, Ben Widawsky <benjamin.widawsky at intel.com>
>> wrote:
>>
>>> This fixes an assertion failure in [at least] one of the Unreal Engine
>>> Linux
>>> demo/games that uses DXT1 compression. Specifically, the "Vehicle Game".
>>>
>>> At some point, the game ends up trying to blit mip level whose size is 2x2,
>>> which is smaller than a DXT1 block. As a result, the assertion in the blit
>>> path
>>> is triggered. It should be safe to simply make sure we align the width and
>>> height, which is sadly an example of compression being less efficient.
>>>
>>> NOTE: The demo seems to work fine without the assert, and therefore release
>>> builds of mesa wouldn't stumble over this. Perhaps there is some
>>> unnoticeable
>>> corruption, but I had trouble spotting it.
>>>
>>> Thanks to Jason for looking at my backtrace and figuring out what was
>>> going on.
>>>
>>> v2: Use NPOT alignment to make sure ASTC is handled properly (Ilia)
>>> Remove comment about how this doesn't fix other bugs, because it does.
>>>
>>> Cc: Jason Ekstrand <jason.ekstrand at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
>>> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
>>> ---
>>>  src/mesa/drivers/dri/i965/intel_copy_image.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
>>> b/src/mesa/drivers/dri/i965/intel_copy_image.c
>>> index 0a3337e..42bd7ff 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_copy_image.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
>>> @@ -212,6 +212,7 @@ intel_copy_image_sub_data(struct gl_context *ctx,
>>>     struct brw_context *brw = brw_context(ctx);
>>>     struct intel_mipmap_tree *src_mt, *dst_mt;
>>>     unsigned src_level, dst_level;
>>> +   GLuint bw, bh;
>>>
>>>     if (_mesa_meta_CopyImageSubData_uncompressed(ctx,
>>>                                                  src_image,
>>> src_renderbuffer,
>>> @@ -275,6 +276,18 @@ intel_copy_image_sub_data(struct gl_context *ctx,
>>>     intel_miptree_all_slices_resolve_depth(brw, dst_mt);
>>>     intel_miptree_resolve_color(brw, dst_mt);
>>>
>>> +   _mesa_get_format_block_size(src_mt->format, &bw, &bh);
>>> +   /* It's legal to have a WxH that's smaller than a compressed block.
>>> This
>>> +    * happens for example when you are using a higher level LOD. For this
>>> case,
>>> +    * we still want to copy the entire block, or else the decompression
>>> will be
>>> +    * incorrect.
>>> +    */
>>> +   if (src_width < bw)
>>> +      src_width = ALIGN_NPOT(src_width, bw);
>>> +
>>> +   if (src_height < bh)
>>> +      src_height = ALIGN_NPOT(src_height, bh);
>>>
>>
>> I've been going back-and-forth as to whether or not this is the right place
>> to do this or if we wanted it further up or down the stack.  At the end of
>> the day, I concluded that it's as good a place as any.
>>
>> Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
>> Cc "11.0 11.1" <mesa-stable at lists.freedesktop.org>
> 
> I left out stable intentionally because we will only hit the assert in debug
> builds, and AFAICT, we'll only get minor corruption by this not being there -
> but it's up to you. I just figured I'd share my thoughts in case they weren't
> clear. I can add stable if you still think it wise.

I think it's a good candidate for stable.  It's pretty low risk, and it
fixes something real.

Is there a piglit test? :)

> Thanks for review.
> 
>>
>> --Jason
>>
>>
>>> +
>>>     if (copy_image_with_blitter(brw, src_mt, src_level,
>>>                                 src_x, src_y, src_z,
>>>                                 dst_mt, dst_level,
>>> --
>>> 2.7.0
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev