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

[Nanley Chery]

On Mon, Feb 25, 2019 at 03:40:24PM -0800, Nanley Chery wrote:
> On Mon, Feb 25, 2019 at 03:14:10PM -0800, Dylan Baker wrote:
> > Quoting Eleni Maria Stea (2019-02-22 13:02:30)
> > > 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
> > >           https://bugs.freedesktop.org/show_bug.cgi?id=109594
> > > 
> > > 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)
> > > 
> > > v4:
> > >   - Added nomination for the mesa-stable branch and the link to the second
> > >   bugzilla bug (Nanley Chery)
> > > 
> > > CC: <mesa-stable at lists.freedesktop.org>
> > > Tested-by: Paul Chelombitko <qamonstergl at gmail.com>
> > > Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>
> > > ---
> > >  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 027dad1e089..73c983ce742 100644
> > > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> > > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > > @@ -2446,7 +2446,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
> > > 
> > 
> > Do you have push access? I'd like to get this merged so we can close said bugs,
> > and Nanley or I can push this for you if you don't have access.
> > 
> 
> I haven't landed this patch because its piglit test isn't catching the
> error in CI. I'm hoping we could resolve that soon though.
> 

The test has been fixed, so I've pushed both patches.

> -Nanley