Re: [Mesa-dev] [PATCH] i965: fixed clamping in set_scissor_bits when the y is flipped
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
Re: [Mesa-dev] [PATCH] i965: fixed clamping in set_scissor_bits when the y is flipped
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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965: fixed clamping in set_scissor_bits when the y is flipped
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(-) 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); } } -- 2.20.0.rc2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
