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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi,

thanks for reviewing. This will be fixed in the next iteration.

Best regards
Thomas

Am 03.03.21 um 11:58 schrieb Noralf Trønnes:
> 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@xxxxxxx>
> > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > Tested-by: Pavel Machek <pavel@xxxxxx>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Acked-by: Christian König <christian.koenig@xxxxxxx>
> > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v5.10+
> > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
> >   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;

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel