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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <gregkh@xxxxxxxxxxxxxxxxxxx> 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 <tzimmermann@xxxxxxx>
Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: Johan Hovold <johan@xxxxxxxxxx>
Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
Cc: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
Cc: Oliver Neukum <oneukum@xxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx> # v5.10+
---
   drivers/gpu/drm/drm_prime.c        | 36 ++++++++++++++++++++++++++++++
   drivers/gpu/drm/tiny/gm12u320.c    |  2 +-
   drivers/gpu/drm/udl/udl_drv.c      |  2 +-
   include/drm/drm_gem_shmem_helper.h | 13 +++++++++++
   include/drm/drm_prime.h            |  5 +++++
   5 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 2a54f86856af..9015850f2160 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -29,6 +29,7 @@
   #include <linux/export.h>
   #include <linux/dma-buf.h>
   #include <linux/rbtree.h>
+#include <linux/usb.h>

   #include <drm/drm.h>
   #include <drm/drm_drv.h>
@@ -1055,3 +1056,38 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
           dma_buf_put(dma_buf);
   }
   EXPORT_SYMBOL(drm_prime_gem_destroy);
+
+/**
+ * drm_gem_prime_import_usb - helper library implementation of the import callback for USB devices
+ * @dev: drm_device to import into
+ * @dma_buf: dma-buf object to import
+ *
+ * This is an implementation of drm_gem_prime_import() for USB-based devices.
+ * USB devices cannot perform DMA directly. This function selects the USB host
+ * controller as DMA device instead. Drivers can use this as their
+ * &drm_driver.gem_prime_import implementation.
+ *
+ * See also drm_gem_prime_import().
+ */
+#ifdef CONFIG_USB
+struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
+                                         struct dma_buf *dma_buf)
+{
+ struct usb_device *udev;
+ struct device *usbhost;
+
+ if (dev->dev->bus != &usb_bus_type)
+         return ERR_PTR(-ENODEV);
+
+ udev = interface_to_usbdev(to_usb_interface(dev->dev));
+ if (!udev->bus)
+         return ERR_PTR(-ENODEV);
+
+ usbhost = udev->bus->controller;
+ if (!usbhost || !usbhost->dma_mask)
+         return ERR_PTR(-ENODEV);

If individual USB drivers need access to this type of thing, shouldn't
that be done in the USB core itself?

{hint, yes}

There shouldn't be anything "special" about a DRM driver that needs this
vs. any other driver that might want to know about DMA things related to
a specific USB device.  Why isn't this an issue with the existing
storage or v4l USB devices?

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
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

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

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux