Re: [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 
>> 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 
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
>>> Signed-off-by: Ben Widawsky 
>>> ---
>>>  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, , );
>>> +   /* 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 
>> Cc "11.0 11.1" 
> 
> 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

Re: [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 
> >> 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 
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
> >>> Signed-off-by: Ben Widawsky 
> >>> ---
> >>>  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, , );
> >>> +   /* 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 
> >> Cc "11.0 11.1" 
> > 
> > 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@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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
> 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 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
> > Signed-off-by: Ben Widawsky 
> > ---
> >  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, , );
> > +   /* 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 
> Cc "11.0 11.1" 

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@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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Sat, Feb 6, 2016 at 6:11 PM, Ben Widawsky 
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 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
> Signed-off-by: Ben Widawsky 
> ---
>  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, , );
> +   /* 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 
Cc "11.0 11.1" 

--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

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

[Matt Turner]

On Sat, Feb 6, 2016 at 6:11 PM, Ben Widawsky
 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 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358

Tested-by: Matt Turner 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Matt Turner]

On Sat, Feb 6, 2016 at 6:11 PM, Ben Widawsky
 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 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
> Signed-off-by: Ben Widawsky 
> ---
>  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, , );

No need to resend, but there should be a blank line before a multi-line comment.

> +   /* 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.
> +*/
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ben Widawsky]

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 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93358
Signed-off-by: Ben Widawsky 
---
 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, , );
+   /* 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);
+
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