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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 25 Feb 2021 08:57:14 +0100,
Thomas Zimmermann wrote:
> 
> 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.

I agree with Thomas, as most of users referring to the sysdev do
access in a pattern like udev->bus->sysdev, AFAIK.


thanks,

Takashi