[Mesa-dev] [PATCH] panfrost: Take into account off-screen FBOs

[Erik Faye-Lund]

On Thu, 2019-07-04 at 10:02 +0200, Tomeu Vizoso wrote:
> In that case, ctx->pipe_framebuffer.cbufs[0] can be NULL.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> Cc: Boris Brezillon <boris.brezillon at collabora.com>
> Fixes: 5375d009be18 ("panfrost: Pass referenced BOs to the SUBMIT
> ioctls")

Nit: this isn't really about off-screen vs on-screen, but rendering
with vs without a color-buffer. Sure, non-color buffers can't be on-
screen, but you can have off-screen color-buffers as well.

I know this patch already landed, but if you keep this in mind for the
future, perhaps we can avoid confusing commit messages.

> ---
>  src/gallium/drivers/panfrost/pan_drm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_drm.c
> b/src/gallium/drivers/panfrost/pan_drm.c
> index 8de4f483435c..b89f8e66a877 100644
> --- a/src/gallium/drivers/panfrost/pan_drm.c
> +++ b/src/gallium/drivers/panfrost/pan_drm.c
> @@ -238,7 +238,6 @@ panfrost_drm_submit_job(struct panfrost_context
> *ctx, u64 job_desc, int reqs)
>  int
>  panfrost_drm_submit_vs_fs_job(struct panfrost_context *ctx, bool
> has_draws, bool is_scanout)
>  {
> -        struct pipe_surface *surf = ctx->pipe_framebuffer.cbufs[0];
>  	int ret;
>  
>          struct panfrost_job *job = panfrost_get_job_for_fbo(ctx);
> @@ -256,9 +255,12 @@ panfrost_drm_submit_vs_fs_job(struct
> panfrost_context *ctx, bool has_draws, bool
>          }
>  
>          if (job->first_tiler.gpu || job->clear) {
> -                struct panfrost_resource *res = pan_resource(surf-
> >texture);
> -                assert(res->bo);
> -                panfrost_job_add_bo(job, res->bo);
> +                struct pipe_surface *surf = ctx-
> >pipe_framebuffer.cbufs[0];
> +                if (surf) {
> +                        struct panfrost_resource *res =
> pan_resource(surf->texture);
> +                        assert(res->bo);
> +                        panfrost_job_add_bo(job, res->bo);
> +                }
>                  ret = panfrost_drm_submit_job(ctx,
> panfrost_fragment_job(ctx, has_draws), PANFROST_JD_REQ_FS);
>                  assert(!ret);
>          }

I'm not sure this is actually correct either. I don't think there's any
guarantee that the currently bound framebuffer is what was actually
last rendered to. For instance if an application renders to the
backbuffer, switches to some FBO, does some rendering, switches back
and swaps buffers right away. I believe the currently bound framebuffer
will be the FBO at this point, because framebuffer-state is bound
lazily.

I think instead you'd want to add a reference in
panfrost_set_framebuffer_state(), right before you start rendering to
it... And at this point, you shouldn't need to special-case cbuf 0, you
should just add a reference to all cbufs as well as the zsbuf.