Re: [Mesa-dev] [PATCH v3] i965: fixed clamping in set_scissor_bits when the y is flipped
On Thu, Feb 21, 2019 at 12:02:58PM +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 > I guess we'll want to add the other bug and Cc stable. > v2: >- I initially clamped the values inside the if (Y is flipped) case >and I made a mistake in the calculation: the clamp of the bbox[2] should >be a check if (bbox[2] >= fbheight) bbox[2] = fbheight - 1 instead and I >shouldn't have changed the ScissorRectangleYMax calculation. As the >fixed code is equivalent with using CLAMP instead of MAX2 at the top of >the function when bbox[2] and bbox[3] are calculated, and the 2nd is more >clear, I replaced it. (Nanley Chery) > > v3: >- Reversed the CLAMP change in bbox[3] as the API guarantees that the >viewport height is positive. (Nanley Chery) > --- > src/mesa/drivers/dri/i965/genX_state_upload.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > This patch is Reviewed-by: Nanley Chery > diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c > b/src/mesa/drivers/dri/i965/genX_state_upload.c > index dcdfb3c9292..47f3741e673 100644 > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c > @@ -2445,7 +2445,7 @@ set_scissor_bits(const struct gl_context *ctx, int i, > > bbox[0] = MAX2(ctx->ViewportArray[i].X, 0); > bbox[1] = MIN2(bbox[0] + ctx->ViewportArray[i].Width, fb_width); > - bbox[2] = MAX2(ctx->ViewportArray[i].Y, 0); > + bbox[2] = CLAMP(ctx->ViewportArray[i].Y, 0, fb_height); > bbox[3] = MIN2(bbox[2] + ctx->ViewportArray[i].Height, fb_height); > _mesa_intersect_scissor_bounding_box(ctx, i, bbox); > > -- > 2.20.1 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] i965: fixed clamping in set_scissor_bits when the y is flipped
I've run the test with applied patch and it's successfully passed. Also, this patch fixed an one issue from another ticket - https://bugs.freedesktop.org/show_bug.cgi?id=109594 - with this patch, totem app doesn't crashes at resizing Thanks! Tested-by: Paul Chelombitko чт, 21 февр. 2019 г. в 12:03, Eleni Maria Stea : > 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 > > v2: >- I initially clamped the values inside the if (Y is flipped) case >and I made a mistake in the calculation: the clamp of the bbox[2] should >be a check if (bbox[2] >= fbheight) bbox[2] = fbheight - 1 instead and I >shouldn't have changed the ScissorRectangleYMax calculation. As the >fixed code is equivalent with using CLAMP instead of MAX2 at the top of >the function when bbox[2] and bbox[3] are calculated, and the 2nd is > more >clear, I replaced it. (Nanley Chery) > > v3: >- Reversed the CLAMP change in bbox[3] as the API guarantees that the >viewport height is positive. (Nanley Chery) > --- > src/mesa/drivers/dri/i965/genX_state_upload.c | 2 +- > 1 file changed, 1 insertion(+), 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 dcdfb3c9292..47f3741e673 100644 > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c > @@ -2445,7 +2445,7 @@ set_scissor_bits(const struct gl_context *ctx, int i, > > bbox[0] = MAX2(ctx->ViewportArray[i].X, 0); > bbox[1] = MIN2(bbox[0] + ctx->ViewportArray[i].Width, fb_width); > - bbox[2] = MAX2(ctx->ViewportArray[i].Y, 0); > + bbox[2] = CLAMP(ctx->ViewportArray[i].Y, 0, fb_height); > bbox[3] = MIN2(bbox[2] + ctx->ViewportArray[i].Height, fb_height); > _mesa_intersect_scissor_bounding_box(ctx, i, bbox); > > -- > 2.20.1 > > ___ > 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 v3] 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 v2: - I initially clamped the values inside the if (Y is flipped) case and I made a mistake in the calculation: the clamp of the bbox[2] should be a check if (bbox[2] >= fbheight) bbox[2] = fbheight - 1 instead and I shouldn't have changed the ScissorRectangleYMax calculation. As the fixed code is equivalent with using CLAMP instead of MAX2 at the top of the function when bbox[2] and bbox[3] are calculated, and the 2nd is more clear, I replaced it. (Nanley Chery) v3: - Reversed the CLAMP change in bbox[3] as the API guarantees that the viewport height is positive. (Nanley Chery) --- src/mesa/drivers/dri/i965/genX_state_upload.c | 2 +- 1 file changed, 1 insertion(+), 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 dcdfb3c9292..47f3741e673 100644 --- a/src/mesa/drivers/dri/i965/genX_state_upload.c +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c @@ -2445,7 +2445,7 @@ set_scissor_bits(const struct gl_context *ctx, int i, bbox[0] = MAX2(ctx->ViewportArray[i].X, 0); bbox[1] = MIN2(bbox[0] + ctx->ViewportArray[i].Width, fb_width); - bbox[2] = MAX2(ctx->ViewportArray[i].Y, 0); + bbox[2] = CLAMP(ctx->ViewportArray[i].Y, 0, fb_height); bbox[3] = MIN2(bbox[2] + ctx->ViewportArray[i].Height, fb_height); _mesa_intersect_scissor_bounding_box(ctx, i, bbox); -- 2.20.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
