Re: [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.pitoi...@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.haeh...@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)

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev