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