Hi Simon, thanks for looking into this! I few comments below inline... El jue, 09-11-2023 a las 07:45 +0000, Simon Ser escribió: > 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. > 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: I think Maxime is the right person to answer about vc4 but I'll answer a few of these questions from my perspective: > > - Does this approach make sense to y'all in general? Yeah, this sounds sensible to me. > - 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)? I think so, yes, after all, the main user of this will be the Mesa driver, so root only would not be great I think. > > TODO: > > - Need to add !vc5 support. I believe that would be models before Raspberry Pi 4, which don't have v3d, so maybe we don't need this there? Iago > - 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@xxxxxxxxxxx> > Cc: Maxime Ripard <mripard@xxxxxxxxxx> > Cc: Daniel Vetter <daniel@xxxxxxxx> > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx> > Cc: Erico Nunes <nunes.erico@xxxxxxxxx> > Cc: Iago Toral Quiroga <itoral@xxxxxxxxxx> > Cc: Maíra Canal <mcanal@xxxxxxxxxx> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> > --- > 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_ */
