[Mesa-dev] [PATCH v2 28/37] panfrost: Start tracking inter-batch dependencies

[Alyssa Rosenzweig]

> > I'm wondering if batch->dependencies should be expressed as a set,
> > rather than a dynarray, such that testing whether a batch has a
> > given dependency is ideally O(1), not O(N).
> > 
> > In practice I don't know if the asymptotic complexity matters, esp. for
> > small numbers of batches, and in practice hash table iteration is slow
> > enough in mesa* that maybe it would be counterproductive.
> > 
> > Just something I thought I might throw out there.
> > 
> > * Set iteration ought to be no slower than array iteration, but constant
> >   factors are a thing.
> 
> I thought the number of deps would be small enough to not justify the
> use of a set, but maybe I'm wrong.

Mm, after all this lands we can profile and revisit, there are bigger
fish to fry.

> > > +static void
> > > +panfrost_bo_access_gc_fences(struct panfrost_context *ctx,
> > > +                             struct panfrost_bo_access *access,
> > > +			     const struct panfrost_bo *bo)  
> > 
> > Could you remind me what gc abbreviates? Sorry.
> 
> Garbage collect.
> 
> > 
> > I'm a little unclear on what the function's purpose is based on the
> > name.
> 
> Collect signaled fences to keep the kernel-side syncobj-map small. The
> idea is to collect those signaled fences at the end of each flush_all
> call. This function is likely to collect only fences from previous
> batch flushes not the one that have just have just been submitted and
> are probably still in flight when we trigger the garbage collection.
> Anyway, we need to do this garbage collection at some point if we don't
> want the BO access map to keep invalid entries around and retain
> syncobjs forever.

This definitely needs to be documented somewhere, then. Maybe paste the
"Collect...forever" paragraph into a comment above access_gc_fences?

> > Question: is it safe to remove entries while iterating the table?
> > (Not a review comment, I don't know the details of mesa's
> > implementation)
> 
> According to the doc, it is.

Good to know, thank you.

> > > -        if (ctx->last_out_sync) {
> > > +        if (reqs & PANFROST_JD_REQ_FS && batch->first_job.gpu)  
> > 
> > Could we add a `bool is_fragment` with this condition and then make this
> > as well as the if below simple if (is_fragment)?
> 
> Hm, the condition is actually is_fragment_and_has_draws, so I'm not
> sure is_fragment is good name. Maybe depends_on_vertex_tiler.

is_fragment_shader?

> > > +        /* Submit the dependencies first. */
> > > +        util_dynarray_foreach(&batch->dependencies,
> > > +                              struct panfrost_batch_fence *, dep) {
> > > +                if ((*dep)->batch)
> > > +                        panfrost_batch_submit((*dep)->batch);
> > > +        }  
> > 
> > I assume this logic will be refined later in the series...?
> 
> It's not actually. Why do you think it should?

Oh, I was just assuming the dependency graph stuff would be more
explicit, I guess.