[Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful

[Boris Brezillon]

On Thu, 8 Aug 2019 06:08:05 -0700
Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com> wrote:

> @Boris I don't see why this patch particularly needs to be backported to
> 19.1?

Nope, no reason to backport it to 19.1. It's just a (bad?) kernel dev
habit to put "Fixes:" tag the patch fixes a problem introduced by a
previous commit. But it does not necessarily means "backport to stable"
in mind.

> 
> On Thu, Aug 08, 2019 at 11:46:43AM +0200, Juan A. Suarez Romero wrote:
> > On Fri, 2019-08-02 at 19:18 +0200, Boris Brezillon wrote:  
> > > ctx->job is supposed to serve as a cache to avoid an hash table lookup
> > > everytime we access the job attached to the currently bound FB, except
> > > it was never assigned to anything but NULL.
> > > 
> > > Fix that by adding the missing assignment in panfrost_get_job_for_fbo().
> > > Also add a missing NULL assignment in the ->set_framebuffer_state()
> > > path.
> > > 
> > > While at it, add extra assert()s to make sure ctx->job is consistent.
> > > 
> > > Fixes: 59c9623d0a75 ("panfrost: Import job data structures from v3d")
> > > Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> > > Changes in v2:
> > > * New patch (suggested by Alyssa)  
> > 
> > 
> > This patch is a candidate for 19.1, but unfortunately it does not apply cleanly.
> > It seems it depends on a bunch of other commits not in 19.1
> > 
> > At this point, I will ignore/reject this patch for 19.1, unless you can provide
> > me a specific version for 19.1 branch (use staging/19.1 as reference).
> > 
> > Thanks.
> > 
> > 	J.A.
> >   
> > > ---
> > >  src/gallium/drivers/panfrost/pan_context.c |  5 +++++
> > >  src/gallium/drivers/panfrost/pan_job.c     | 19 ++++++++++++++++++-
> > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> > > index 014f8f6a9d07..ba2df2ce66ae 100644
> > > --- a/src/gallium/drivers/panfrost/pan_context.c
> > > +++ b/src/gallium/drivers/panfrost/pan_context.c
> > > @@ -2372,6 +2372,11 @@ panfrost_set_framebuffer_state(struct pipe_context *pctx,
> > >                  panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
> > >          }
> > >  
> > > +        /* Invalidate the FBO job cache since we've just been assigned a new
> > > +         * FB state.
> > > +         */
> > > +        ctx->job = NULL;
> > > +
> > >          util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
> > >  
> > >          /* Given that we're rendering, we'd love to have compression */
> > > diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> > > index 6339b39d29a0..cc81d475165e 100644
> > > --- a/src/gallium/drivers/panfrost/pan_job.c
> > > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > > @@ -23,6 +23,8 @@
> > >   *
> > >   */
> > >  
> > > +#include <assert.h>
> > > +
> > >  #include "pan_context.h"
> > >  #include "util/hash_table.h"
> > >  #include "util/ralloc.h"
> > > @@ -124,8 +126,13 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
> > >  
> > >          /* If we already began rendering, use that */
> > >  
> > > -        if (ctx->job)
> > > +        if (ctx->job) {
> > > +                assert(ctx->job->key.zsbuf == ctx->pipe_framebuffer.zsbuf &&
> > > +                       !memcmp(ctx->job->key.cbufs,
> > > +                               ctx->pipe_framebuffer.cbufs,
> > > +                               sizeof(ctx->job->key.cbufs)));
> > >                  return ctx->job;
> > > +        }
> > >  
> > >          /* If not, look up the job */
> > >  
> > > @@ -133,6 +140,10 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
> > >          struct pipe_surface *zsbuf = ctx->pipe_framebuffer.zsbuf;
> > >          struct panfrost_job *job = panfrost_get_job(ctx, cbufs, zsbuf);
> > >  
> > > +        /* Set this job as the current FBO job. Will be reset when updating the
> > > +         * FB state and when submitting or releasing a job.
> > > +         */
> > > +        ctx->job = job;
> > >          return job;
> > >  }
> > >  
> > > @@ -181,6 +192,12 @@ panfrost_job_submit(struct panfrost_context *ctx, struct panfrost_job *job)
> > >  
> > >          if (ret)
> > >                  fprintf(stderr, "panfrost_job_submit failed: %d\n", ret);
> > > +
> > > +        /* The job has been submitted, let's invalidate the current FBO job
> > > +         * cache.
> > > +	 */
> > > +        assert(!ctx->job || job == ctx->job);
> > > +        ctx->job = NULL;
> > >  }
> > >  
> > >  void  
> >