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