On Tue, Feb 23, 2021 at 12:46:20PM +0100, Daniel Vetter wrote:
> On Tue, Feb 23, 2021 at 12:19:56PM +0100, Greg KH wrote:
> > On Tue, Feb 23, 2021 at 11:58:42AM +0100, Thomas Zimmermann wrote:
> > > USB devices cannot perform DMA and hence have no dma_mask set in their
> > > device structure. Importing dmabuf into a USB-based driver fails, which
> > > break joining and mirroring of display in X11.
> > >
> > > For USB devices, pick the associated USB controller as attachment device,
> > > so that it can perform DMA. If the DMa controller does not support DMA
> > > transfers, we're aout of luck and cannot import.
> > >
> > > 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.
> > >
> > > 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: Johan Hovold <johan@xxxxxxxxxx>
> > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > > Cc: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > > Cc: Oliver Neukum <oneukum@xxxxxxxx>
> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx> # v5.10+
> > > ---
> > > drivers/gpu/drm/drm_prime.c | 36 ++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/tiny/gm12u320.c | 2 +-
> > > drivers/gpu/drm/udl/udl_drv.c | 2 +-
> > > include/drm/drm_gem_shmem_helper.h | 13 +++++++++++
> > > include/drm/drm_prime.h | 5 +++++
> > > 5 files changed, 56 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > index 2a54f86856af..9015850f2160 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -29,6 +29,7 @@
> > > #include <linux/export.h>
> > > #include <linux/dma-buf.h>
> > > #include <linux/rbtree.h>
> > > +#include <linux/usb.h>
> > >
> > > #include <drm/drm.h>
> > > #include <drm/drm_drv.h>
> > > @@ -1055,3 +1056,38 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
> > > dma_buf_put(dma_buf);
> > > }
> > > EXPORT_SYMBOL(drm_prime_gem_destroy);
> > > +
> > > +/**
> > > + * drm_gem_prime_import_usb - helper library implementation of the import callback for USB devices
> > > + * @dev: drm_device to import into
> > > + * @dma_buf: dma-buf object to import
> > > + *
> > > + * This is an implementation of drm_gem_prime_import() for USB-based devices.
> > > + * USB devices cannot perform DMA directly. This function selects the USB host
> > > + * controller as DMA device instead. Drivers can use this as their
> > > + * &drm_driver.gem_prime_import implementation.
> > > + *
> > > + * See also drm_gem_prime_import().
> > > + */
> > > +#ifdef CONFIG_USB
> > > +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
> > > + struct dma_buf *dma_buf)
> > > +{
> > > + struct usb_device *udev;
> > > + struct device *usbhost;
> > > +
> > > + if (dev->dev->bus != &usb_bus_type)
> > > + return ERR_PTR(-ENODEV);
> > > +
> > > + udev = interface_to_usbdev(to_usb_interface(dev->dev));
> > > + if (!udev->bus)
> > > + return ERR_PTR(-ENODEV);
> > > +
> > > + usbhost = udev->bus->controller;
> > > + if (!usbhost || !usbhost->dma_mask)
> > > + return ERR_PTR(-ENODEV);
> >
> > If individual USB drivers need access to this type of thing, shouldn't
> > that be done in the USB core itself?
> >
> > {hint, yes}
> >
> > There shouldn't be anything "special" about a DRM driver that needs this
> > vs. any other driver that might want to know about DMA things related to
> > a specific USB device. Why isn't this an issue with the existing
> > storage or v4l USB devices?
>
> The trouble is that this is a regression fix for 5.9, because the dma-api
> got more opinionated about what it allows. The proper fix is a lot more
> invasive (we essentially need to rework the drm_prime.c to allow dma-buf
> importing for just cpu access), and that's a ton more invasive than just a
> small patch with can stuff into stable kernels.
>
> This here is ugly, but it should at least get rid of black screens again.
>
> I think solid FIXME comment explaining the situation would be good.
Why can't I take a USB patch for a regression fix? Is drm somehow
stand-alone that you make changes here that should belong in other
subsystems?
{hint, it shouldn't be}
When you start poking in the internals of usb controller structures,
that logic belongs in the USB core for all drivers to use, not in a
random tiny subsystem where no USB developer will ever notice it? :)
thanks,
greg k-h
