[RFC PATCH 2/2] vc4: introduce DMA-BUF heap

Thu Nov 9 12:14:29 UTC 2023

Hi Simon,

Thanks for working on this feature!

On 11/9/23 04:45, Simon Ser wrote:
> User-space sometimes needs to allocate scanout-capable memory for
> GPU rendering purposes. On a vc4/v3d split render/display SoC, this
> is achieved via DRM dumb buffers: the v3d user-space driver opens
> the primary vc4 node, allocates a DRM dumb buffer there, exports it
> as a DMA-BUF, imports it into the v3d render node, and renders to it.
> 
> However, DRM dumb buffers are only meant for CPU rendering, they are
> not intended to be used for GPU rendering. Primary nodes should only
> be used for mode-setting purposes, other programs should not attempt
> to open it. Moreover, opening the primary node is already broken on
> some setups: systemd grants permission to open primary nodes to
> physically logged in users, but this breaks when the user is not
> physically logged in (e.g. headless setup) and when the distribution
> is using a different init (e.g. Alpine Linux uses openrc).
> 
> We need an alternate way for v3d to allocate scanout-capable memory.

For me, it is a bit unclear how we import the vc4 DMA-BUF heap into v3d.
Should we create an IOCTL for it on v3d?

Also, if you need some help testing with the RPi 5, you can ping on IRC
and I can try to help by testing.

Best Regards,
- Maíra

> Leverage DMA heaps for this purpose: expose a CMA heap to user-space.
> Preliminary testing has been done with wlroots [1].
> 
> This is still an RFC. Open questions:
> 
> - Does this approach make sense to y'all in general?
> - What would be a good name for the heap? "vc4" is maybe a bit naive and
>    not precise enough. Something with "cma"? Do we need to plan a naming
>    scheme to accomodate for multiple vc4 devices?
> - Right now root privileges are necessary to open the heap. Should we
>    allow everybody to open the heap by default (after all, user-space can
>    already allocate arbitrary amounts of GPU memory)? Should we leave it
>    up to user-space to solve this issue (via logind/seatd or a Wayland
>    protocol or something else)?
> 
> TODO:
> 
> - Need to add !vc5 support.
> - Need to the extend DMA heaps API to allow vc4 to unregister the heap
>    on unload.
> 
> [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4414
> 
> Signed-off-by: Simon Ser <contact at emersion.fr>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Daniel Stone <daniel at fooishbar.org>
> Cc: Erico Nunes <nunes.erico at gmail.com>
> Cc: Iago Toral Quiroga <itoral at igalia.com>
> Cc: Maíra Canal <mcanal at igalia.com>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>   drivers/gpu/drm/vc4/Makefile       |  2 ++
>   drivers/gpu/drm/vc4/vc4_dma_heap.c | 51 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/vc4/vc4_drv.c      |  6 ++++
>   drivers/gpu/drm/vc4/vc4_drv.h      |  5 +++
>   4 files changed, 64 insertions(+)
>   create mode 100644 drivers/gpu/drm/vc4/vc4_dma_heap.c
> 
> diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile
> index c41f89a15a55..e4048870cec7 100644
> --- a/drivers/gpu/drm/vc4/Makefile
> +++ b/drivers/gpu/drm/vc4/Makefile
> @@ -34,4 +34,6 @@ vc4-$(CONFIG_DRM_VC4_KUNIT_TEST) += \
>   
>   vc4-$(CONFIG_DEBUG_FS) += vc4_debugfs.o
>   
> +vc4-$(CONFIG_DMABUF_HEAPS) += vc4_dma_heap.o
> +
>   obj-$(CONFIG_DRM_VC4)  += vc4.o
> diff --git a/drivers/gpu/drm/vc4/vc4_dma_heap.c b/drivers/gpu/drm/vc4/vc4_dma_heap.c
> new file mode 100644
> index 000000000000..010d0a88f3fa
> --- /dev/null
> +++ b/drivers/gpu/drm/vc4/vc4_dma_heap.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Copyright © 2023 Simon Ser
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
> +
> +#include "vc4_drv.h"
> +
> +static struct dma_buf *vc4_dma_heap_allocate(struct dma_heap *heap,
> +					     unsigned long size,
> +					     unsigned long fd_flags,
> +					     unsigned long heap_flags)
> +{
> +	struct vc4_dev *vc4 = dma_heap_get_drvdata(heap);
> +	//DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +	struct drm_gem_dma_object *dma_obj;
> +	struct dma_buf *dmabuf;
> +
> +	if (WARN_ON_ONCE(!vc4->is_vc5))
> +		return ERR_PTR(-ENODEV);
> +
> +	dma_obj = drm_gem_dma_create(&vc4->base, size);
> +	if (IS_ERR(dma_obj))
> +		return ERR_CAST(dma_obj);
> +
> +	dmabuf = drm_gem_prime_export(&dma_obj->base, fd_flags);
> +	drm_gem_object_put(&dma_obj->base);
> +	return dmabuf;
> +}
> +
> +static const struct dma_heap_ops vc4_dma_heap_ops = {
> +	.allocate = vc4_dma_heap_allocate,
> +};
> +
> +int vc4_dma_heap_create(struct vc4_dev *vc4)
> +{
> +	struct dma_heap_export_info exp_info;
> +	struct dma_heap *heap;
> +
> +	exp_info.name = "vc4"; /* TODO: allow multiple? */
> +	exp_info.ops = &vc4_dma_heap_ops;
> +	exp_info.priv = vc4; /* TODO: unregister when unloading */
> +
> +	heap = dma_heap_add(&exp_info);
> +	if (IS_ERR(heap))
> +		return PTR_ERR(heap);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index c133e96b8aca..c7297dd7d9d5 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -391,6 +391,12 @@ static int vc4_drm_bind(struct device *dev)
>   
>   	drm_fbdev_dma_setup(drm, 16);
>   
> +#if IS_ENABLED(CONFIG_DMABUF_HEAPS)
> +	ret = vc4_dma_heap_create(vc4);
> +	if (ret)
> +		goto err;
> +#endif
> +
>   	return 0;
>   
>   err:
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index ab61e96e7e14..d5c5dd18815c 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -1078,4 +1078,9 @@ int vc4_perfmon_destroy_ioctl(struct drm_device *dev, void *data,
>   int vc4_perfmon_get_values_ioctl(struct drm_device *dev, void *data,
>   				 struct drm_file *file_priv);
>   
> +/* vc4_dma_heap.c */
> +#if IS_ENABLED(CONFIG_DMABUF_HEAPS)
> +int vc4_dma_heap_create(struct vc4_dev *vc4);
> +#endif
> +
>   #endif /* _VC4_DRV_H_ */