[Mesa-dev] [PATCH] i965: fixed clamping in set_scissor_bits when the y is flipped

[Eleni Maria Stea]

On Tue, 19 Feb 2019 16:27:56 -0800
Nanley Chery <nanleychery at 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