panfrost: Questions regarding pan_wallpaper.c (and the 'reload FB content' logic in general)

Submitted by Boris Brezillon on May 3, 2019, 10:44 a.m.

Details

Message ID 20190503124409.63407aa8@collabora.com
State New
Headers show
Series "panfrost: Questions regarding pan_wallpaper.c (and the 'reload FB content' logic in general)" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon May 3, 2019, 10:44 a.m.
Hello Alyssa,

This week I've tried to make the wallpapering logic to work on panfrost
but I'm not quite there yet.

I've done all my tests with weston that tries to use buffer_age and
update only the parts that have changed (which I know is inefficient,
but before implementing partial_update() I need to have the 'reload
FB content into tile buf' stuff working).

Looks like with the below diff applied, weston background is completely
replaced by a black color and the panel is missing.

Interestingly, if I update the texcoord/pos varyings to duplicate the
FB to half the output height it seems to draw something valid on the
screen, but in any case, everytime I open a terminal window the output
is all messed up (meaning that the 'reload' logic does not work properly).

I know you're working other topics right and don't necessarily have the
time to have a deeper look into that code, but maybe you have some
ideas on what could be wrong in my code/approach or have some tricks
to debug this kind of issues.

Also had a look at the reload logic in the lima driver and the driver
seems to do something similar. Quiang, Vasily, any ideas?

Thanks in advance for your help,

Boris

NOTE: I also tried with an u_blitter based implementation and it produces
the same effects.

--->8---

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 c50c546a3995..46d82b2e19c2 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -1314,6 +1314,8 @@  panfrost_submit_frame(struct panfrost_context *ctx, bool flush_immediate,
         /* Workaround a bizarre lockup (a hardware errata?) */
         if (!has_draws)
                 flush_immediate = true;
+        else if (!(job->clear & PIPE_CLEAR_COLOR))
+                panfrost_draw_wallpaper(gallium);
 
         /* A number of jobs are batched -- this must be linked and cleared */
         panfrost_link_jobs(ctx);
diff --git a/src/gallium/drivers/panfrost/pan_wallpaper.c b/src/gallium/drivers/panfrost/pan_wallpaper.c
index ac77ad089bc1..96a4f87723e0 100644
--- a/src/gallium/drivers/panfrost/pan_wallpaper.c
+++ b/src/gallium/drivers/panfrost/pan_wallpaper.c
@@ -100,7 +100,9 @@  panfrost_create_wallpaper_program(struct pipe_context *pctx)
 }
 
 static struct panfrost_shader_variants *wallpaper_program = NULL;
+static struct panfrost_blend_state *wallpaper_blend = NULL;
 static struct panfrost_shader_variants *wallpaper_saved_program = NULL;
+static struct panfrost_blend_state *wallpaper_saved_blend = NULL;
 
 static void
 panfrost_enable_wallpaper_program(struct pipe_context *pctx)
@@ -108,14 +110,28 @@  panfrost_enable_wallpaper_program(struct pipe_context *pctx)
         struct panfrost_context *ctx = pan_context(pctx);
 
         if (!wallpaper_program) {
+               struct pipe_blend_state bs = {
+                       .rt[0].blend_enable = 1,
+                       .rt[0].rgb_func = PIPE_BLEND_ADD,
+                       .rt[0].rgb_src_factor = PIPE_BLENDFACTOR_ONE,
+                       .rt[0].rgb_dst_factor = PIPE_BLENDFACTOR_ZERO,
+                       .rt[0].alpha_func = PIPE_BLEND_ADD,
+                       .rt[0].alpha_src_factor = PIPE_BLENDFACTOR_ZERO,
+                       .rt[0].alpha_dst_factor = PIPE_BLENDFACTOR_ONE,
+                       .rt[0].colormask = PIPE_MASK_R | PIPE_MASK_G | PIPE_MASK_B | PIPE_MASK_A,
+               };
+
                 wallpaper_program = panfrost_create_wallpaper_program(pctx);
+               wallpaper_blend = pctx->create_blend_state(pctx, &bs);
         }
 
         /* Push the shader state */
         wallpaper_saved_program = ctx->fs;
+        wallpaper_saved_blend = ctx->blend;
 
         /* Bind the program */
         pctx->bind_fs_state(pctx, wallpaper_program);
+        pctx->bind_blend_state(pctx, wallpaper_blend);
 }
 
 static void
@@ -123,6 +139,7 @@  panfrost_disable_wallpaper_program(struct pipe_context *pctx)
 {
         /* Pop off the shader state */
         pctx->bind_fs_state(pctx, wallpaper_saved_program);
+        pctx->bind_blend_state(pctx, wallpaper_saved_blend);
 }
 
 /* Essentially, we insert a fullscreen textured quad, reading from the
@@ -133,12 +150,7 @@  panfrost_draw_wallpaper(struct pipe_context *pipe)
 {
         /* Disable wallpapering for now, but still exercise the shader generation to minimise bit rot */
 
-        panfrost_enable_wallpaper_program(pipe);
-        panfrost_disable_wallpaper_program(pipe);
-
-        return;
-
-#if 0
+#if 1
         struct panfrost_context *ctx = pan_context(pipe);
 
         /* Setup payload for elided quad. TODO: Refactor draw_vbo so this can
@@ -180,12 +192,12 @@  panfrost_draw_wallpaper(struct pipe_context *pipe)
                 .normalized_coords = 1
         };
 
-        struct pipe_resource *rsrc = pan_screen(pipe->screen)->display_target;
+        struct pipe_resource *rsrc = &pan_screen(pipe->screen)->display_target->base;
         struct pipe_sampler_state *sampler_state = pipe->create_sampler_state(pipe, &state);
         struct pipe_sampler_view *sampler_view = pipe->create_sampler_view(pipe, rsrc, &tmpl);
 
         /* Bind texture/sampler. TODO: push/pop */
-        pipe->bind_sampler_states(pipe, PIPE_SHADER_FRAGMENT, 0, 1, &sampler_state);
+        pipe->bind_sampler_states(pipe, PIPE_SHADER_FRAGMENT, 0, 1, (void **)&sampler_state);
         pipe->set_sampler_views(pipe, PIPE_SHADER_FRAGMENT, 0, 1, &sampler_view);
 
         panfrost_emit_for_draw(ctx, false);
@@ -205,10 +217,16 @@  panfrost_draw_wallpaper(struct pipe_context *pipe)
 //                65536.0, 65536.0,  0.0, 1.0
 
                 /* The following output is correct for a fullscreen quad with screen size 2048x1600 */
+               /*
                 0.0, 0.0, 0.0, 1.0,
                 0.0, 1600.0, 0.0, 1.0,
                 2048.0, 0.0, 0.0, 1.0,
                 2048.0, 1280.0, 0.0, 1.0,
+               */
+                0.0, ctx->pipe_framebuffer.height, 0.0, 1.0,
+                0.0, 0.0, 0.0, 1.0,
+                ctx->pipe_framebuffer.width, ctx->pipe_framebuffer.height, 0.0, 1.0,
+                ctx->pipe_framebuffer.width, 0.0, 0.0, 1.0,
         };
 
         ctx->payload_tiler.postfix.position_varying = panfrost_upload_transient(ctx, implied_position_varying, sizeof(implied_position_varying));
@@ -235,11 +253,9 @@  panfrost_draw_wallpaper(struct pipe_context *pipe)
 
         struct mali_attr_meta varying_meta[1] = {
                 {
-                        .type = MALI_ATYPE_FLOAT,
-                        .nr_components = MALI_POSITIVE(4),
-                        .not_normalised = 1,
-                        .unknown1 = /*0x2c22 - nr_comp=2*/ 0x2a22,
-                        .unknown2 = 0x1
+                        .format = MALI_RGBA32F,
+                        .swizzle = panfrost_get_default_swizzle(4),
+                        .unknown1 = 0x2,
                 }
         };
 
@@ -249,7 +265,14 @@  panfrost_draw_wallpaper(struct pipe_context *pipe)
         /* Emit the tiler job */
         struct panfrost_transfer tiler = panfrost_vertex_tiler_job(ctx, true, true);
         struct mali_job_descriptor_header *jd = (struct mali_job_descriptor_header *) tiler.cpu;
-        ctx->u_tiler_jobs[ctx->tiler_job_count] = jd;
+
+       /* Looks like setting first_tiler_job->dependency_index_2 to point to
+        * out 'reload fb content' job is not enough, the link order matters too.
+        * Let's insert our job in the first slot so that panfrost_link_jobs()
+        * place it before any other tiler jobs.
+        */
+        memmove(ctx->u_tiler_jobs + 1, ctx->u_tiler_jobs, ctx->tiler_job_count * sizeof(*ctx->u_tiler_jobs));
+        ctx->u_tiler_jobs[0] = jd;
         ctx->tiler_jobs[ctx->tiler_job_count++] = tiler.gpu;
         ctx->draw_count++;
 
@@ -262,7 +285,7 @@  panfrost_draw_wallpaper(struct pipe_context *pipe)
          */
 
         if (ctx->tiler_job_count > 1) {
-                ctx->u_tiler_jobs[0]->job_dependency_index_2 = jd->job_index;
+                ctx->u_tiler_jobs[1]->job_dependency_index_2 = jd->job_index;
         }
 
         printf("Wallpaper boop\n");

Comments

> +        else if (!(job->clear & PIPE_CLEAR_COLOR))

Make sure this is actually being called when you expect. I don't
remember if job->clear is being zeroed when we expect (hint: it might
not be due to a missing job_free routine somewhere, *blush*).

> +                       .rt[0].rgb_func = PIPE_BLEND_ADD,
> +                       .rt[0].rgb_src_factor = PIPE_BLENDFACTOR_ONE,
> +                       .rt[0].rgb_dst_factor = PIPE_BLENDFACTOR_ZERO,
> +                       .rt[0].alpha_func = PIPE_BLEND_ADD,
> +                       .rt[0].alpha_src_factor = PIPE_BLENDFACTOR_ZERO,
> +                       .rt[0].alpha_dst_factor = PIPE_BLENDFACTOR_ONE,

Why is alpha flipped? Is the black you're seeing really (0, 0, 0, a) or
might it be (0, 0, 0, 0)?

>          /* Bind texture/sampler. TODO: push/pop */

(Was this TODO addressed? It might explain the missing panel)

> +                0.0, ctx->pipe_framebuffer.height, 0.0, 1.0,
> +                0.0, 0.0, 0.0, 1.0,
> +                ctx->pipe_framebuffer.width, ctx->pipe_framebuffer.height, 0.0, 1.0,
> +                ctx->pipe_framebuffer.width, 0.0, 0.0, 1.0,

Just FWIW, this routine is running a fragment shader _without a vertex
shader_. In effect, we're running the vertex shader in software and
writing varyings straight to memory, as if the VS ran. So these values
are essentially the transformed output of a vertex shader.

> +                        .format = MALI_RGBA32F,
> +                        .swizzle = panfrost_get_default_swizzle(4),
> +                        .unknown1 = 0x2,

+1

> +       /* Looks like setting first_tiler_job->dependency_index_2 to point to
> +        * out 'reload fb content' job is not enough, the link order matters too.
> +        * Let's insert our job in the first slot so that panfrost_link_jobs()
> +        * place it before any other tiler jobs.

Oh, that's very interesting, I never realized that was the issue (I had
draw order issues). It's worth more investigation in the future, but +1
and here's a cookie :P

--------------------------------------

Hoping one of these is related, but the diff looks fairly solid....
On Fri, 3 May 2019 07:29:22 -0700
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:

> > +        else if (!(job->clear & PIPE_CLEAR_COLOR))  
> 
> Make sure this is actually being called when you expect. I don't
> remember if job->clear is being zeroed when we expect (hint: it might
> not be due to a missing job_free routine somewhere, *blush*).
> 
> > +                       .rt[0].rgb_func = PIPE_BLEND_ADD,
> > +                       .rt[0].rgb_src_factor = PIPE_BLENDFACTOR_ONE,
> > +                       .rt[0].rgb_dst_factor = PIPE_BLENDFACTOR_ZERO,
> > +                       .rt[0].alpha_func = PIPE_BLEND_ADD,
> > +                       .rt[0].alpha_src_factor = PIPE_BLENDFACTOR_ZERO,
> > +                       .rt[0].alpha_dst_factor = PIPE_BLENDFACTOR_ONE,  
> 
> Why is alpha flipped?

Actually, I tried several things, so I might have left it flipped at
some point, but it doesn't work with with src_factor=one and
dts_factor=zero.

> Is the black you're seeing really (0, 0, 0, a) or
> might it be (0, 0, 0, 0)?

Good question. I haven't dumped the buffers yet. Another thing to note:
the texture I'm reloading from is using PIPE_FORMAT_B8G8R8X8_UNORM as
a format, so no alpha component in there. I don't know exactly what
happens in this case (do we have garbage in the alpha component?) :-/.


> 
> >          /* Bind texture/sampler. TODO: push/pop */  
> 
> (Was this TODO addressed? It might explain the missing panel)

We definitely bind the new sampler/texture, but I'm not sure we restore
the old one. This being said, the version I have made using
util_blitter_blit() was taking care of saving/restoring those, and I
still had the issue :-/.

> 
> > +                0.0, ctx->pipe_framebuffer.height, 0.0, 1.0,
> > +                0.0, 0.0, 0.0, 1.0,
> > +                ctx->pipe_framebuffer.width, ctx->pipe_framebuffer.height, 0.0, 1.0,
> > +                ctx->pipe_framebuffer.width, 0.0, 0.0, 1.0,  
> 
> Just FWIW, this routine is running a fragment shader _without a vertex
> shader_. In effect, we're running the vertex shader in software and
> writing varyings straight to memory, as if the VS ran. So these values
> are essentially the transformed output of a vertex shader.

You mean we should apply the viewport transform on top, right? But
again, the blitter-based implementation had a full VS -> FS pipeline
with viewport transform applied and it didn't work either.

> 
> > +                        .format = MALI_RGBA32F,
> > +                        .swizzle = panfrost_get_default_swizzle(4),
> > +                        .unknown1 = 0x2,  
> 
> +1
> 
> > +       /* Looks like setting first_tiler_job->dependency_index_2 to point to
> > +        * out 'reload fb content' job is not enough, the link order matters too.
> > +        * Let's insert our job in the first slot so that panfrost_link_jobs()
> > +        * place it before any other tiler jobs.  
> 
> Oh, that's very interesting, I never realized that was the issue (I had
> draw order issues). It's worth more investigation in the future, but +1
> and here's a cookie :P

Hehe, glad to hear that at least part of this investigation lead to
something useful :-).

> 
> --------------------------------------
> 
> Hoping one of these is related, but the diff looks fairly solid....
> Actually, I tried several things, so I might have left it flipped at
> some point, but it doesn't work with with src_factor=one and
> dts_factor=zero.

Hum.

> Good question. I haven't dumped the buffers yet. Another thing to note:
> the texture I'm reloading from is using PIPE_FORMAT_B8G8R8X8_UNORM as
> a format, so no alpha component in there. I don't know exactly what
> happens in this case (do we have garbage in the alpha component?) :-/.

That just means the alpha component is implicitly cleared to 1.0 and
never really supposed to be read/written beyond that.

> We definitely bind the new sampler/texture, but I'm not sure we restore
> the old one. This being said, the version I have made using
> util_blitter_blit() was taking care of saving/restoring those, and I
> still had the issue :-/.

I meant the restore bit, but :/

> You mean we should apply the viewport transform on top, right?

No, this is ok, I just wanted to give context on what this is. The
input varying from OpenGL side (with a blitter implementation, for
instance), would be in the range (-1.0, +1.0), but then that gets
viewport transformed. With the default (0, 0)->(w, h) transform, it's
this. I guess for modified viewports this is a little, so we need to fix that
as well, but I don't think that's the issue here, since then blitter
would've been ok.

> Hehe, glad to hear that at least part of this investigation lead to
> something useful :-).

Haha :)