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

[Alyssa Rosenzweig]

> +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? 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!

> +        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)

> +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?

---------------------------------------------

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!