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

[Boris Brezillon]

On Thu, 5 Sep 2019 16:31:04 -0400
Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:

> > +static struct panfrost_bo *
> > +panfrost_bo_alloc(struct panfrost_screen *screen, size_t size,
> > +                  uint32_t flags)
> > +{  
> ...
> > +        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_CREATE_BO, &create_bo);
> > +        if (ret)
> > +                return NULL;  
> 
> 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.

> 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.

> > +        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.
 
> > +static void
> > +panfrost_bo_release(struct panfrost_bo *bo)
> > +{
> > +
> > +        /* Rather than freeing the BO now, we'll cache the BO for later
> > +         * allocations if we're allowed to */
> > +
> > +        panfrost_bo_munmap(bo);
> > +
> > +        if (panfrost_bo_cache_put(bo))
> > +                return;
> > +
> > +        panfrost_bo_free(bo);
> > +}  
> 
> 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().

> 
> ---------------------------------------------
> 
> Other than these, the cleanup in general seems like a good idea. But in
> general, please try to split up patches like this to aid reviewin. Thank
> you!

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.

Thanks for the review!

Boris