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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: airlied@linux.ie, gregkh@linuxfoundation.org,
	Christoph Hellwig <hch@lst.de>,
	hdegoede@redhat.com, dri-devel@lists.freedesktop.org,
	stable@vger.kernel.org, sean@poorly.run,
	christian.koenig@amd.com
Subject: Re: [PATCH v4] drm: Use USB controller's DMA mask when importing dmabufs
Date: Thu, 25 Feb 2021 08:57:14 +0100	[thread overview]
Message-ID: <b44307cf-25f9-acd0-eb35-92e8716205de@suse.de> (raw)
In-Reply-To: <20210224152153.GA1307460@rowland.harvard.edu>

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

Hi

Am 24.02.21 um 16:21 schrieb Alan Stern:
> On Wed, Feb 24, 2021 at 10:23:04AM +0100, Thomas Zimmermann wrote:
>> 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.
>>
>> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
>>
>> 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")
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: <stable@vger.kernel.org> # v5.10+
>> ---
> 
>> +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
>> +						struct dma_buf *dma_buf)
>> +{
>> +	struct usb_device *udev;
>> +	struct device *dmadev;
>> +	struct drm_gem_object *obj;
>> +
>> +	if (!dev_is_usb(dev->dev))
>> +		return ERR_PTR(-ENODEV);
>> +	udev = interface_to_usbdev(to_usb_interface(dev->dev));
>> +
>> +	dmadev = usb_get_dma_device(udev);
> 
> You can do it this way if you want, but I think usb_get_dma_device would
> be easier to use if its argument was a pointer to struct usb_interface
> or (even better) a pointer to a usb_interface's embedded struct device.
> Then you wouldn't need to compute udev, and the same would be true for
> other callers.

It seemed natural to me to use usb_device, because it contains the bus 
pointer. But maybe a little wrapper for usb_interface in the header file 
makes things easier to read. I'll wait a bit for other reviews to come in.

Best regards
Thomas

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

-- 
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 --]