i965: fixed clamping in set_scissor_bits when the y is flipped

Submitted by Eleni Maria Stea on Dec. 10, 2018, 10:42 a.m.

Details

Message ID 20181210104240.19400-1-estea@igalia.com
State New
Headers show
Series "i965: fixed clamping in set_scissor_bits when the y is flipped" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Eleni Maria Stea Dec. 10, 2018, 10:42 a.m.
Calculating the scissor rectangle fields with the y flipped (0 on top)
can generate negative values that will cause assertion failure later on
as the scissor fields are all unsigned. We must clamp the bbox values
again to make sure they don't exceed the fb_height. Also fixed a
calculation error.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108999
---
 src/mesa/drivers/dri/i965/genX_state_upload.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
index 8e3fcbf12e..5d8fc8214e 100644
--- a/src/mesa/drivers/dri/i965/genX_state_upload.c
+++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
@@ -2424,8 +2424,21 @@  set_scissor_bits(const struct gl_context *ctx, int i,
       /* memory: Y=0=top */
       sc->ScissorRectangleXMin = bbox[0];
       sc->ScissorRectangleXMax = bbox[1] - 1;
+
+      /* Clamping to fb_height is necessary because otherwise the
+       * subtractions below would produce a negative result, which would
+       * then be assigned to the unsigned YMin/YMax scissor fields,
+       * resulting in an assertion failure in GENX(SCISSOR_RECT_pack)
+       */
+
+      if (bbox[3] > fb_height)
+         bbox[3] = fb_height;
+
+      if (bbox[2] > fb_height)
+         bbox[2] = fb_height;
+
       sc->ScissorRectangleYMin = fb_height - bbox[3];
-      sc->ScissorRectangleYMax = fb_height - bbox[2] - 1;
+      sc->ScissorRectangleYMax = fb_height - (bbox[2] - 1);
    }
 }
 

Comments

On Mon, Dec 10, 2018 at 12:42:40PM +0200, Eleni Maria Stea wrote:
> Calculating the scissor rectangle fields with the y flipped (0 on top)
> can generate negative values that will cause assertion failure later on
> as the scissor fields are all unsigned. We must clamp the bbox values
> again to make sure they don't exceed the fb_height. Also fixed a
> calculation error.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108999

Good find. Could you send the test to the piglit list?

> ---
>  src/mesa/drivers/dri/i965/genX_state_upload.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index 8e3fcbf12e..5d8fc8214e 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -2424,8 +2424,21 @@ set_scissor_bits(const struct gl_context *ctx, int i,
>        /* memory: Y=0=top */
>        sc->ScissorRectangleXMin = bbox[0];
>        sc->ScissorRectangleXMax = bbox[1] - 1;
> +
> +      /* Clamping to fb_height is necessary because otherwise the
> +       * subtractions below would produce a negative result, which would
> +       * then be assigned to the unsigned YMin/YMax scissor fields,
> +       * resulting in an assertion failure in GENX(SCISSOR_RECT_pack)
> +       */
> +
> +      if (bbox[3] > fb_height)
> +         bbox[3] = fb_height;
> +
> +      if (bbox[2] > fb_height)
> +         bbox[2] = fb_height;
> +

We should be able to fix this bug in a simpler manner by changing the
MAX2 calls at the top of this function to CLAMP calls.

>        sc->ScissorRectangleYMin = fb_height - bbox[3];
> -      sc->ScissorRectangleYMax = fb_height - bbox[2] - 1;
> +      sc->ScissorRectangleYMax = fb_height - (bbox[2] - 1);

I don't think we want to start adding 1 instead of subtracting 1. The
subtraction is there to satisfy the requirement for the HW packet.

-Nanley

>     }
>  }
>  
> -- 
> 2.20.0.rc2
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, 19 Feb 2019 16:27:56 -0800
Nanley Chery <nanleychery@gmail.com> wrote:

> On Mon, Dec 10, 2018 at 12:42:40PM +0200, Eleni Maria Stea wrote:
> > Calculating the scissor rectangle fields with the y flipped (0 on
> > top) can generate negative values that will cause assertion failure
> > later on as the scissor fields are all unsigned. We must clamp the
> > bbox values again to make sure they don't exceed the fb_height.
> > Also fixed a calculation error.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108999  
> 
> Good find. Could you send the test to the piglit list?
Sure, I will send it.


> 
> > ---
> >  src/mesa/drivers/dri/i965/genX_state_upload.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c
> > b/src/mesa/drivers/dri/i965/genX_state_upload.c index
> > 8e3fcbf12e..5d8fc8214e 100644 ---
> > a/src/mesa/drivers/dri/i965/genX_state_upload.c +++
> > b/src/mesa/drivers/dri/i965/genX_state_upload.c @@ -2424,8 +2424,21
> > @@ set_scissor_bits(const struct gl_context *ctx, int i, /* memory:
> > Y=0=top */ sc->ScissorRectangleXMin = bbox[0];
> >        sc->ScissorRectangleXMax = bbox[1] - 1;
> > +
> > +      /* Clamping to fb_height is necessary because otherwise the
> > +       * subtractions below would produce a negative result, which
> > would
> > +       * then be assigned to the unsigned YMin/YMax scissor fields,
> > +       * resulting in an assertion failure in
> > GENX(SCISSOR_RECT_pack)
> > +       */
> > +
> > +      if (bbox[3] > fb_height)
> > +         bbox[3] = fb_height;
> > +
> > +      if (bbox[2] > fb_height)
> > +         bbox[2] = fb_height;
> > +  
> 
> We should be able to fix this bug in a simpler manner by changing the
> MAX2 calls at the top of this function to CLAMP calls.
> 
> >        sc->ScissorRectangleYMin = fb_height - bbox[3];
> > -      sc->ScissorRectangleYMax = fb_height - bbox[2] - 1;
> > +      sc->ScissorRectangleYMax = fb_height - (bbox[2] - 1);  
> 
> I don't think we want to start adding 1 instead of subtracting 1. The
> subtraction is there to satisfy the requirement for the HW packet.
> 
> -Nanley

Right! This code would be correct if I had done:

      if (bbox[2] >= fb_height)
         bbox[2] = fb_height - 1;

and then had left:
      sc->ScissorRectangleYMax = fb_height - bbox[2] - 1;

as it was. :)

I think I like your solution better because with the CLAMP at the top
what we do here is more clear. I am going to send a new patch soon.

Thank you!
Eleni