[Mesa-dev] [PATCH] panfrost: Don't flip scanout
Tomeu Vizoso
tomeu.vizoso at collabora.com
Fri May 31 13:29:27 UTC 2019
On Tue, 28 May 2019 at 08:17, Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
>
> 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>
Actually, the CI has found these regressions:
dEQP-GLES2.functional.fbo.render.resize.rbo_rgb565_stencil_index8
dEQP-GLES2.functional.fbo.render.stencil_clear.rbo_rgb565_stencil_index8
dEQP-GLES2.functional.fbo.render.stencil_clear.tex2d_rgb_stencil_index8
dEQP-GLES2.functional.fbo.render.stencil_clear.tex2d_rgba_stencil_index8
dEQP-GLES2.functional.shaders.builtin_variable.fragcoord_xyz
dEQP-GLES2.functional.shaders.builtin_variable.pointcoord
Guess there's some flipping in stencil and *coord that needs to be unflipped?
Thanks,
Tomeu
> 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");
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list