[Mesa-dev] [PATCH v3 09/25] panfrost: Rework the panfrost_bo API

[Alyssa Rosenzweig]

> > I notice this had a print to stderr before with an assertion out, but
> > now it fails silently. Is this change of behaviour intentional? 
> 
> It is.

Alright! :-)

> > BO
> > creation would previously return a valid BO gauranteed. This is no
> > longer so obviously true -- although I see we later assert that the
> > return is non-NULL in the caller.
> > 
> > Could you help me understand the new logic a bit? Thank you!
> 
> The rationale behind this change being that panfrost_bo_alloc() will
> not be our last option (see patch 9). I can add the fprintf() back in
> this patch, and move it to the caller in patch 9 if you prefer.

Ah, that makes sense; thank you for clarifying!

> > > +        if (!(flags & (PAN_ALLOCATE_INVISIBLE | PAN_ALLOCATE_DELAY_MMAP)))
> > > +                panfrost_bo_mmap(bo);
> > > +	else if ((flags & PAN_ALLOCATE_INVISIBLE) && (pan_debug & PAN_DBG_TRACE))  
> > 
> > I think the spacing got wacky here (on the beginning of the last line)
> >
> 
> Will fix that.

+1

> > I see we now have the distinction between panfrost_bo_release (cached)
> > and panfrost_bo_free (uncached). I'm worried the distinction might not
> > be obvious to future Panfrost hackers.
> > 
> > Could you add a comment above each function clarifying the cache
> > behaviour?
> 
> Looks like the _release() function can be inlined in
> panfrost_bo_unreference(). I'm still not happy with the
> panfrost_bo_create() name though. Maybe we should rename this one into
> panfrost_get_bo().

I think splitting free/release to separate functions is good; I don't
know that inlining _release() is inherently needed. I'm just wondering
if we want a comment to make the distinction clear for future denizens
trying to figure out which routine to use --- although inlining one
would certainly solve that part...

> Yes, I guess I got tired splitting things up and decided to group
> changes that were kind of related in a single patch (also don't like
> having 30+ patch series). I'll split that up in v4.

No need to split it for v4; just a general note for future series :)