[Mesa-dev] [PATCH 2/2] mesa: clamp viewport values only once when using glViewport()

[Samuel Pitoiset]

On 07/31/2017 12:02 PM, Nicolai Hähnle wrote:
> On 28.07.2017 15:47, Samuel Pitoiset wrote:
>> It's useless to clamp the same values for all viewports.
>>
>> +7% in the "viewport change" test (drawoverhead benchmark).
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> 
> I think it would be cleaner to call clamp_viewport in all callers of 
> set_viewport_no_notify. Fewer boolean function args is better and 
> clearer in my book.

Yes, I wasn't sure myself, but I do agree, thanks!

> 
> With that changed, the patch is
> 
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> 
>> ---
>>   src/mesa/main/viewport.c | 36 ++++++++++++++++++++++++------------
>>   1 file changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/mesa/main/viewport.c b/src/mesa/main/viewport.c
>> index e6004bad40..1cff4d846e 100644
>> --- a/src/mesa/main/viewport.c
>> +++ b/src/mesa/main/viewport.c
>> @@ -36,13 +36,12 @@
>>   #include "viewport.h"
>>   static void
>> -set_viewport_no_notify(struct gl_context *ctx, unsigned idx,
>> -                       GLfloat x, GLfloat y,
>> -                       GLfloat width, GLfloat height)
>> +clamp_viewport(struct gl_context *ctx, GLfloat *x, GLfloat *y,
>> +               GLfloat *width, GLfloat *height)
>>   {
>>      /* clamp width and height to the implementation dependent range */
>> -   width  = MIN2(width, (GLfloat) ctx->Const.MaxViewportWidth);
>> -   height = MIN2(height, (GLfloat) ctx->Const.MaxViewportHeight);
>> +   *width  = MIN2(*width, (GLfloat) ctx->Const.MaxViewportWidth);
>> +   *height = MIN2(*height, (GLfloat) ctx->Const.MaxViewportHeight);
>>      /* The GL_ARB_viewport_array spec says:
>>       *
>> @@ -55,11 +54,20 @@ set_viewport_no_notify(struct gl_context *ctx, 
>> unsigned idx,
>>      if (ctx->Extensions.ARB_viewport_array ||
>>          (ctx->Extensions.OES_viewport_array &&
>>           _mesa_is_gles31(ctx))) {
>> -      x = CLAMP(x,
>> -                ctx->Const.ViewportBounds.Min, 
>> ctx->Const.ViewportBounds.Max);
>> -      y = CLAMP(y,
>> -                ctx->Const.ViewportBounds.Min, 
>> ctx->Const.ViewportBounds.Max);
>> +      *x = CLAMP(*x,
>> +                 ctx->Const.ViewportBounds.Min, 
>> ctx->Const.ViewportBounds.Max);
>> +      *y = CLAMP(*y,
>> +                 ctx->Const.ViewportBounds.Min, 
>> ctx->Const.ViewportBounds.Max);
>>      }
>> +}
>> +
>> +static void
>> +set_viewport_no_notify(struct gl_context *ctx, unsigned idx,
>> +                       GLfloat x, GLfloat y,
>> +                       GLfloat width, GLfloat height, bool clamped)
>> +{
>> +   if (!clamped)
>> +      clamp_viewport(ctx, &x, &y, &width, &height);
>>      if (ctx->ViewportArray[idx].X == x &&
>>          ctx->ViewportArray[idx].Width == width &&
>> @@ -89,6 +97,10 @@ static void
>>   viewport(struct gl_context *ctx, GLint x, GLint y, GLsizei width,
>>            GLsizei height)
>>   {
>> +   /* Clamp the viewport to the implementation dependent values. */
>> +   clamp_viewport(ctx, (GLfloat *)&x, (GLfloat *)&y,
>> +                  (GLfloat *)&width, (GLfloat *)&height);
>> +
>>      /* The GL_ARB_viewport_array spec says:
>>       *
>>       *     "Viewport sets the parameters for all viewports to the 
>> same values
>> @@ -101,7 +113,7 @@ viewport(struct gl_context *ctx, GLint x, GLint y, 
>> GLsizei width,
>>       * signal the driver once at the end.
>>       */
>>      for (unsigned i = 0; i < ctx->Const.MaxViewports; i++)
>> -      set_viewport_no_notify(ctx, i, x, y, width, height);
>> +      set_viewport_no_notify(ctx, i, x, y, width, height, true);
>>      if (ctx->Driver.Viewport)
>>         ctx->Driver.Viewport(ctx);
>> @@ -153,7 +165,7 @@ void
>>   _mesa_set_viewport(struct gl_context *ctx, unsigned idx, GLfloat x, 
>> GLfloat y,
>>                       GLfloat width, GLfloat height)
>>   {
>> -   set_viewport_no_notify(ctx, idx, x, y, width, height);
>> +   set_viewport_no_notify(ctx, idx, x, y, width, height, false);
>>      if (ctx->Driver.Viewport)
>>         ctx->Driver.Viewport(ctx);
>> @@ -165,7 +177,7 @@ viewport_array(struct gl_context *ctx, GLuint 
>> first, GLsizei count,
>>   {
>>      for (GLsizei i = 0; i < count; i++) {
>>         set_viewport_no_notify(ctx, i + first, inputs[i].X, inputs[i].Y,
>> -                             inputs[i].Width, inputs[i].Height);
>> +                             inputs[i].Width, inputs[i].Height, false);
>>      }
>>      if (ctx->Driver.Viewport)
>>
> 
>