[Mesa-dev] [PATCH 1/2] panfrost: Track BO lifetime with jobs and reference counts

[Boris Brezillon]

On Tue, 16 Apr 2019 09:31:46 +0200
Boris Brezillon <boris.brezillon at collabora.com> wrote:

> On Tue, 16 Apr 2019 01:49:21 +0000
> Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
> 
> > @@ -1793,22 +1799,9 @@ panfrost_set_vertex_buffers(
> >          const struct pipe_vertex_buffer *buffers)
> >  {
> >          struct panfrost_context *ctx = pan_context(pctx);
> > -        assert(num_buffers <= PIPE_MAX_ATTRIBS);
> > -
> > -        /* XXX: Dirty tracking? etc */
> > -        if (buffers) {
> > -                size_t sz = sizeof(buffers[0]) * num_buffers;
> > -                ctx->vertex_buffers = malloc(sz);
> > -                ctx->vertex_buffer_count = num_buffers;
> > -                memcpy(ctx->vertex_buffers, buffers, sz);
> > -        } else {
> > -                if (ctx->vertex_buffers) {
> > -                        free(ctx->vertex_buffers);
> > -                        ctx->vertex_buffers = NULL;
> > -                }
> >  
> > -                ctx->vertex_buffer_count = 0;
> > -        }
> > +        util_set_vertex_buffers_mask(ctx->vertex_buffers, &ctx->vb_mask, buffers, start_slot, num_buffers);

Had a look at the cso code (more precisely
cso_{save,restore}_vertex_buffer0() and their users) and I'm not sure
util_set_vertex_buffers_mask() does what's expected by the CSO layer
(don't know who's right about the ->set_vertex_buffers() semantic
though).

My understanding is that CSO just saves/restores buffer in slot 0 and
expects other slots to be unchanged, but util_set_vertex_buffers_mask()
marks all buffers outside of the 'start_slot -> start_slot+num_buffers'
range as inactive.

So, we should either make sure ->set_vertex_buffers() only modifies the
status of the bufs in the 'start_slot -> start_slot+num_buffers' range
(and force callers of ->set_vertex_buffers() to explicitly invalidate
bufs if they need to keep only this specific range active) or patch the
cso code to save/restore all active buffer slots.

> > +        ctx->vertex_buffer_count = num_buffers;  
> 
> ->vertex_buffer_count should be set to fls(ctx->vb_mask) (fls == find  
> last bit set) if you want the

Looks like it's called util_last_bit() in mesa.

> 
> 	for (int i = 0; i < ctx->vertex_buffer_count; ++i)
> 
> loop in panfrost_emit_vertex_data() to do the right thing. But I think
> we can get rid of ->vertex_buffer_count entirely and just do a
> 
> 	for (int i = 0; i < ARRAY_SIZE(ctx->vertex_buffers); ++i)
> 
> >  }