[Mesa-dev] [PATCH 38/40] i965: Consider all viewports before enabling guardband clipping

[Kenneth Graunke]

On 01/10/2014 06:30 PM, Ian Romanick wrote:
> On 01/10/2014 06:24 PM, Kenneth Graunke wrote:
>> On 01/10/2014 05:40 PM, Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> ---
>>>  src/mesa/drivers/dri/i965/gen6_clip_state.c | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/gen6_clip_state.c b/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> index 3499e37..ed7afd7 100644
>>> --- a/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> +++ b/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> @@ -96,11 +96,15 @@ upload_clip_state(struct brw_context *brw)
>>>     dw2 |= (ctx->Transform.ClipPlanesEnabled <<
>>>             GEN6_USER_CLIP_CLIP_DISTANCES_SHIFT);
>>>  
>>> -   if (ctx->ViewportArray[0].X == 0 &&
>>> -       ctx->ViewportArray[0].Y == 0 &&
>>> -       ctx->ViewportArray[0].Width == (float) fb->Width &&
>>> -       ctx->ViewportArray[0].Height == (float) fb->Height) {
>>> -      dw2 |= GEN6_CLIP_GB_TEST;
>>> +   dw2 |= GEN6_CLIP_GB_TEST;
>>> +   for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) {
>>
>> Don't you want to check for actually enabled viewports here?  I'm not
>> sure what data the others get, but I imagine it's fairly bogus and thus
>> would result in guardband clipping always getting disabled.
> 
> There are three cases, I think...
> 
> In the common case, applications call glViewport, and all of the
> viewports get the same data.

Ah, you're right...you do set them all from glViewport...so your code is
fine as-is.

Objection withdrawn :)  Thanks for the explanation!

> In another case, applications call glViewportIndexedf (or one if its
> friends), and does rendering using dynamic viewport indexing.  In this
> case, we need to check all the viewports (as this code does).
> 
> In the final case, the application has called glViewportIndexedf (or one
> if its friends), but renders without using dynamic viewport indexing
> (e.g., no shader contains a static write to gl_ViewportIndex).  In this
> case we should be able to only check the first viewport.
> 
> However, at this point in the code, we don't have the data to
> differentiate the last two cases.  I have a small list of follow-on
> cleaning and optimization... can I add this to that list?

Yeah, I wouldn't worry about that now.  We could add it to a "ideas for
optimization someday" list somewhere.

>>> +      if (ctx->ViewportArray[i].X != 0 ||
>>> +          ctx->ViewportArray[i].Y != 0 ||
>>> +          ctx->ViewportArray[i].Width != (float) fb->Width ||
>>> +          ctx->ViewportArray[i].Height != (float) fb->Height) {
>>> +         dw2 &= ~GEN6_CLIP_GB_TEST;
>>> +         break;
>>> +      }
>>>     }
>>>  
>>>     /* BRW_NEW_RASTERIZER_DISCARD */
>>>
>