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.widaw...@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.ekstr...@intel.com> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358 >>> Signed-off-by: Ben Widawsky <benjamin.widaw...@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.ekstr...@intel.com> >> Cc "11.0 11.1" <mesa-sta...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
