Re: [PATCH v7] drm: Use USB controller's DMA mask when importing dmabufs

["Noralf Trønnes" <noralf@tronnes.org>]

Den 03.03.2021 09.45, skrev Thomas Zimmermann:
> USB devices cannot perform DMA and hence have no dma_mask set in their
> device structure. Therefore importing dmabuf into a USB-based driver
> fails, which breaks joining and mirroring of display in X11.
> 
> For USB devices, pick the associated USB controller as attachment device.
> This allows the DRM import helpers to perform the DMA setup. If the DMA
> controller does not support DMA transfers, we're out of luck and cannot
> import. Our current USB-based DRM drivers don't use DMA, so the actual
> DMA device is not important.
> 
> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their
> instance of struct drm_driver.
> 

This doesn't seem to be the case anymore.

> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.

s/un der/under/

> 
> v7:
> 	* fix use-before-init bug in gm12u320 (Dan)
> v6:
> 	* implement workaround in DRM drivers and hold reference to
> 	  DMA device while USB device is in use
> 	* remove dev_is_usb() (Greg)
> 	* collapse USB helper into usb_intf_get_dma_device() (Alan)
> 	* integrate Daniel's TODO statement (Daniel)
> 	* fix typos (Greg)
> v5:
> 	* provide a helper for USB interfaces (Alan)
> 	* add FIXME item to documentation and TODO list (Daniel)
> v4:
> 	* implement workaround with USB helper functions (Greg)
> 	* use struct usb_device->bus->sysdev as DMA device (Takashi)
> v3:
> 	* drop gem_create_object
> 	* use DMA mask of USB controller, if any (Daniel, Christian, Noralf)
> v2:
> 	* move fix to importer side (Christian, Daniel)
> 	* update SHMEM and CMA helpers for new PRIME callbacks
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Christian König <christian.koenig@amd.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: <stable@vger.kernel.org> # v5.10+
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  Documentation/gpu/todo.rst      | 21 +++++++++++++++++++++
>  drivers/gpu/drm/tiny/gm12u320.c | 28 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/udl/udl_drv.c   | 17 +++++++++++++++++
>  drivers/gpu/drm/udl/udl_drv.h   |  1 +
>  drivers/gpu/drm/udl/udl_main.c  |  9 +++++++++
>  drivers/usb/core/usb.c          | 32 ++++++++++++++++++++++++++++++++
>  include/linux/usb.h             |  2 ++
>  7 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 0b4f4f2af1ef..4fe372f43cf5 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c

[...]

> @@ -638,12 +656,15 @@ static int gm12u320_usb_probe(struct usb_interface *interface,
>  				      struct gm12u320_device, dev);
>  	if (IS_ERR(gm12u320))
>  		return PTR_ERR(gm12u320);
> +	dev = &gm12u320->dev;
> +
> +	gm12u320->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev));
> +	if (!gm12u320->dmadev)
> +		drm_warn(dev, "buffer sharing not supported"); /* not an error */
>  

When implementing this in my own driver I discovered that this device
ref will leak if probing fails after this.

I've done it like this:

	gdrm->dmadev = usb_intf_get_dma_device(intf);
	if (!gdrm->dmadev)
		dev_warn(dev, "buffer sharing not supported");

	ret = drm_dev_register(drm, 0);
	if (ret) {
		put_device(gdrm->dmadev);
		return ret;
	}

An even better solution would be to have a devm_ version of the function.

Noralf.

>  	INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work);
>  	mutex_init(&gm12u320->fb_update.lock);
>  
> -	dev = &gm12u320->dev;
> -
>  	ret = drmm_mode_config_init(dev);
>  	if (ret)
>  		return ret;