On Thu, 5 Sep 2019 16:31:04 -0400
Alyssa Rosenzweig <aly...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
