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

[Ben Widawsky]

On Tue, Feb 09, 2016 at 12:10:18PM -0800, Ian Romanick wrote:
> 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? :)
> 

Anuj is working on it.

> > 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
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev