On Fri, 28 Feb 2025, Thomas Zimmermann <tzimmerm...@suse.de> wrote:
> Importing dma-bufs via PRIME requires a DMA-capable device. Devices on
> peripheral busses, such as USB, often cannot perform DMA by themselves.
> Without DMA-capable device PRIME import fails. DRM drivers for USB
> devices already use a separate DMA device for dma-buf imports. Make the
> mechanism generally available.
>
> Add the field dma_dev to struct drm_device to refer to the device's DMA
> device. For USB this should be the USB controller. Use dma_dev in the
> PRIME import helpers, if set.
>
> Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
> ---
> drivers/gpu/drm/drm_drv.c | 2 ++
> drivers/gpu/drm/drm_prime.c | 2 +-
> include/drm/drm_device.h | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 17fc5dc708f4..f8c3c9f77d22 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -654,6 +654,8 @@ static void drm_dev_init_release(struct drm_device *dev,
> void *res)
> {
> drm_fs_inode_free(dev->anon_inode);
>
> + put_device(dev->dma_dev);
> + dev->dma_dev = NULL;
The asymmetry in who gets and puts the ->dma_dev bugs me.
You could avoid that by providing a helper for setting ->dma_dev, and
having that get the device:
int drm_dev_set_dma_device(struct drm_device *drm, struct device *dma_device)
{
dev->dma_dev = get_device(dma_device);
return dev->dma_dev ? 0 : -ENODEV.
}
Taking drm/gm12u320 from patch 2 as an example, it would become:
dma_device = usb_intf_get_dma_device(to_usb_interface(dev->dev))
if (drm_dev_set_dma_device(drm, dma_device))
drm_warn(dev, "buffer sharing not supported"); /* not an error
*/
put_device(dma_device);
Sligthly more verbose, but has a get/put pair in the driver, and a
get/put pair in the core, instead of the asymmetry.
I'm not insisting, but something to consider.
BR,
Jani.
> put_device(dev->dev);
> /* Prevent use-after-free in drm_managed_release when debugging is
> * enabled. Slightly awkward, but can't really be helped. */
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 32a8781cfd67..258858f2f8dd 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -1004,7 +1004,7 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev);
> struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
> struct dma_buf *dma_buf)
> {
> - return drm_gem_prime_import_dev(dev, dma_buf, dev->dev);
> + return drm_gem_prime_import_dev(dev, dma_buf, drm_dev_dma_dev(dev));
> }
> EXPORT_SYMBOL(drm_gem_prime_import);
>
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 6ea54a578cda..a24cac4b2077 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -64,6 +64,23 @@ struct drm_device {
> /** @dev: Device structure of bus-device */
> struct device *dev;
>
> + /**
> + * @dma_dev:
> + *
> + * Device for DMA operations. Only required if the device @dev
> + * cannot perform DMA by itself. Should be NULL otherwise.
> + *
> + * Devices on USB and other peripheral busses cannot perform DMA
> + * by themselves. The @dma_dev field should point the bus controller
> + * that does DMA on behalve of such a device. Required for importing
> + * buffers via dma-buf.
> + *
> + * If set, the DRM driver has to acquire a reference on the DMA
> + * device, which will be owned and released automatically by the
> + * DRM core.
> + */
> + struct device *dma_dev;
> +
> /**
> * @managed:
> *
> @@ -327,4 +344,24 @@ struct drm_device {
> struct dentry *debugfs_root;
> };
>
> +/**
> + * drm_dev_dma_dev - returns the DMA device for a DRM device
> + * @dev: DRM device
> + *
> + * Returns the DMA device of the given DRM device. This is usually
> + * the same as the DRM device's parent. Devices on some peripheral
> + * busses, such as USB, are incapable of performing DMA by themselves.
> + * Drivers should set struct &drm_device.dma_dev to the bus controller
> + * to make DMA work. Required for PRIME buffer import.
> + *
> + * Returns:
> + * A DMA-capable device for the DRM device.
> + */
> +static inline struct device *drm_dev_dma_dev(struct drm_device *dev)
> +{
> + if (dev->dma_dev)
> + return dev->dma_dev;
> + return dev->dev;
> +}
> +
> #endif
--
Jani Nikula, Intel
