[Mesa-dev] [PATCH 3/5] panfrost: Overhaul viewport code; fix scissor test

[Ilia Mirkin]

On Fri, Feb 8, 2019 at 8:27 PM Alyssa Rosenzweig <alyssa at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev