[Mesa-dev] [PATCH v2 22/37] panfrost: Extend the panfrost_batch_add_bo() API to pass access flags

[Boris Brezillon]

On Mon, 16 Sep 2019 09:59:15 -0400
Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:

> PAN_BO_GPU_ACCESS_* is rather wordy. We're a GPU driver, of course it's
> GPU access :)

Well, the driver can also do CPU accesses to the same BOs :P.

> 
> Could we just do PAN_BO_ACCESS_* instead?

I guess that's fine as long as it's documented.

> 
> >  static mali_ptr
> >  panfrost_upload_tex(
> >          struct panfrost_context *ctx,
> > +        enum pipe_shader_type st,
> >          struct panfrost_sampler_view *view)
> >  {
> >          if (!view)
> > @@ -610,7 +611,11 @@ panfrost_upload_tex(
> >  
> >          /* Add the BO to the job so it's retained until the job is done. */
> >          struct panfrost_batch *batch = panfrost_get_batch_for_fbo(ctx);
> > -        panfrost_batch_add_bo(batch, rsrc->bo);
> > +        panfrost_batch_add_bo(batch, rsrc->bo,
> > +                              PAN_BO_GPU_ACCESS_SHARED | PAN_BO_GPU_ACCESS_READ |
> > +                              PAN_BO_GPU_ACCESS_VERTEX_TILER |
> > +                              (st == PIPE_SHADER_FRAGMENT ?
> > +                               PAN_BO_GPU_ACCESS_FRAGMENT : 0));  
> 
> I'm not sure this is quite right... should it maybe be:
> 
> (st == PIPE_SHADER_FRAGMENT ? PAN_BO_ACCESS_FRAGMENT :
> PAN_BO_ACCESS_VERTEX_TILER)

That's a good question. I wasn't sure so I decided to put the
vertex/tiler unconditionally.

> 
> I.e., if it's accessed from the fragment shader, is it necessarily
> needed for the vertex/tiler part?
> 
> > -        panfrost_batch_add_bo(batch, bo);
> > +        panfrost_batch_add_bo(batch, bo,
> > +                              PAN_BO_GPU_ACCESS_SHARED | PAN_BO_GPU_ACCESS_RW |
> > +                              PAN_BO_GPU_ACCESS_VERTEX_TILER |
> > +                              (st == PIPE_SHADER_FRAGMENT ?
> > +                               PAN_BO_GPU_ACCESS_FRAGMENT : 0));  
> 
> Ditto. We should maybe have a `pan_bo_access_for_stage(enum
> pipe_shader_type)` to abstract this logic.

Good idea.

> 
> > @@ -803,7 +813,12 @@ panfrost_map_constant_buffer_gpu(
> >          struct panfrost_batch *batch = panfrost_get_batch_for_fbo(ctx);
> >  
> >          if (rsrc) {
> > -                panfrost_batch_add_bo(batch, rsrc->bo);
> > +                panfrost_batch_add_bo(batch, rsrc->bo,
> > +                                      PAN_BO_GPU_ACCESS_SHARED |
> > +                                      PAN_BO_GPU_ACCESS_READ |
> > +                                      PAN_BO_GPU_ACCESS_VERTEX_TILER |
> > +                                      (st == PIPE_SHADER_FRAGMENT ?
> > +                                       PAN_BO_GPU_ACCESS_FRAGMENT : 0));  
> 
> Ditto.
> 
> >          if (!info->has_user_indices) {
> >                  /* Only resources can be directly mapped */
> > -                panfrost_batch_add_bo(batch, rsrc->bo);
> > +                panfrost_batch_add_bo(batch, rsrc->bo,
> > +                                      PAN_BO_GPU_ACCESS_FRAGMENT);
> >                  return rsrc->bo->gpu + offset;  
> 
> The index buffer is to determine geometry, so it is definitely accessed
> from the vertex/tiler chain.

Oops, that's a mistake. I meant PAN_BO_GPU_ACCESS_VERTEX_TILER here.

> 
> I'm not sure if it's also accessed by the fragment chain. Also, should
> this have ACCESS_SHARED | ACCESS_READ to be consistent with the others?

It should definitely have ACCESS_SHARED | ACCESS_READ.

> 
> > @@ -69,7 +69,10 @@ panfrost_emit_streamout(
> >          /* Grab the BO and bind it to the batch */
> >          struct panfrost_batch *batch = panfrost_get_batch_for_fbo(ctx);
> >          struct panfrost_bo *bo = pan_resource(target->buffer)->bo;
> > -        panfrost_batch_add_bo(batch, bo);
> > +        panfrost_batch_add_bo(batch, bo,
> > +                              PAN_BO_GPU_ACCESS_SHARED |
> > +                              PAN_BO_GPU_ACCESS_WRITE |
> > +                              PAN_BO_GPU_ACCESS_VERTEX_TILER);  
> 
> We operate somewhat like:
> 
> [ Vertices ] -- vertex shader --> [ Varyings ] -- tiler --> [ Geometry ]
> 
> So varyings are WRITE from the perspective of the VERTEX but READ from
> the perspective of the TILER and FRAGMENT.
> 
> Now, this is for streamout. However, streamout does not imply rasterize
> discard. Hence, it is legal to have streamout and also render that
> geometry with a FRAGMENT job. So it's premature to drop the READ and
> FRAGMENT flags (this will presumably regress a bunch of dEQP-GLES3 tests
> for streamout).

Okay, will add PAN_BO_GPU_ACCESS_FRAGMENT and turn
PAN_BO_GPU_ACCESS_WRITE into PAN_BO_GPU_ACCESS_RW.

Thanks for the review.

Boris