On Thu, Feb 25, 2021 at 09:23:05AM +0100, Takashi Iwai wrote:
> 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@xxxxxxx>
> > >> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > >> Cc: Christoph Hellwig <hch@xxxxxx>
> > >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > >> Cc: <stable@xxxxxxxxxxxxxxx> # 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.
Apart from the USB core and host/gadget controller drivers, there
appears to be only one reference to sysdev for a USB device: the one in
usb-storage (and that one really should be dmadev).
In general, I expect callers of the new routine would be drivers that
bind to a USB interface (like usb-storage), not to a USB device. So
they would naturally have the interface pointer handy.
But the routine could be written in a different way. If it took a
pointer to struct device as its argument, it could easily tell whether
that structure was embedded in a usb_device or a usb_interface
struct, and do the right thing either way.
Or there could be two routines: one taking a usb_device pointer and one
taking a usb_interface pointer.
The idea here is to make the routine as easy as possible for callers.
If this means making the routine a little longer, that's okay -- there's
only one copy of the routine but there could be lots of callers.
Alan Stern
