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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4495 bytes --]

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@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;

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]