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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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_ */





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux