On Tue, 19 Feb 2019 16:27:56 -0800
Nanley Chery 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
