[Mesa-dev] [PATCH v3 21/25] panfrost: Add new helpers to describe job depencencies on BOs

[Boris Brezillon]

On Thu, 5 Sep 2019 19:26:45 -0400
Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:

> > --- a/src/gallium/drivers/panfrost/pan_fragment.c
> > +++ b/src/gallium/drivers/panfrost/pan_fragment.c
> > @@ -44,7 +44,7 @@ panfrost_initialize_surface(
> >          rsrc->slices[level].initialized = true;
> >  
> >          assert(rsrc->bo);
> > -        panfrost_batch_add_bo(batch, rsrc->bo);
> > +        panfrost_batch_add_bo(batch, rsrc->bo, PAN_SHARED_BO_RW);
> >  }  
> 
> This should be write-only. The corresponding read would be iff we're
> wallpapering, so add an add_bo with RO in the wallpaper drawing
> routine.

Actually we can't do that in the wallpaper draw, it's too late (the
wallpaper draw happens at flush time, and adding the BO when we're
already flushing the batch is pointless). 

> 
> I don't know if it really matters (since we can only have one write
> at a time) but let's be precise.

That's true, marking the BO for read access is useless when it's
already flagged for write since a write will anyway force batches that
want to read or write this BO to flush. If we really want to be precise
(for debug purpose I guess), we should probably have:

   panfrost_batch_add_bo(batch, rsrc->bo, PAN_SHARED_BO_WR);
   if (!batch->clear)
      panfrost_batch_add_bo(batch, rsrc->bo, PAN_SHARED_BO_RD);

> 
> -------------------------------
> 
> On that note, sometimes we stuff multiple related-but-independent
> buffers within a single BO, particularly multiple miplevels/cubemap
> faces/etc in one BO.  Hypothetically, it is legal to render to
> multiple faces independently at once. In practice, I don't know if
> this case is it is, we can of course split up the resource into
> per-face BOs.

I guess we'd have to introduce the concept of BO regions and only
force a flush when things overlap, assuming we want to keep those
independent buffers stored in the same BO of course.

> 
> >          _mesa_hash_table_remove_key(ctx->batches, &batch->key);
> > +        util_unreference_framebuffer_state(&batch->key);  
> 
> (Remind me where was the corresponding reference..?)

Duh, should be moved to patch 11 ("panfrost: Use a
pipe_framebuffer_state as the batch key").

> 
> > +void panfrost_batch_add_fbo_bos(struct panfrost_batch *batch)
> > +{
> > +        for (unsigned i = 0; i < batch->key.nr_cbufs; ++i) {
> > +                struct panfrost_resource *rsrc =
> > pan_resource(batch->key.cbufs[i]->texture);
> > +                panfrost_batch_add_bo(batch, rsrc->bo,
> > PAN_SHARED_BO_RW);
> > +	}
> > +
> > +        if (batch->key.zsbuf) {
> > +                struct panfrost_resource *rsrc =
> > pan_resource(batch->key.zsbuf->texture);
> > +                panfrost_batch_add_bo(batch, rsrc->bo,
> > PAN_SHARED_BO_RW);
> > +        }
> > +}  
> 
> As per above, these should be write-only. Also, is this duplicate from
> the panfrost_batch_add_bo in panfrost_initialize_surface? It feels
> like it. Which one is deadcode..?

We only draw the wallpaper on cbufs[0] right now, so I guess we can use
BO_WR here.