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

[Ian Romanick]

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.

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?

>> +      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 */
>>