[v3] drm: Use USB controller's DMA mask when importing dmabufs

Commit Message

Thomas Zimmermann Feb. 23, 2021, 10:58 a.m. UTC

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 <[e-mail]>
Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
Cc: Christoph Hellwig <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Johan Hovold <[e-mail]>
Cc: Alan Stern <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Sebastian Andrzej Siewior <[e-mail]>
Cc: Mathias Nyman <[e-mail]>
Cc: Oliver Neukum <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: <[e-mail]> # 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(-)

--
2.30.1

Comments

Greg Kroah-Hartman Feb. 23, 2021, 11:19 a.m. UTC | #1

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 <[e-mail]>
> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> Cc: Christoph Hellwig <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Johan Hovold <[e-mail]>
> Cc: Alan Stern <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Sebastian Andrzej Siewior <[e-mail]>
> Cc: Mathias Nyman <[e-mail]>
> Cc: Oliver Neukum <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: <[e-mail]> # 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?

thanks,

greg k-h

Greg Kroah-Hartman Feb. 23, 2021, 11:27 a.m. UTC | #2

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 <[e-mail]>
> > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > Cc: Christoph Hellwig <[e-mail]>
> > Cc: Greg Kroah-Hartman <[e-mail]>
> > Cc: Johan Hovold <[e-mail]>
> > Cc: Alan Stern <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>
> > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > Cc: Mathias Nyman <[e-mail]>
> > Cc: Oliver Neukum <[e-mail]>
> > Cc: Thomas Gleixner <[e-mail]>
> > Cc: <[e-mail]> # 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?

Also, where is the locking for any of the above?  What guarantees that
the device will not go away during those pointer walks?

thanks,

greg k-h

Daniel Vetter Feb. 23, 2021, 11:46 a.m. UTC | #3

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 <[e-mail]>
> > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > Cc: Christoph Hellwig <[e-mail]>
> > Cc: Greg Kroah-Hartman <[e-mail]>
> > Cc: Johan Hovold <[e-mail]>
> > Cc: Alan Stern <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>
> > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > Cc: Mathias Nyman <[e-mail]>
> > Cc: Oliver Neukum <[e-mail]>
> > Cc: Thomas Gleixner <[e-mail]>
> > Cc: <[e-mail]> # 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.
-Daniel

Greg Kroah-Hartman Feb. 23, 2021, 12:02 p.m. UTC | #4

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 <[e-mail]>
> > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > Cc: Christoph Hellwig <[e-mail]>
> > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > Cc: Johan Hovold <[e-mail]>
> > > Cc: Alan Stern <[e-mail]>
> > > Cc: Andy Shevchenko <[e-mail]>
> > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > Cc: Mathias Nyman <[e-mail]>
> > > Cc: Oliver Neukum <[e-mail]>
> > > Cc: Thomas Gleixner <[e-mail]>
> > > Cc: <[e-mail]> # 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

Daniel Vetter Feb. 23, 2021, 12:14 p.m. UTC | #5

On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
>
> 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 <[e-mail]>
> > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > Cc: Christoph Hellwig <[e-mail]>
> > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > Cc: Johan Hovold <[e-mail]>
> > > > Cc: Alan Stern <[e-mail]>
> > > > Cc: Andy Shevchenko <[e-mail]>
> > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > Cc: Mathias Nyman <[e-mail]>
> > > > Cc: Oliver Neukum <[e-mail]>
> > > > Cc: Thomas Gleixner <[e-mail]>
> > > > Cc: <[e-mail]> # 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?  :)

Because the usb fix isn't the right fix here, it's just the duct-tape.
We don't want to dig around in these internals, it's just a convenient
way to shut up the dma-api until drm has sorted out its confusion.

We can polish the turd if you want, but the thing is, it's still a turd ...

The right fix is to change drm_prime.c code to not call dma_map_sg
when we don't need it. The problem is that roughly 3 layers of code
(drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
drm_prime is shared by all drm drivers, all other drm drivers are
impacted too. We're not going to be able to cc: stable that kind of
stuff. Thomas actually started with that series, until I pointed out
how bad things really are.

And before you ask: The dma-api change makes sense, and dma-api vs drm
relations are strained since years, so we're not going ask for some
hack there for our abuse to paper over the regression. I've been in
way too many of those threads, only result is shouting and failed
anger management.
-Daniel

Greg Kroah-Hartman Feb. 23, 2021, 12:24 p.m. UTC | #6

On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> >
> > 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 <[e-mail]>
> > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > Cc: Johan Hovold <[e-mail]>
> > > > > Cc: Alan Stern <[e-mail]>
> > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > Cc: <[e-mail]> # 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?  :)
> 
> Because the usb fix isn't the right fix here, it's just the duct-tape.
> We don't want to dig around in these internals, it's just a convenient
> way to shut up the dma-api until drm has sorted out its confusion.
> 
> We can polish the turd if you want, but the thing is, it's still a turd ...
> 
> The right fix is to change drm_prime.c code to not call dma_map_sg
> when we don't need it. The problem is that roughly 3 layers of code
> (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> drm_prime is shared by all drm drivers, all other drm drivers are
> impacted too. We're not going to be able to cc: stable that kind of
> stuff. Thomas actually started with that series, until I pointed out
> how bad things really are.
> 
> And before you ask: The dma-api change makes sense, and dma-api vs drm
> relations are strained since years, so we're not going ask for some
> hack there for our abuse to paper over the regression. I've been in
> way too many of those threads, only result is shouting and failed
> anger management.

Let's do it right.  If this is a regression from 5.9, it isn't a huge
one as that kernel was released last October.  I don't like to see this
messing around with USB internals in non-USB-core code please.

thanks,

greg k-h

Thomas Zimmermann Feb. 23, 2021, 12:37 p.m. UTC | #7

Hi

Am 23.02.21 um 12:19 schrieb Greg KH:
> 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 <[e-mail]>
>> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
>> Cc: Christoph Hellwig <[e-mail]>
>> Cc: Greg Kroah-Hartman <[e-mail]>
>> Cc: Johan Hovold <[e-mail]>
>> Cc: Alan Stern <[e-mail]>
>> Cc: Andy Shevchenko <[e-mail]>
>> Cc: Sebastian Andrzej Siewior <[e-mail]>
>> Cc: Mathias Nyman <[e-mail]>
>> Cc: Oliver Neukum <[e-mail]>
>> Cc: Thomas Gleixner <[e-mail]>
>> Cc: <[e-mail]> # 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?

I don't know about vc4 or storage. My guess is that they don't call 
dma_map_sgtable() for devices with dma_mask. Ideally, USB DRM devices 
wouldn't do that either, but, as Daniel explained, DRM's PRIME framework 
expects a dma_mask on the importing device.

The real fix would move this from framework to drivers, so that each 
driver can import the dmabuf according to its capabilities. I tried to 
do this with v2 of this patch, but I was not feasible at this time.

For this to work, we'd have rework at least 3 drivers, the PRIME 
framework and the dmabuf framework. I don't think the stable maintainer 
would be keen on merging that. ;)

Wrt your question about the USB core: what we do here is a workaround 
for dmabuf importing. The DRM USB drivers don't even use the resulting 
page mapping directly. Putting the workaround into the USB core is maybe 
not useful. If we ever use DMA directly for streaming framebuffers to 
the device, thinks might be different.

Best regards
Thomas

> 
> thanks,
> 
> greg k-h
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Daniel Vetter Feb. 23, 2021, 12:40 p.m. UTC | #8

On Tue, Feb 23, 2021 at 1:24 PM Greg KH <[e-mail]> wrote:
>
> On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> > >
> > > 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 <[e-mail]>
> > > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > > Cc: Johan Hovold <[e-mail]>
> > > > > > Cc: Alan Stern <[e-mail]>
> > > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > > Cc: <[e-mail]> # 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?  :)
> >
> > Because the usb fix isn't the right fix here, it's just the duct-tape.
> > We don't want to dig around in these internals, it's just a convenient
> > way to shut up the dma-api until drm has sorted out its confusion.
> >
> > We can polish the turd if you want, but the thing is, it's still a turd ...
> >
> > The right fix is to change drm_prime.c code to not call dma_map_sg
> > when we don't need it. The problem is that roughly 3 layers of code
> > (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> > drm_prime is shared by all drm drivers, all other drm drivers are
> > impacted too. We're not going to be able to cc: stable that kind of
> > stuff. Thomas actually started with that series, until I pointed out
> > how bad things really are.
> >
> > And before you ask: The dma-api change makes sense, and dma-api vs drm
> > relations are strained since years, so we're not going ask for some
> > hack there for our abuse to paper over the regression. I've been in
> > way too many of those threads, only result is shouting and failed
> > anger management.
>
> Let's do it right.  If this is a regression from 5.9, it isn't a huge
> one as that kernel was released last October.  I don't like to see this
> messing around with USB internals in non-USB-core code please.

So regressions don't count if it takes people more than 1 release to catch them?

New to me ... It does explain though why people really, really, really
don't want to upgrade kernels.

The thing is, the proper drm refactor will land earliest in 5.13, if
someone can figure it out, and there's no surprises/breakage in
drivers. If we're unlucky, it'll be later.

I'll work with Thomas to get this fixed some other way, with zero
usage of anything related to usb. But letting black screen regressions
hang out there simply because we absolutely have to have clean code
always: Nope, not going to happen.
-Daniel

Greg Kroah-Hartman Feb. 23, 2021, 12:44 p.m. UTC | #9

On Tue, Feb 23, 2021 at 01:37:09PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.02.21 um 12:19 schrieb Greg KH:
> > 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 <[e-mail]>
> > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > Cc: Christoph Hellwig <[e-mail]>
> > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > Cc: Johan Hovold <[e-mail]>
> > > Cc: Alan Stern <[e-mail]>
> > > Cc: Andy Shevchenko <[e-mail]>
> > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > Cc: Mathias Nyman <[e-mail]>
> > > Cc: Oliver Neukum <[e-mail]>
> > > Cc: Thomas Gleixner <[e-mail]>
> > > Cc: <[e-mail]> # 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?
> 
> I don't know about vc4 or storage. My guess is that they don't call
> dma_map_sgtable() for devices with dma_mask. Ideally, USB DRM devices
> wouldn't do that either, but, as Daniel explained, DRM's PRIME framework
> expects a dma_mask on the importing device.
> 
> The real fix would move this from framework to drivers, so that each driver
> can import the dmabuf according to its capabilities. I tried to do this with
> v2 of this patch, but I was not feasible at this time.
> 
> For this to work, we'd have rework at least 3 drivers, the PRIME framework
> and the dmabuf framework. I don't think the stable maintainer would be keen
> on merging that. ;)

Why not?  If it fixes an issue that has been reported, we've taken
bigger for smaller bugs :)

> Wrt your question about the USB core: what we do here is a workaround for
> dmabuf importing. The DRM USB drivers don't even use the resulting page
> mapping directly. Putting the workaround into the USB core is maybe not
> useful. If we ever use DMA directly for streaming framebuffers to the
> device, thinks might be different.

Then I really do not understand the issue here.  Why are you wanting to
grab a "naked" reference to the usb host controller device here?  What
ensures that it is correct (hint, lots of host controllers do not handle
dma), and what prevents it from going away underneath you?

And if storage doesn't have this problem, I can't see that it would be
that difficult for DRM to solve it correctly :)

thanks,

greg k-h

Daniel Vetter Feb. 23, 2021, 12:47 p.m. UTC | #10

On Tue, Feb 23, 2021 at 1:37 PM Thomas Zimmermann <[e-mail]> wrote:
>
> Hi
>
> Am 23.02.21 um 12:19 schrieb Greg KH:
> > 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 <[e-mail]>
> >> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> >> Cc: Christoph Hellwig <[e-mail]>
> >> Cc: Greg Kroah-Hartman <[e-mail]>
> >> Cc: Johan Hovold <[e-mail]>
> >> Cc: Alan Stern <[e-mail]>
> >> Cc: Andy Shevchenko <[e-mail]>
> >> Cc: Sebastian Andrzej Siewior <[e-mail]>
> >> Cc: Mathias Nyman <[e-mail]>
> >> Cc: Oliver Neukum <[e-mail]>
> >> Cc: Thomas Gleixner <[e-mail]>
> >> Cc: <[e-mail]> # 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?
>
> I don't know about vc4 or storage. My guess is that they don't call
> dma_map_sgtable() for devices with dma_mask. Ideally, USB DRM devices
> wouldn't do that either, but, as Daniel explained, DRM's PRIME framework
> expects a dma_mask on the importing device.
>
> The real fix would move this from framework to drivers, so that each
> driver can import the dmabuf according to its capabilities. I tried to
> do this with v2 of this patch, but I was not feasible at this time.
>
> For this to work, we'd have rework at least 3 drivers, the PRIME
> framework and the dmabuf framework. I don't think the stable maintainer
> would be keen on merging that. ;)
>
> Wrt your question about the USB core: what we do here is a workaround
> for dmabuf importing. The DRM USB drivers don't even use the resulting
> page mapping directly. Putting the workaround into the USB core is maybe
> not useful. If we ever use DMA directly for streaming framebuffers to
> the device, thinks might be different.

Yeah if we ever do this (direct dma from dma-buf to usb devices) then
a clean interface would need the usb subsystem to take care of the
dma-buf attachment, dma setup and everything in the usb host driver.
So even in that hypothetical world, none of this is anything that usb
ever will want, we'd need an interface at a slightly higher level.
Essentially urb transfer function that can receive/send into/from
dma-buf.
-Daniel

>
> Best regards
> Thomas
>
> >
> > thanks,
> >
> > greg k-h
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > 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
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter Feb. 23, 2021, 12:49 p.m. UTC | #11

On Tue, Feb 23, 2021 at 1:44 PM Greg KH <[e-mail]> wrote:
>
> On Tue, Feb 23, 2021 at 01:37:09PM +0100, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 23.02.21 um 12:19 schrieb Greg KH:
> > > 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 <[e-mail]>
> > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > Cc: Christoph Hellwig <[e-mail]>
> > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > Cc: Johan Hovold <[e-mail]>
> > > > Cc: Alan Stern <[e-mail]>
> > > > Cc: Andy Shevchenko <[e-mail]>
> > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > Cc: Mathias Nyman <[e-mail]>
> > > > Cc: Oliver Neukum <[e-mail]>
> > > > Cc: Thomas Gleixner <[e-mail]>
> > > > Cc: <[e-mail]> # 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?
> >
> > I don't know about vc4 or storage. My guess is that they don't call
> > dma_map_sgtable() for devices with dma_mask. Ideally, USB DRM devices
> > wouldn't do that either, but, as Daniel explained, DRM's PRIME framework
> > expects a dma_mask on the importing device.
> >
> > The real fix would move this from framework to drivers, so that each driver
> > can import the dmabuf according to its capabilities. I tried to do this with
> > v2 of this patch, but I was not feasible at this time.
> >
> > For this to work, we'd have rework at least 3 drivers, the PRIME framework
> > and the dmabuf framework. I don't think the stable maintainer would be keen
> > on merging that. ;)
>
> Why not?  If it fixes an issue that has been reported, we've taken
> bigger for smaller bugs :)

The problem is also that I can't just invent a bunch of people out of
thin are to make it happen. If you have them, please send them over,
there's lots to do here :-)

> > Wrt your question about the USB core: what we do here is a workaround for
> > dmabuf importing. The DRM USB drivers don't even use the resulting page
> > mapping directly. Putting the workaround into the USB core is maybe not
> > useful. If we ever use DMA directly for streaming framebuffers to the
> > device, thinks might be different.
>
> Then I really do not understand the issue here.  Why are you wanting to
> grab a "naked" reference to the usb host controller device here?  What
> ensures that it is correct (hint, lots of host controllers do not handle
> dma), and what prevents it from going away underneath you?

We know it's not correct, it's just a hack. We never use this even,
it's just there to not have to rewrite/audit large chunks of drm code.

> And if storage doesn't have this problem, I can't see that it would be
> that difficult for DRM to solve it correctly :)

Give me infinite people, and there's no problem.
-Daniel

Greg Kroah-Hartman Feb. 23, 2021, 12:50 p.m. UTC | #12

On Tue, Feb 23, 2021 at 01:40:51PM +0100, Daniel Vetter wrote:
> On Tue, Feb 23, 2021 at 1:24 PM Greg KH <[e-mail]> wrote:
> >
> > On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> > > On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> > > >
> > > > 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 <[e-mail]>
> > > > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > > > Cc: Johan Hovold <[e-mail]>
> > > > > > > Cc: Alan Stern <[e-mail]>
> > > > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > > > Cc: <[e-mail]> # 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?  :)
> > >
> > > Because the usb fix isn't the right fix here, it's just the duct-tape.
> > > We don't want to dig around in these internals, it's just a convenient
> > > way to shut up the dma-api until drm has sorted out its confusion.
> > >
> > > We can polish the turd if you want, but the thing is, it's still a turd ...
> > >
> > > The right fix is to change drm_prime.c code to not call dma_map_sg
> > > when we don't need it. The problem is that roughly 3 layers of code
> > > (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> > > drm_prime is shared by all drm drivers, all other drm drivers are
> > > impacted too. We're not going to be able to cc: stable that kind of
> > > stuff. Thomas actually started with that series, until I pointed out
> > > how bad things really are.
> > >
> > > And before you ask: The dma-api change makes sense, and dma-api vs drm
> > > relations are strained since years, so we're not going ask for some
> > > hack there for our abuse to paper over the regression. I've been in
> > > way too many of those threads, only result is shouting and failed
> > > anger management.
> >
> > Let's do it right.  If this is a regression from 5.9, it isn't a huge
> > one as that kernel was released last October.  I don't like to see this
> > messing around with USB internals in non-USB-core code please.
> 
> So regressions don't count if it takes people more than 1 release to catch them?
> 
> New to me ... It does explain though why people really, really, really
> don't want to upgrade kernels.

No, don't be silly.  I mean that you can take the time (i.e. a week) to
do it right here.

> The thing is, the proper drm refactor will land earliest in 5.13, if
> someone can figure it out, and there's no surprises/breakage in
> drivers. If we're unlucky, it'll be later.

A whole refactor seems really odd when all you need is access to the
proper dma mask stuff, right?  That's what this patch is doing.  That's
what the storage drivers need as well, so I don't really understand why
drm is so unique here with regards to this.

> I'll work with Thomas to get this fixed some other way, with zero
> usage of anything related to usb. But letting black screen regressions
> hang out there simply because we absolutely have to have clean code
> always: Nope, not going to happen.

I'm just saying to not put USB core code/logic in a random driver, which
I'm sure you can agree with.  You wouldn't want to see something like
that in a usb gadget driver for a drm device, right?  :)

I don't understand what is so convoluted here with DRM and why this
became an issue for that subsystem only on 5.9 and no other subsystem
that does this same type of thing for USB drivers.

thanks,

greg k-h

Thomas Zimmermann Feb. 23, 2021, 12:51 p.m. UTC | #13

Hi

Am 23.02.21 um 13:24 schrieb Greg KH:
> On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
>> On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
>>>
>>> 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 <[e-mail]>
>>>>>> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
>>>>>> Cc: Christoph Hellwig <[e-mail]>
>>>>>> Cc: Greg Kroah-Hartman <[e-mail]>
>>>>>> Cc: Johan Hovold <[e-mail]>
>>>>>> Cc: Alan Stern <[e-mail]>
>>>>>> Cc: Andy Shevchenko <[e-mail]>
>>>>>> Cc: Sebastian Andrzej Siewior <[e-mail]>
>>>>>> Cc: Mathias Nyman <[e-mail]>
>>>>>> Cc: Oliver Neukum <[e-mail]>
>>>>>> Cc: Thomas Gleixner <[e-mail]>
>>>>>> Cc: <[e-mail]> # 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?  :)
>>
>> Because the usb fix isn't the right fix here, it's just the duct-tape.
>> We don't want to dig around in these internals, it's just a convenient
>> way to shut up the dma-api until drm has sorted out its confusion.
>>
>> We can polish the turd if you want, but the thing is, it's still a turd ...
>>
>> The right fix is to change drm_prime.c code to not call dma_map_sg
>> when we don't need it. The problem is that roughly 3 layers of code
>> (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
>> drm_prime is shared by all drm drivers, all other drm drivers are
>> impacted too. We're not going to be able to cc: stable that kind of
>> stuff. Thomas actually started with that series, until I pointed out
>> how bad things really are.
>>
>> And before you ask: The dma-api change makes sense, and dma-api vs drm
>> relations are strained since years, so we're not going ask for some
>> hack there for our abuse to paper over the regression. I've been in
>> way too many of those threads, only result is shouting and failed
>> anger management.
> 
> Let's do it right.  If this is a regression from 5.9, it isn't a huge
> one as that kernel was released last October.  I don't like to see this
> messing around with USB internals in non-USB-core code please.

I get

  > git tag --contains 6eb0233ec2d0
  ...
  v5.10-rc1
  ...

as the first upstream release. The regression is only now hitting 
bleeding-edge distros. And those USB adapters are used as secondary, 
additional graphics cards, so it's not like most users will see the 
issue immediately after upgrading.

Display mirroring/joining is the typical way of using these adapters. 
Their main use case is now broken.

Best regards
Thomas

> 
> thanks,
> 
> greg k-h
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Greg Kroah-Hartman Feb. 23, 2021, 12:52 p.m. UTC | #14

On Tue, Feb 23, 2021 at 01:49:50PM +0100, Daniel Vetter wrote:
> On Tue, Feb 23, 2021 at 1:44 PM Greg KH <[e-mail]> wrote:
> >
> > On Tue, Feb 23, 2021 at 01:37:09PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 23.02.21 um 12:19 schrieb Greg KH:
> > > > 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 <[e-mail]>
> > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > Cc: Johan Hovold <[e-mail]>
> > > > > Cc: Alan Stern <[e-mail]>
> > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > Cc: <[e-mail]> # 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?
> > >
> > > I don't know about vc4 or storage. My guess is that they don't call
> > > dma_map_sgtable() for devices with dma_mask. Ideally, USB DRM devices
> > > wouldn't do that either, but, as Daniel explained, DRM's PRIME framework
> > > expects a dma_mask on the importing device.
> > >
> > > The real fix would move this from framework to drivers, so that each driver
> > > can import the dmabuf according to its capabilities. I tried to do this with
> > > v2 of this patch, but I was not feasible at this time.
> > >
> > > For this to work, we'd have rework at least 3 drivers, the PRIME framework
> > > and the dmabuf framework. I don't think the stable maintainer would be keen
> > > on merging that. ;)
> >
> > Why not?  If it fixes an issue that has been reported, we've taken
> > bigger for smaller bugs :)
> 
> The problem is also that I can't just invent a bunch of people out of
> thin are to make it happen. If you have them, please send them over,
> there's lots to do here :-)
> 
> > > Wrt your question about the USB core: what we do here is a workaround for
> > > dmabuf importing. The DRM USB drivers don't even use the resulting page
> > > mapping directly. Putting the workaround into the USB core is maybe not
> > > useful. If we ever use DMA directly for streaming framebuffers to the
> > > device, thinks might be different.
> >
> > Then I really do not understand the issue here.  Why are you wanting to
> > grab a "naked" reference to the usb host controller device here?  What
> > ensures that it is correct (hint, lots of host controllers do not handle
> > dma), and what prevents it from going away underneath you?
> 
> We know it's not correct, it's just a hack. We never use this even,
> it's just there to not have to rewrite/audit large chunks of drm code.

If you don't need the device, then why do you need a pointer to it?

Now I'm totally lost and confused what this patch is even supposed to be
doing in the first place :(

Is all you need that magic "mask"?  Something else?

Again, passing around "naked" pointers is going to cause problems, what
happens when the bus is removed from the system?

thanks,

greg k-h

Daniel Vetter Feb. 23, 2021, 12:59 p.m. UTC | #15

On Tue, Feb 23, 2021 at 1:50 PM Greg KH <[e-mail]> wrote:
>
> On Tue, Feb 23, 2021 at 01:40:51PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 23, 2021 at 1:24 PM Greg KH <[e-mail]> wrote:
> > >
> > > On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> > > > On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > 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 <[e-mail]>
> > > > > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > > > > Cc: Johan Hovold <[e-mail]>
> > > > > > > > Cc: Alan Stern <[e-mail]>
> > > > > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > > > > Cc: <[e-mail]> # 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?  :)
> > > >
> > > > Because the usb fix isn't the right fix here, it's just the duct-tape.
> > > > We don't want to dig around in these internals, it's just a convenient
> > > > way to shut up the dma-api until drm has sorted out its confusion.
> > > >
> > > > We can polish the turd if you want, but the thing is, it's still a turd ...
> > > >
> > > > The right fix is to change drm_prime.c code to not call dma_map_sg
> > > > when we don't need it. The problem is that roughly 3 layers of code
> > > > (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> > > > drm_prime is shared by all drm drivers, all other drm drivers are
> > > > impacted too. We're not going to be able to cc: stable that kind of
> > > > stuff. Thomas actually started with that series, until I pointed out
> > > > how bad things really are.
> > > >
> > > > And before you ask: The dma-api change makes sense, and dma-api vs drm
> > > > relations are strained since years, so we're not going ask for some
> > > > hack there for our abuse to paper over the regression. I've been in
> > > > way too many of those threads, only result is shouting and failed
> > > > anger management.
> > >
> > > Let's do it right.  If this is a regression from 5.9, it isn't a huge
> > > one as that kernel was released last October.  I don't like to see this
> > > messing around with USB internals in non-USB-core code please.
> >
> > So regressions don't count if it takes people more than 1 release to catch them?
> >
> > New to me ... It does explain though why people really, really, really
> > don't want to upgrade kernels.
>
> No, don't be silly.  I mean that you can take the time (i.e. a week) to
> do it right here.
>
> > The thing is, the proper drm refactor will land earliest in 5.13, if
> > someone can figure it out, and there's no surprises/breakage in
> > drivers. If we're unlucky, it'll be later.
>
> A whole refactor seems really odd when all you need is access to the
> proper dma mask stuff, right?  That's what this patch is doing.  That's
> what the storage drivers need as well, so I don't really understand why
> drm is so unique here with regards to this.
>
> > I'll work with Thomas to get this fixed some other way, with zero
> > usage of anything related to usb. But letting black screen regressions
> > hang out there simply because we absolutely have to have clean code
> > always: Nope, not going to happen.
>
> I'm just saying to not put USB core code/logic in a random driver, which
> I'm sure you can agree with.  You wouldn't want to see something like
> that in a usb gadget driver for a drm device, right?  :)
>
> I don't understand what is so convoluted here with DRM and why this
> became an issue for that subsystem only on 5.9 and no other subsystem
> that does this same type of thing for USB drivers.

Once more the problem.

- drm_prime code results in a calling dma_map_sg when importing a
dma-buf, on all devices. Even when your device never does dma, and all
dma-buf access is done through the cpu side using dma_buf_vmap, like
is the case for all usb drm drivers.
- this was harmless before the patch that landed in 5.9, even when
your device never had dma needs
- this is obviously not very cool, but also about a 10 year old
mistake, and so it has leaked _everywhere_ in the graphics subsystem.
You're not going to fix this in one week, since it's kinda been on the
todo list as a "would be nice to clean this up" for 10 years by now

All we need is some device (we really don't care which) where we can
dma_map_sg. Host device is convenient, but we can take any other, it
really doesn't matter to "fix" the regression.

So really, you want absolutely none of this in the usb subsystem,
since it really doesn't make sense at all technically. All it does is
keep drm_prime code going for another release so we can maybe fix this
for real, and meanwhile stuff isn't broken.
-Daniel

Greg Kroah-Hartman Feb. 23, 2021, 1:09 p.m. UTC | #16

On Tue, Feb 23, 2021 at 01:51:09PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.02.21 um 13:24 schrieb Greg KH:
> > On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> > > On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> > > > 
> > > > 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 <[e-mail]>
> > > > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > > > Cc: Johan Hovold <[e-mail]>
> > > > > > > Cc: Alan Stern <[e-mail]>
> > > > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > > > Cc: <[e-mail]> # 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?  :)
> > > 
> > > Because the usb fix isn't the right fix here, it's just the duct-tape.
> > > We don't want to dig around in these internals, it's just a convenient
> > > way to shut up the dma-api until drm has sorted out its confusion.
> > > 
> > > We can polish the turd if you want, but the thing is, it's still a turd ...
> > > 
> > > The right fix is to change drm_prime.c code to not call dma_map_sg
> > > when we don't need it. The problem is that roughly 3 layers of code
> > > (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> > > drm_prime is shared by all drm drivers, all other drm drivers are
> > > impacted too. We're not going to be able to cc: stable that kind of
> > > stuff. Thomas actually started with that series, until I pointed out
> > > how bad things really are.
> > > 
> > > And before you ask: The dma-api change makes sense, and dma-api vs drm
> > > relations are strained since years, so we're not going ask for some
> > > hack there for our abuse to paper over the regression. I've been in
> > > way too many of those threads, only result is shouting and failed
> > > anger management.
> > 
> > Let's do it right.  If this is a regression from 5.9, it isn't a huge
> > one as that kernel was released last October.  I don't like to see this
> > messing around with USB internals in non-USB-core code please.
> 
> I get
> 
>  > git tag --contains 6eb0233ec2d0
>  ...
>  v5.10-rc1
>  ...

Ah, I thought you said 5.9 was when the problem happened, ok, yes, 5.10
is slow to get out to a lot of distros that do not update frequently :(

thanks,

greg k-h

Thomas Zimmermann Feb. 23, 2021, 1:43 p.m. UTC | #17

Hi

Am 23.02.21 um 13:52 schrieb Greg KH:
> On Tue, Feb 23, 2021 at 01:49:50PM +0100, Daniel Vetter wrote:
>> On Tue, Feb 23, 2021 at 1:44 PM Greg KH <[e-mail]> wrote:
>>>
>>> On Tue, Feb 23, 2021 at 01:37:09PM +0100, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 23.02.21 um 12:19 schrieb Greg KH:
>>>>> 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 <[e-mail]>
>>>>>> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
>>>>>> Cc: Christoph Hellwig <[e-mail]>
>>>>>> Cc: Greg Kroah-Hartman <[e-mail]>
>>>>>> Cc: Johan Hovold <[e-mail]>
>>>>>> Cc: Alan Stern <[e-mail]>
>>>>>> Cc: Andy Shevchenko <[e-mail]>
>>>>>> Cc: Sebastian Andrzej Siewior <[e-mail]>
>>>>>> Cc: Mathias Nyman <[e-mail]>
>>>>>> Cc: Oliver Neukum <[e-mail]>
>>>>>> Cc: Thomas Gleixner <[e-mail]>
>>>>>> Cc: <[e-mail]> # 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?
>>>>
>>>> I don't know about vc4 or storage. My guess is that they don't call
>>>> dma_map_sgtable() for devices with dma_mask. Ideally, USB DRM devices
>>>> wouldn't do that either, but, as Daniel explained, DRM's PRIME framework
>>>> expects a dma_mask on the importing device.
>>>>
>>>> The real fix would move this from framework to drivers, so that each driver
>>>> can import the dmabuf according to its capabilities. I tried to do this with
>>>> v2 of this patch, but I was not feasible at this time.
>>>>
>>>> For this to work, we'd have rework at least 3 drivers, the PRIME framework
>>>> and the dmabuf framework. I don't think the stable maintainer would be keen
>>>> on merging that. ;)
>>>
>>> Why not?  If it fixes an issue that has been reported, we've taken
>>> bigger for smaller bugs :)
>>
>> The problem is also that I can't just invent a bunch of people out of
>> thin are to make it happen. If you have them, please send them over,
>> there's lots to do here :-)
>>
>>>> Wrt your question about the USB core: what we do here is a workaround for
>>>> dmabuf importing. The DRM USB drivers don't even use the resulting page
>>>> mapping directly. Putting the workaround into the USB core is maybe not
>>>> useful. If we ever use DMA directly for streaming framebuffers to the
>>>> device, thinks might be different.
>>>
>>> Then I really do not understand the issue here.  Why are you wanting to
>>> grab a "naked" reference to the usb host controller device here?  What
>>> ensures that it is correct (hint, lots of host controllers do not handle
>>> dma), and what prevents it from going away underneath you?
>>
>> We know it's not correct, it's just a hack. We never use this even,
>> it's just there to not have to rewrite/audit large chunks of drm code.
> 
> If you don't need the device, then why do you need a pointer to it?
> 
> Now I'm totally lost and confused what this patch is even supposed to be
> doing in the first place :(
> 
> Is all you need that magic "mask"?  Something else?

We only need the dma_mask field. importing currently fails at [1]. As 
the driver doesn't use DMA, it really doesn't matter which device 
provides it.

> 
> Again, passing around "naked" pointers is going to cause problems, what
> happens when the bus is removed from the system?

If this requires locks, we can certainly hold them while being in the 
new helper function. After we completed the import, we don't have to 
care any longer about the devices involved.

Best regards
Thomas

[1] https://elixir.bootlin.com/linux/v5.11/source/kernel/dma/mapping.c#L190
> 
> thanks,
> 
> greg k-h
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Takashi Iwai Feb. 23, 2021, 1:44 p.m. UTC | #18

On Tue, 23 Feb 2021 11:58:42 +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 <[e-mail]>
> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> Cc: Christoph Hellwig <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Johan Hovold <[e-mail]>
> Cc: Alan Stern <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Sebastian Andrzej Siewior <[e-mail]>
> Cc: Mathias Nyman <[e-mail]>
> Cc: Oliver Neukum <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: <[e-mail]> # 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;

Aside from the discussion whether this "workaround" is needed, the use
of udev->bus->controller here looks a bit suspicious.  As the old USB
code (before the commit 6eb0233ec2d0) indicated, it was rather
usb->bus->sysdev that was used for the DMA mask, and it's also the one
most of USB core code refers to.  A similar question came up while
fixing the same kind of bug in the media subsystem, and we concluded
that bus->sysdev is a better choice.


Takashi

Thomas Zimmermann Feb. 23, 2021, 2:06 p.m. UTC | #19

Hi

Am 23.02.21 um 14:44 schrieb Takashi Iwai:
> On Tue, 23 Feb 2021 11:58:42 +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 <[e-mail]>
>> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
>> Cc: Christoph Hellwig <[e-mail]>
>> Cc: Greg Kroah-Hartman <[e-mail]>
>> Cc: Johan Hovold <[e-mail]>
>> Cc: Alan Stern <[e-mail]>
>> Cc: Andy Shevchenko <[e-mail]>
>> Cc: Sebastian Andrzej Siewior <[e-mail]>
>> Cc: Mathias Nyman <[e-mail]>
>> Cc: Oliver Neukum <[e-mail]>
>> Cc: Thomas Gleixner <[e-mail]>
>> Cc: <[e-mail]> # 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;
> 
> Aside from the discussion whether this "workaround" is needed, the use
> of udev->bus->controller here looks a bit suspicious.  As the old USB
> code (before the commit 6eb0233ec2d0) indicated, it was rather
> usb->bus->sysdev that was used for the DMA mask, and it's also the one
> most of USB core code refers to.  A similar question came up while
> fixing the same kind of bug in the media subsystem, and we concluded
> that bus->sysdev is a better choice.

Good to hear that we're not the only ones affected by this. Wrt the 
original code, using sysdev makes even more sense.

Best regards
Thomas

> 
> 
> Takashi
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Alan Stern Feb. 23, 2021, 3:45 p.m. UTC | #20

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:

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

Thomas, I doubt that you have to go through all of this.  The 
usb-storage driver, for instance, just uses the equivalent of 
dev->dev->dma_mask.  Even though USB devices don't do DMA themselves, 
the DMA mask value is inherited from the parent host controller's device 
struct.

Have you tried doing this?

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

If Thomas finds that the approach I outlined above works, then the rest 
of this email thread becomes moot.  :-)

Alan Stern

Alan Stern Feb. 23, 2021, 4 p.m. UTC | #21

On Tue, Feb 23, 2021 at 03:06:07PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.02.21 um 14:44 schrieb Takashi Iwai:

> > Aside from the discussion whether this "workaround" is needed, the use
> > of udev->bus->controller here looks a bit suspicious.  As the old USB
> > code (before the commit 6eb0233ec2d0) indicated, it was rather
> > usb->bus->sysdev that was used for the DMA mask, and it's also the one
> > most of USB core code refers to.  A similar question came up while
> > fixing the same kind of bug in the media subsystem, and we concluded
> > that bus->sysdev is a better choice.
> 
> Good to hear that we're not the only ones affected by this. Wrt the original
> code, using sysdev makes even more sense.

Hmmm, I had forgotten about this.  So DMA masks aren't inherited after 
all, thanks to commit 6eb0233ec2d0.  That leas me to wonder how well 
usb-storage is really working these days...

The impression I get is that Greg would like the USB core to export a 
function which takes struct usb_interface * as argument and returns the 
appropriate DMA mask value.  Then instead of messing around with USB 
internals, drm_gem_prime_import_usb could just call this new function.

Adding such a utility function would be a sufficiently small change that 
it could go into the -stable kernels with no trouble.

Alan Stern

Takashi Iwai Feb. 23, 2021, 4:12 p.m. UTC | #22

On Tue, 23 Feb 2021 17:00:54 +0100,
Alan Stern wrote:
> 
> On Tue, Feb 23, 2021 at 03:06:07PM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 23.02.21 um 14:44 schrieb Takashi Iwai:
> 
> > > Aside from the discussion whether this "workaround" is needed, the use
> > > of udev->bus->controller here looks a bit suspicious.  As the old USB
> > > code (before the commit 6eb0233ec2d0) indicated, it was rather
> > > usb->bus->sysdev that was used for the DMA mask, and it's also the one
> > > most of USB core code refers to.  A similar question came up while
> > > fixing the same kind of bug in the media subsystem, and we concluded
> > > that bus->sysdev is a better choice.
> > 
> > Good to hear that we're not the only ones affected by this. Wrt the original
> > code, using sysdev makes even more sense.
> 
> Hmmm, I had forgotten about this.  So DMA masks aren't inherited after 
> all, thanks to commit 6eb0233ec2d0.  That leas me to wonder how well 
> usb-storage is really working these days...
> 
> The impression I get is that Greg would like the USB core to export a 
> function which takes struct usb_interface * as argument and returns the 
> appropriate DMA mask value.  Then instead of messing around with USB 
> internals, drm_gem_prime_import_usb could just call this new function.
> 
> Adding such a utility function would be a sufficiently small change that 
> it could go into the -stable kernels with no trouble.

Note that drm isn't the only subsystem that needs the proper DMA
mask.  The media and sound subsystems require it, at least.
Those had already used sysdev for the DMA mask and the actual memory
allocation.


Takashi

Greg Kroah-Hartman Feb. 23, 2021, 5:30 p.m. UTC | #23

On Tue, Feb 23, 2021 at 11:00:54AM -0500, Alan Stern wrote:
> On Tue, Feb 23, 2021 at 03:06:07PM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 23.02.21 um 14:44 schrieb Takashi Iwai:
> 
> > > Aside from the discussion whether this "workaround" is needed, the use
> > > of udev->bus->controller here looks a bit suspicious.  As the old USB
> > > code (before the commit 6eb0233ec2d0) indicated, it was rather
> > > usb->bus->sysdev that was used for the DMA mask, and it's also the one
> > > most of USB core code refers to.  A similar question came up while
> > > fixing the same kind of bug in the media subsystem, and we concluded
> > > that bus->sysdev is a better choice.
> > 
> > Good to hear that we're not the only ones affected by this. Wrt the original
> > code, using sysdev makes even more sense.
> 
> Hmmm, I had forgotten about this.  So DMA masks aren't inherited after 
> all, thanks to commit 6eb0233ec2d0.  That leas me to wonder how well 
> usb-storage is really working these days...
> 
> The impression I get is that Greg would like the USB core to export a 
> function which takes struct usb_interface * as argument and returns the 
> appropriate DMA mask value.  Then instead of messing around with USB 
> internals, drm_gem_prime_import_usb could just call this new function.
> 
> Adding such a utility function would be a sufficiently small change that 
> it could go into the -stable kernels with no trouble.

Yes, sorry for not being clear, that is what I think would make the most
sense, then all USB drivers could use it easily and it can be changed in
one location if the DMA logic ever changes.

thanks,

greg k-h

Thomas Zimmermann Feb. 24, 2021, 5:59 a.m. UTC | #24

Hi

Am 23.02.21 um 18:30 schrieb Greg KH:
> On Tue, Feb 23, 2021 at 11:00:54AM -0500, Alan Stern wrote:
>> On Tue, Feb 23, 2021 at 03:06:07PM +0100, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 23.02.21 um 14:44 schrieb Takashi Iwai:
>>
>>>> Aside from the discussion whether this "workaround" is needed, the use
>>>> of udev->bus->controller here looks a bit suspicious.  As the old USB
>>>> code (before the commit 6eb0233ec2d0) indicated, it was rather
>>>> usb->bus->sysdev that was used for the DMA mask, and it's also the one
>>>> most of USB core code refers to.  A similar question came up while
>>>> fixing the same kind of bug in the media subsystem, and we concluded
>>>> that bus->sysdev is a better choice.
>>>
>>> Good to hear that we're not the only ones affected by this. Wrt the original
>>> code, using sysdev makes even more sense.
>>
>> Hmmm, I had forgotten about this.  So DMA masks aren't inherited after
>> all, thanks to commit 6eb0233ec2d0.  That leas me to wonder how well
>> usb-storage is really working these days...
>>
>> The impression I get is that Greg would like the USB core to export a
>> function which takes struct usb_interface * as argument and returns the
>> appropriate DMA mask value.  Then instead of messing around with USB
>> internals, drm_gem_prime_import_usb could just call this new function.
>>
>> Adding such a utility function would be a sufficiently small change that
>> it could go into the -stable kernels with no trouble.
> 
> Yes, sorry for not being clear, that is what I think would make the most
> sense, then all USB drivers could use it easily and it can be changed in
> one location if the DMA logic ever changes.

We can certainly do that. Thanks for clarifying. I'll send out an 
updated patch soon.

Best regards
Thomas

> 
> thanks,
> 
> greg k-h
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Thomas Zimmermann Feb. 24, 2021, 6:02 a.m. UTC | #25

Hi

Am 23.02.21 um 16:45 schrieb Alan Stern:
> 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:
> 
>>> --- 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);
> 
> Thomas, I doubt that you have to go through all of this.  The
> usb-storage driver, for instance, just uses the equivalent of
> dev->dev->dma_mask.  Even though USB devices don't do DMA themselves,
> the DMA mask value is inherited from the parent host controller's device
> struct.
> 
> Have you tried doing this?

But it's the field that is now NULL, isn't it? :S How does usb-storage 
get away with this?

Best regards
Thomas

> 
>> 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?
> 
> If Thomas finds that the approach I outlined above works, then the rest
> of this email thread becomes moot.  :-)
> 
> Alan Stern
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Sudip Mukherjee Feb. 25, 2021, 7:01 p.m. UTC | #26

On Tue, Feb 23, 2021 at 02:09:58PM +0100, Greg KH wrote:
> On Tue, Feb 23, 2021 at 01:51:09PM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 23.02.21 um 13:24 schrieb Greg KH:
> > > On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> > > > On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > > > > Cc: Johan Hovold <[e-mail]>
> > > > > > > > Cc: Alan Stern <[e-mail]>
> > > > > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > > > > Cc: <[e-mail]> # v5.10+
> > > > > > > > ---
<snip>
> > > > > > > 
> > > > > > > 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?  :)
> > > > 
> > > > Because the usb fix isn't the right fix here, it's just the duct-tape.
> > > > We don't want to dig around in these internals, it's just a convenient
> > > > way to shut up the dma-api until drm has sorted out its confusion.
> > > > 
> > > > We can polish the turd if you want, but the thing is, it's still a turd ...
> > > > 
> > > > The right fix is to change drm_prime.c code to not call dma_map_sg
> > > > when we don't need it. The problem is that roughly 3 layers of code
> > > > (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> > > > drm_prime is shared by all drm drivers, all other drm drivers are
> > > > impacted too. We're not going to be able to cc: stable that kind of
> > > > stuff. Thomas actually started with that series, until I pointed out
> > > > how bad things really are.
> > > > 
> > > > And before you ask: The dma-api change makes sense, and dma-api vs drm
> > > > relations are strained since years, so we're not going ask for some
> > > > hack there for our abuse to paper over the regression. I've been in
> > > > way too many of those threads, only result is shouting and failed
> > > > anger management.
> > > 
> > > Let's do it right.  If this is a regression from 5.9, it isn't a huge
> > > one as that kernel was released last October.  I don't like to see this
> > > messing around with USB internals in non-USB-core code please.
> > 
> > I get
> > 
> >  > git tag --contains 6eb0233ec2d0
> >  ...
> >  v5.10-rc1
> >  ...
> 
> Ah, I thought you said 5.9 was when the problem happened, ok, yes, 5.10
> is slow to get out to a lot of distros that do not update frequently :(

iiuc, Debian Bullseye release will be having v5.10.y.

Ben ?


--
Regards
Sudip

Salvatore Bonaccorso Feb. 25, 2021, 9:39 p.m. UTC | #27

Hi,

On Thu, Feb 25, 2021 at 07:01:57PM +0000, Sudip Mukherjee wrote:
> On Tue, Feb 23, 2021 at 02:09:58PM +0100, Greg KH wrote:
> > On Tue, Feb 23, 2021 at 01:51:09PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 23.02.21 um 13:24 schrieb Greg KH:
> > > > On Tue, Feb 23, 2021 at 01:14:30PM +0100, Daniel Vetter wrote:
> > > > > On Tue, Feb 23, 2021 at 1:02 PM Greg KH <[e-mail]> wrote:
> > > > > > 
> > > > > > 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 <[e-mail]>
> > > > > > > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> > > > > > > > > Cc: Christoph Hellwig <[e-mail]>
> > > > > > > > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > > > > > > > Cc: Johan Hovold <[e-mail]>
> > > > > > > > > Cc: Alan Stern <[e-mail]>
> > > > > > > > > Cc: Andy Shevchenko <[e-mail]>
> > > > > > > > > Cc: Sebastian Andrzej Siewior <[e-mail]>
> > > > > > > > > Cc: Mathias Nyman <[e-mail]>
> > > > > > > > > Cc: Oliver Neukum <[e-mail]>
> > > > > > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > > > > > Cc: <[e-mail]> # v5.10+
> > > > > > > > > ---
> <snip>
> > > > > > > > 
> > > > > > > > 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?  :)
> > > > > 
> > > > > Because the usb fix isn't the right fix here, it's just the duct-tape.
> > > > > We don't want to dig around in these internals, it's just a convenient
> > > > > way to shut up the dma-api until drm has sorted out its confusion.
> > > > > 
> > > > > We can polish the turd if you want, but the thing is, it's still a turd ...
> > > > > 
> > > > > The right fix is to change drm_prime.c code to not call dma_map_sg
> > > > > when we don't need it. The problem is that roughly 3 layers of code
> > > > > (drm_prime, dma-buf, gem shmem helpers) are involved. Plus, since
> > > > > drm_prime is shared by all drm drivers, all other drm drivers are
> > > > > impacted too. We're not going to be able to cc: stable that kind of
> > > > > stuff. Thomas actually started with that series, until I pointed out
> > > > > how bad things really are.
> > > > > 
> > > > > And before you ask: The dma-api change makes sense, and dma-api vs drm
> > > > > relations are strained since years, so we're not going ask for some
> > > > > hack there for our abuse to paper over the regression. I've been in
> > > > > way too many of those threads, only result is shouting and failed
> > > > > anger management.
> > > > 
> > > > Let's do it right.  If this is a regression from 5.9, it isn't a huge
> > > > one as that kernel was released last October.  I don't like to see this
> > > > messing around with USB internals in non-USB-core code please.
> > > 
> > > I get
> > > 
> > >  > git tag --contains 6eb0233ec2d0
> > >  ...
> > >  v5.10-rc1
> > >  ...
> > 
> > Ah, I thought you said 5.9 was when the problem happened, ok, yes, 5.10
> > is slow to get out to a lot of distros that do not update frequently :(
> 
> iiuc, Debian Bullseye release will be having v5.10.y.
> 
> Ben ?

That is correct, Debian bullseye will ship with 5.10.y.

Regards,
Salvatore

Patch

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);
+
+	return drm_gem_prime_import_dev(dev, dma_buf, usbhost);
+}
+EXPORT_SYMBOL(drm_gem_prime_import_usb);
+#endif
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 0b4f4f2af1ef..99e7bd36a220 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -611,7 +611,7 @@  static const struct drm_driver gm12u320_drm_driver = {
 	.minor		 = DRIVER_MINOR,

 	.fops		 = &gm12u320_fops,
-	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_GEM_SHMEM_DRIVER_OPS_USB,
 };

 static const struct drm_mode_config_funcs gm12u320_mode_config_funcs = {
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 9269092697d8..2db483b2b199 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -39,7 +39,7 @@  static const struct drm_driver driver = {

 	/* GEM hooks */
 	.fops = &udl_driver_fops,
-	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_GEM_SHMEM_DRIVER_OPS_USB,

 	.name = DRIVER_NAME,
 	.desc = DRIVER_DESC,
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 434328d8a0d9..09d12f632cad 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -162,4 +162,17 @@  struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj);
 	.gem_prime_mmap		= drm_gem_prime_mmap, \
 	.dumb_create		= drm_gem_shmem_dumb_create

+#ifdef CONFIG_USB
+/**
+ * DRM_GEM_SHMEM_DRIVER_OPS_USB - Default shmem GEM operations for USB devices
+ *
+ * This macro provides a shortcut for setting the shmem GEM operations in
+ * the &drm_driver structure. Drivers for USB-based devices should use this
+ * macro instead of &DRM_GEM_SHMEM_DRIVER_OPS.
+ */
+#define DRM_GEM_SHMEM_DRIVER_OPS_USB \
+	DRM_GEM_SHMEM_DRIVER_OPS, \
+	.gem_prime_import = drm_gem_prime_import_usb
+#endif
+
 #endif /* __DRM_GEM_SHMEM_HELPER_H__ */
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 54f2c58305d2..b42e07edd9e6 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -110,4 +110,9 @@  int drm_prime_sg_to_page_array(struct sg_table *sgt, struct page **pages,
 int drm_prime_sg_to_dma_addr_array(struct sg_table *sgt, dma_addr_t *addrs,
 				   int max_pages);

+#ifdef CONFIG_USB
+struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
+						struct dma_buf *dma_buf);
+#endif
+
 #endif /* __DRM_PRIME_H__ */