[Mesa-dev] [PATCH] panfrost: Don't flip scanout
Tomeu Vizoso
tomeu.vizoso at collabora.com
Tue May 28 06:17:00 UTC 2019
On 5/26/19 1:51 AM, Alyssa Rosenzweig wrote:
> The mesa/st flips the viewport, so we respect that rather than
> trying to flip the framebuffer itself and ignoring the viewport and
> using a messy heuristic.
>
> However, this brings an underlying disagreement about the interpretation
> of winding order to light. The blob uses a different strategy than Mesa
> for handling viewport Y flipping, so the meanings of the winding order
> bit are flipped for it. To keep things clean on our end, we rename to
> explicitly use Gallium (rather than flipped OpenGL) conventions.
>
> Fixes upside-down Xwayland/egl windows.
>
> Suggested-by: Rob Clark <robdclark at chromium.og>
> Signed-off-by: Alyssa Rosenzweig <alyssa at rosenzweig.io>
> Cc: Tomeu Vizoso <tomeu.vizoso at collabora.com>
This is a great cleanup, thanks!
Reviewed-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
Cheers,
Tomeu
> ---
> .../drivers/panfrost/include/panfrost-job.h | 8 +--
> src/gallium/drivers/panfrost/pan_context.c | 54 +++++++------------
> src/gallium/drivers/panfrost/pan_context.h | 7 +--
> src/gallium/drivers/panfrost/pan_fragment.c | 7 +--
> src/gallium/drivers/panfrost/pan_mfbd.c | 16 ++----
> src/gallium/drivers/panfrost/pan_sfbd.c | 17 ++----
> .../drivers/panfrost/pandecode/decode.c | 12 ++---
> 7 files changed, 40 insertions(+), 81 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/include/panfrost-job.h b/src/gallium/drivers/panfrost/include/panfrost-job.h
> index f4f145890de..8a4a7644070 100644
> --- a/src/gallium/drivers/panfrost/include/panfrost-job.h
> +++ b/src/gallium/drivers/panfrost/include/panfrost-job.h
> @@ -73,9 +73,11 @@ enum mali_draw_mode {
> #define MALI_OCCLUSION_QUERY (1 << 3)
> #define MALI_OCCLUSION_PRECISE (1 << 4)
>
> -#define MALI_FRONT_FACE(v) (v << 5)
> -#define MALI_CCW (0)
> -#define MALI_CW (1)
> +/* Set for a glFrontFace(GL_CCW) in a Y=0=TOP coordinate system (like Gallium).
> + * In OpenGL, this would corresponds to glFrontFace(GL_CW). Mesa and the blob
> + * disagree about how to do viewport flipping, so the blob actually sets this
> + * for GL_CW but then has a negative viewport stride */
> +#define MALI_FRONT_CCW_TOP (1 << 5)
>
> #define MALI_CULL_FACE_FRONT (1 << 6)
> #define MALI_CULL_FACE_BACK (1 << 7)
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index 5cae386f070..d0170a63def 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -1143,15 +1143,6 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>
> const struct pipe_viewport_state *vp = &ctx->pipe_viewport;
>
> - /* For flipped-Y buffers (signaled by negative scale), the translate is
> - * flipped as well */
> -
> - bool invert_y = vp->scale[1] < 0.0;
> - float translate_y = vp->translate[1];
> -
> - if (invert_y)
> - translate_y = ctx->pipe_framebuffer.height - translate_y;
> -
> for (int i = 0; i <= PIPE_SHADER_FRAGMENT; ++i) {
> struct panfrost_constant_buffer *buf = &ctx->constant_buffer[i];
>
> @@ -1171,11 +1162,11 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>
> if (sysval == PAN_SYSVAL_VIEWPORT_SCALE) {
> uniforms[4*i + 0] = vp->scale[0];
> - uniforms[4*i + 1] = fabsf(vp->scale[1]);
> + uniforms[4*i + 1] = vp->scale[1];
> uniforms[4*i + 2] = vp->scale[2];
> } else if (sysval == PAN_SYSVAL_VIEWPORT_OFFSET) {
> uniforms[4*i + 0] = vp->translate[0];
> - uniforms[4*i + 1] = translate_y;
> + uniforms[4*i + 1] = vp->translate[1];
> uniforms[4*i + 2] = vp->translate[2];
> } else {
> assert(0);
> @@ -1245,24 +1236,28 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
> view.viewport0[0] = (int) (vp->translate[0] - vp->scale[0]);
> view.viewport1[0] = MALI_POSITIVE((int) (vp->translate[0] + vp->scale[0]));
>
> - view.viewport0[1] = (int) (translate_y - fabs(vp->scale[1]));
> - view.viewport1[1] = MALI_POSITIVE((int) (translate_y + fabs(vp->scale[1])));
> + int miny = (int) (vp->translate[1] - vp->scale[1]);
> + int maxy = (int) (vp->translate[1] + vp->scale[1]);
>
> if (ss && ctx->rasterizer && ctx->rasterizer->base.scissor) {
> - /* Invert scissor if needed */
> - unsigned miny = invert_y ?
> - ctx->pipe_framebuffer.height - ss->maxy : ss->miny;
> -
> - unsigned maxy = invert_y ?
> - ctx->pipe_framebuffer.height - ss->miny : ss->maxy;
> -
> - /* Set the actual scissor */
> view.viewport0[0] = ss->minx;
> - view.viewport0[1] = miny;
> view.viewport1[0] = MALI_POSITIVE(ss->maxx);
> - view.viewport1[1] = MALI_POSITIVE(maxy);
> +
> + miny = ss->miny;
> + maxy = ss->maxy;
> }
>
> + /* Hardware needs the min/max to be strictly ordered, so flip if we
> + * need to */
> + if (miny > maxy) {
> + int temp = miny;
> + miny = maxy;
> + maxy = temp;
> + }
> +
> + view.viewport0[1] = miny;
> + view.viewport1[1] = MALI_POSITIVE(maxy);
> +
> ctx->payload_tiler.postfix.viewport =
> panfrost_upload_transient(ctx,
> &view,
> @@ -1597,8 +1592,8 @@ panfrost_create_rasterizer_state(
> /* Bitmask, unknown meaning of the start value */
> so->tiler_gl_enables = ctx->is_t6xx ? 0x105 : 0x7;
>
> - so->tiler_gl_enables |= MALI_FRONT_FACE(
> - cso->front_ccw ? MALI_CCW : MALI_CW);
> + if (cso->front_ccw)
> + so->tiler_gl_enables |= MALI_FRONT_CCW_TOP;
>
> if (cso->cull_face & PIPE_FACE_FRONT)
> so->tiler_gl_enables |= MALI_CULL_FACE_FRONT;
> @@ -2305,15 +2300,6 @@ panfrost_set_viewport_states(struct pipe_context *pipe,
> assert(num_viewports == 1);
>
> 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
> }
>
> static void
> diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
> index 7f08d471511..81937c0d3c7 100644
> --- a/src/gallium/drivers/panfrost/pan_context.h
> +++ b/src/gallium/drivers/panfrost/pan_context.h
> @@ -340,11 +340,8 @@ panfrost_flush(
> bool
> panfrost_is_scanout(struct panfrost_context *ctx);
>
> -mali_ptr
> -panfrost_sfbd_fragment(struct panfrost_context *ctx, bool flip_y);
> -
> -mali_ptr
> -panfrost_mfbd_fragment(struct panfrost_context *ctx, bool flip_y);
> +mali_ptr panfrost_sfbd_fragment(struct panfrost_context *ctx);
> +mali_ptr panfrost_mfbd_fragment(struct panfrost_context *ctx);
>
> struct bifrost_framebuffer
> panfrost_emit_mfbd(struct panfrost_context *ctx);
> diff --git a/src/gallium/drivers/panfrost/pan_fragment.c b/src/gallium/drivers/panfrost/pan_fragment.c
> index 0dc15c2d235..16405a4ed21 100644
> --- a/src/gallium/drivers/panfrost/pan_fragment.c
> +++ b/src/gallium/drivers/panfrost/pan_fragment.c
> @@ -34,12 +34,9 @@
> mali_ptr
> panfrost_fragment_job(struct panfrost_context *ctx)
> {
> - /* TODO: Check viewport or something */
> - bool flip_y = panfrost_is_scanout(ctx);
> -
> mali_ptr framebuffer = ctx->require_sfbd ?
> - panfrost_sfbd_fragment(ctx, flip_y) :
> - panfrost_mfbd_fragment(ctx, flip_y);
> + panfrost_sfbd_fragment(ctx) :
> + panfrost_mfbd_fragment(ctx);
>
> struct mali_job_descriptor_header header = {
> .job_type = JOB_TYPE_FRAGMENT,
> diff --git a/src/gallium/drivers/panfrost/pan_mfbd.c b/src/gallium/drivers/panfrost/pan_mfbd.c
> index 6986992012f..78d676511d6 100644
> --- a/src/gallium/drivers/panfrost/pan_mfbd.c
> +++ b/src/gallium/drivers/panfrost/pan_mfbd.c
> @@ -85,8 +85,7 @@ panfrost_mfbd_clear(
> static void
> panfrost_mfbd_set_cbuf(
> struct bifrost_render_target *rt,
> - struct pipe_surface *surf,
> - bool flip_y)
> + struct pipe_surface *surf)
> {
> struct panfrost_resource *rsrc = pan_resource(surf->texture);
> int stride = rsrc->bo->slices[0].stride;
> @@ -96,14 +95,7 @@ panfrost_mfbd_set_cbuf(
> /* Now, we set the layout specific pieces */
>
> if (rsrc->bo->layout == PAN_LINEAR) {
> - mali_ptr framebuffer = rsrc->bo->gpu;
> -
> - if (flip_y) {
> - framebuffer += stride * (surf->texture->height0 - 1);
> - stride = -stride;
> - }
> -
> - rt->framebuffer = framebuffer;
> + rt->framebuffer = rsrc->bo->gpu;
> rt->framebuffer_stride = stride / 16;
> } else if (rsrc->bo->layout == PAN_AFBC) {
> rt->afbc.metadata = rsrc->bo->afbc_slab.gpu;
> @@ -211,7 +203,7 @@ panfrost_mfbd_upload(
> /* Creates an MFBD for the FRAGMENT section of the bound framebuffer */
>
> mali_ptr
> -panfrost_mfbd_fragment(struct panfrost_context *ctx, bool flip_y)
> +panfrost_mfbd_fragment(struct panfrost_context *ctx)
> {
> struct panfrost_job *job = panfrost_get_job_for_fbo(ctx);
>
> @@ -228,7 +220,7 @@ panfrost_mfbd_fragment(struct panfrost_context *ctx, bool flip_y)
>
> for (int cb = 0; cb < ctx->pipe_framebuffer.nr_cbufs; ++cb) {
> struct pipe_surface *surf = ctx->pipe_framebuffer.cbufs[cb];
> - panfrost_mfbd_set_cbuf(&rts[cb], surf, flip_y);
> + panfrost_mfbd_set_cbuf(&rts[cb], surf);
> }
>
> if (ctx->pipe_framebuffer.zsbuf) {
> diff --git a/src/gallium/drivers/panfrost/pan_sfbd.c b/src/gallium/drivers/panfrost/pan_sfbd.c
> index 4235c198199..6989cd925a8 100644
> --- a/src/gallium/drivers/panfrost/pan_sfbd.c
> +++ b/src/gallium/drivers/panfrost/pan_sfbd.c
> @@ -87,8 +87,7 @@ panfrost_sfbd_clear(
> static void
> panfrost_sfbd_set_cbuf(
> struct mali_single_framebuffer *fb,
> - struct pipe_surface *surf,
> - bool flip_y)
> + struct pipe_surface *surf)
> {
> struct panfrost_resource *rsrc = pan_resource(surf->texture);
>
> @@ -97,15 +96,7 @@ panfrost_sfbd_set_cbuf(
> fb->format = panfrost_sfbd_format(surf);
>
> if (rsrc->bo->layout == PAN_LINEAR) {
> - mali_ptr framebuffer = rsrc->bo->gpu;
> -
> - /* The default is upside down from OpenGL's perspective. */
> - if (flip_y) {
> - framebuffer += stride * (surf->texture->height0 - 1);
> - stride = -stride;
> - }
> -
> - fb->framebuffer = framebuffer;
> + fb->framebuffer = rsrc->bo->gpu;
> fb->stride = stride;
> } else {
> fprintf(stderr, "Invalid render layout\n");
> @@ -116,7 +107,7 @@ panfrost_sfbd_set_cbuf(
> /* Creates an SFBD for the FRAGMENT section of the bound framebuffer */
>
> mali_ptr
> -panfrost_sfbd_fragment(struct panfrost_context *ctx, bool flip_y)
> +panfrost_sfbd_fragment(struct panfrost_context *ctx)
> {
> struct panfrost_job *job = panfrost_get_job_for_fbo(ctx);
> struct mali_single_framebuffer fb = panfrost_emit_sfbd(ctx);
> @@ -125,7 +116,7 @@ panfrost_sfbd_fragment(struct panfrost_context *ctx, bool flip_y)
>
> /* SFBD does not support MRT natively; sanity check */
> assert(ctx->pipe_framebuffer.nr_cbufs == 1);
> - panfrost_sfbd_set_cbuf(&fb, ctx->pipe_framebuffer.cbufs[0], flip_y);
> + panfrost_sfbd_set_cbuf(&fb, ctx->pipe_framebuffer.cbufs[0]);
>
> if (ctx->pipe_framebuffer.zsbuf) {
> /* TODO */
> diff --git a/src/gallium/drivers/panfrost/pandecode/decode.c b/src/gallium/drivers/panfrost/pandecode/decode.c
> index fa91094d1f0..dac27c36684 100644
> --- a/src/gallium/drivers/panfrost/pandecode/decode.c
> +++ b/src/gallium/drivers/panfrost/pandecode/decode.c
> @@ -147,10 +147,11 @@ pandecode_log_decoded_flags(const struct pandecode_flag_info *flag_info,
>
> #define FLAG_INFO(flag) { MALI_##flag, "MALI_" #flag }
> static const struct pandecode_flag_info gl_enable_flag_info[] = {
> - FLAG_INFO(CULL_FACE_FRONT),
> - FLAG_INFO(CULL_FACE_BACK),
> FLAG_INFO(OCCLUSION_QUERY),
> FLAG_INFO(OCCLUSION_PRECISE),
> + FLAG_INFO(FRONT_CCW_TOP),
> + FLAG_INFO(CULL_FACE_FRONT),
> + FLAG_INFO(CULL_FACE_BACK),
> {}
> };
> #undef FLAG_INFO
> @@ -1763,13 +1764,6 @@ pandecode_replay_gl_enables(uint32_t gl_enables, int job_type)
> {
> pandecode_log(".gl_enables = ");
>
> - if (job_type == JOB_TYPE_TILER) {
> - pandecode_log_cont("MALI_FRONT_FACE(MALI_%s) | ",
> - gl_enables & MALI_FRONT_FACE(MALI_CW) ? "CW" : "CCW");
> -
> - gl_enables &= ~(MALI_FRONT_FACE(1));
> - }
> -
> pandecode_log_decoded_flags(gl_enable_flag_info, gl_enables);
>
> pandecode_log_cont(",\n");
>
More information about the mesa-dev
mailing list