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

[Ben Widawsky]

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.

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