On Mon, 16 Sep 2019 09:59:15 -0400 Alyssa Rosenzweig <aly...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev