> > 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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
