[3/5] panfrost: Overhaul viewport code; fix scissor test

Submitted by Alyssa Rosenzweig on Feb. 9, 2019, 1:25 a.m.

Details

Message ID 20190209012541.3484-1-alyssa@rosenzweig.io
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alyssa Rosenzweig Feb. 9, 2019, 1:25 a.m.
The previous code for handling viewport changes and scissors was ad hoc
and subject to edge cases. This patch unifies and streamlines these
routines, improving code quality and partially fixing the scissor test.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 src/gallium/drivers/panfrost/pan_context.c | 94 +++++++++++++---------
 1 file changed, 55 insertions(+), 39 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
index bc92933ccda..31c0029ac1a 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -492,33 +492,42 @@  panfrost_attach_vt_framebuffer(struct panfrost_context *ctx)
         ctx->payload_tiler.postfix.framebuffer = framebuffer;
 }
 
+/* Helpers for filling out mali_viewport structure */
+
 static void
-panfrost_viewport(struct panfrost_context *ctx,
-                  float depth_range_n,
-                  float depth_range_f,
-                  int viewport_x0, int viewport_y0,
-                  int viewport_x1, int viewport_y1)
+panfrost_set_viewport_bounds(struct panfrost_context *ctx, bool scissored, 
+                unsigned minx, unsigned miny, unsigned maxx, unsigned maxy)
 {
-        /* Viewport encoding is asymmetric. Purpose of the floats is unknown? */
+        /* Set main viewport fields */
 
-        struct mali_viewport ret = {
-                .floats = {
-#if 0
-                        -inff, -inff,
-                        inff, inff,
-#endif
-                        0.0, 0.0,
-                        2048.0, 1600.0,
-                },
+        ctx->viewport->viewport0[0] = minx;
+        ctx->viewport->viewport0[1] = miny;
 
-                .depth_range_n = depth_range_n,
-                .depth_range_f = depth_range_f,
+        ctx->viewport->viewport1[0] = MALI_POSITIVE(maxx);
+        ctx->viewport->viewport1[1] = MALI_POSITIVE(maxy);
 
-                .viewport0 = { viewport_x0, viewport_y0 },
-                .viewport1 = { MALI_POSITIVE(viewport_x1), MALI_POSITIVE(viewport_y1) },
-        };
+        /* Set the floats if we're scissoring */
+        if (scissored) {
+                /* Clip to the scissor bounds */
+
+                ctx->viewport->floats[0] = minx;
+                ctx->viewport->floats[1] = miny;
+                ctx->viewport->floats[2] = maxx;
+                ctx->viewport->floats[3] = maxy;
+        } else {
+                /* Essentially, clip to (-inf, inf) */
+
+                ctx->viewport->floats[0] = ctx->viewport->floats[1] = -inff;
+                ctx->viewport->floats[2] = ctx->viewport->floats[3] = +inff;
+        }
+}
 
-        memcpy(ctx->viewport, &ret, sizeof(ret));
+static void
+panfrost_set_viewport_depth(struct panfrost_context *ctx,
+                float depth_near, float depth_far)
+{
+        ctx->viewport->depth_range_n = depth_near;
+        ctx->viewport->depth_range_f = depth_far;
 }
 
 /* Reset per-frame context, called on context initialisation as well as after
@@ -1698,16 +1707,25 @@  panfrost_set_scissor(struct panfrost_context *ctx)
 {
         const struct pipe_scissor_state *ss = &ctx->scissor;
 
-        if (ss && ctx->rasterizer && ctx->rasterizer->base.scissor && 0) {
-                ctx->viewport->viewport0[0] = ss->minx;
-                ctx->viewport->viewport0[1] = ss->miny;
-                ctx->viewport->viewport1[0] = MALI_POSITIVE(ss->maxx);
-                ctx->viewport->viewport1[1] = MALI_POSITIVE(ss->maxy);
+        if (ss && ctx->rasterizer && ctx->rasterizer->base.scissor) {
+                panfrost_set_viewport_bounds(ctx, true,
+                                ss->minx, ss->miny, ss->maxx, ss->maxy);
         } else {
-                ctx->viewport->viewport0[0] = 0;
-                ctx->viewport->viewport0[1] = 0;
-                ctx->viewport->viewport1[0] = MALI_POSITIVE(ctx->pipe_framebuffer.width);
-                ctx->viewport->viewport1[1] = MALI_POSITIVE(ctx->pipe_framebuffer.height);
+                /* Default to entire viewport */
+
+                float mx = abs(ctx->pipe_viewport.translate[0]);
+                float my = abs(ctx->pipe_viewport.translate[1]);
+                float w = abs(ctx->pipe_viewport.scale[0]);
+                float h = abs(ctx->pipe_viewport.scale[1]);
+
+                /* Compute the bounds based on the viewport floats */
+
+                float minx = mx - w, maxx = mx + w;
+                float miny = my - h, maxy = my + h;
+
+                /* Actualize the bounds */
+
+                panfrost_set_viewport_bounds(ctx, false, minx, miny, maxx, maxy);
         }
 }
 
@@ -2426,14 +2444,8 @@  panfrost_set_viewport_states(struct pipe_context *pipe,
 
         ctx->pipe_viewport = *viewports;
 
-#if 0
-        /* TODO: What if not centered? */
-        float w = abs(viewports->scale[0]) * 2.0;
-        float h = abs(viewports->scale[1]) * 2.0;
-
-        ctx->viewport.viewport1[0] = MALI_POSITIVE((int) w);
-        ctx->viewport.viewport1[1] = MALI_POSITIVE((int) h);
-#endif
+        /* Actualize update in the cmdstream */
+        panfrost_set_scissor(ctx);
 }
 
 static void
@@ -2682,7 +2694,11 @@  panfrost_create_context(struct pipe_screen *screen, void *priv, unsigned flags)
         panfrost_emit_vertex_payload(ctx);
         panfrost_emit_tiler_payload(ctx);
         panfrost_invalidate_frame(ctx);
-        panfrost_viewport(ctx, 0.0, 1.0, 0, 0, ctx->pipe_framebuffer.width, ctx->pipe_framebuffer.height);
+
+        /* Setup viewport implicitly via scissor */
+        panfrost_set_scissor(ctx);
+        panfrost_set_viewport_depth(ctx, 0.0, 1.0);
+
         panfrost_default_shader_backend(ctx);
         panfrost_generate_space_filler_indices();
 

Comments

On Fri, Feb 8, 2019 at 8:27 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> The previous code for handling viewport changes and scissors was ad hoc
> and subject to edge cases. This patch unifies and streamlines these
> routines, improving code quality and partially fixing the scissor test.
>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  src/gallium/drivers/panfrost/pan_context.c | 94 +++++++++++++---------
>  1 file changed, 55 insertions(+), 39 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index bc92933ccda..31c0029ac1a 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -492,33 +492,42 @@ panfrost_attach_vt_framebuffer(struct panfrost_context *ctx)
>          ctx->payload_tiler.postfix.framebuffer = framebuffer;
>  }
>
> +/* Helpers for filling out mali_viewport structure */
> +
>  static void
> -panfrost_viewport(struct panfrost_context *ctx,
> -                  float depth_range_n,
> -                  float depth_range_f,
> -                  int viewport_x0, int viewport_y0,
> -                  int viewport_x1, int viewport_y1)
> +panfrost_set_viewport_bounds(struct panfrost_context *ctx, bool scissored,
> +                unsigned minx, unsigned miny, unsigned maxx, unsigned maxy)
>  {
> -        /* Viewport encoding is asymmetric. Purpose of the floats is unknown? */
> +        /* Set main viewport fields */
>
> -        struct mali_viewport ret = {
> -                .floats = {
> -#if 0
> -                        -inff, -inff,
> -                        inff, inff,
> -#endif
> -                        0.0, 0.0,
> -                        2048.0, 1600.0,
> -                },
> +        ctx->viewport->viewport0[0] = minx;
> +        ctx->viewport->viewport0[1] = miny;
>
> -                .depth_range_n = depth_range_n,
> -                .depth_range_f = depth_range_f,
> +        ctx->viewport->viewport1[0] = MALI_POSITIVE(maxx);
> +        ctx->viewport->viewport1[1] = MALI_POSITIVE(maxy);
>
> -                .viewport0 = { viewport_x0, viewport_y0 },
> -                .viewport1 = { MALI_POSITIVE(viewport_x1), MALI_POSITIVE(viewport_y1) },
> -        };
> +        /* Set the floats if we're scissoring */
> +        if (scissored) {
> +                /* Clip to the scissor bounds */
> +
> +                ctx->viewport->floats[0] = minx;
> +                ctx->viewport->floats[1] = miny;
> +                ctx->viewport->floats[2] = maxx;
> +                ctx->viewport->floats[3] = maxy;
> +        } else {
> +                /* Essentially, clip to (-inf, inf) */
> +
> +                ctx->viewport->floats[0] = ctx->viewport->floats[1] = -inff;
> +                ctx->viewport->floats[2] = ctx->viewport->floats[3] = +inff;
> +        }
> +}
>
> -        memcpy(ctx->viewport, &ret, sizeof(ret));
> +static void
> +panfrost_set_viewport_depth(struct panfrost_context *ctx,
> +                float depth_near, float depth_far)
> +{
> +        ctx->viewport->depth_range_n = depth_near;
> +        ctx->viewport->depth_range_f = depth_far;
>  }
>
>  /* Reset per-frame context, called on context initialisation as well as after
> @@ -1698,16 +1707,25 @@ panfrost_set_scissor(struct panfrost_context *ctx)
>  {
>          const struct pipe_scissor_state *ss = &ctx->scissor;
>
> -        if (ss && ctx->rasterizer && ctx->rasterizer->base.scissor && 0) {
> -                ctx->viewport->viewport0[0] = ss->minx;
> -                ctx->viewport->viewport0[1] = ss->miny;
> -                ctx->viewport->viewport1[0] = MALI_POSITIVE(ss->maxx);
> -                ctx->viewport->viewport1[1] = MALI_POSITIVE(ss->maxy);
> +        if (ss && ctx->rasterizer && ctx->rasterizer->base.scissor) {
> +                panfrost_set_viewport_bounds(ctx, true,
> +                                ss->minx, ss->miny, ss->maxx, ss->maxy);
>          } else {
> -                ctx->viewport->viewport0[0] = 0;
> -                ctx->viewport->viewport0[1] = 0;
> -                ctx->viewport->viewport1[0] = MALI_POSITIVE(ctx->pipe_framebuffer.width);
> -                ctx->viewport->viewport1[1] = MALI_POSITIVE(ctx->pipe_framebuffer.height);
> +                /* Default to entire viewport */
> +
> +                float mx = abs(ctx->pipe_viewport.translate[0]);
> +                float my = abs(ctx->pipe_viewport.translate[1]);
> +                float w = abs(ctx->pipe_viewport.scale[0]);
> +                float h = abs(ctx->pipe_viewport.scale[1]);
> +
> +                /* Compute the bounds based on the viewport floats */
> +
> +                float minx = mx - w, maxx = mx + w;
> +                float miny = my - h, maxy = my + h;
> +
> +                /* Actualize the bounds */
> +
> +                panfrost_set_viewport_bounds(ctx, false, minx, miny, maxx, maxy);
>          }
>  }
>
> @@ -2426,14 +2444,8 @@ panfrost_set_viewport_states(struct pipe_context *pipe,
>
>          ctx->pipe_viewport = *viewports;
>
> -#if 0
> -        /* TODO: What if not centered? */
> -        float w = abs(viewports->scale[0]) * 2.0;
> -        float h = abs(viewports->scale[1]) * 2.0;
> -
> -        ctx->viewport.viewport1[0] = MALI_POSITIVE((int) w);
> -        ctx->viewport.viewport1[1] = MALI_POSITIVE((int) h);
> -#endif
> +        /* Actualize update in the cmdstream */
> +        panfrost_set_scissor(ctx);

It seems surprising that you don't set the depth range here. It should
be translate[2] +/- scale[2] (unless halfz is set, then it's a bit
different. There's a util_viewport_zmin_zmax to help with that...
depending on how inverted near/far need to be handled).

>  }
>
>  static void
> @@ -2682,7 +2694,11 @@ panfrost_create_context(struct pipe_screen *screen, void *priv, unsigned flags)
>          panfrost_emit_vertex_payload(ctx);
>          panfrost_emit_tiler_payload(ctx);
>          panfrost_invalidate_frame(ctx);
> -        panfrost_viewport(ctx, 0.0, 1.0, 0, 0, ctx->pipe_framebuffer.width, ctx->pipe_framebuffer.height);
> +
> +        /* Setup viewport implicitly via scissor */
> +        panfrost_set_scissor(ctx);
> +        panfrost_set_viewport_depth(ctx, 0.0, 1.0);
> +
>          panfrost_default_shader_backend(ctx);
>          panfrost_generate_space_filler_indices();
>
> --
> 2.20.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev