| Message ID | 20210303084512.25635-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7] drm: Use USB controller's DMA mask when importing dmabufs | expand |
Den 03.03.2021 09.45, skrev Thomas Zimmermann: > USB devices cannot perform DMA and hence have no dma_mask set in their > device structure. Therefore importing dmabuf into a USB-based driver > fails, which breaks joining and mirroring of display in X11. > > For USB devices, pick the associated USB controller as attachment device. > This allows the DRM import helpers to perform the DMA setup. If the DMA > controller does not support DMA transfers, we're out of luck and cannot > import. Our current USB-based DRM drivers don't use DMA, so the actual > DMA device is not important. > > Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their > instance of struct drm_driver. > This doesn't seem to be the case anymore. > Tested by joining/mirroring displays of udl and radeon un der Gnome/X11. s/un der/under/ > > v7: > * fix use-before-init bug in gm12u320 (Dan) > v6: > * implement workaround in DRM drivers and hold reference to > DMA device while USB device is in use > * remove dev_is_usb() (Greg) > * collapse USB helper into usb_intf_get_dma_device() (Alan) > * integrate Daniel's TODO statement (Daniel) > * fix typos (Greg) > v5: > * provide a helper for USB interfaces (Alan) > * add FIXME item to documentation and TODO list (Daniel) > v4: > * implement workaround with USB helper functions (Greg) > * use struct usb_device->bus->sysdev as DMA device (Takashi) > v3: > * drop gem_create_object > * use DMA mask of USB controller, if any (Daniel, Christian, Noralf) > v2: > * move fix to importer side (Christian, Daniel) > * update SHMEM and CMA helpers for new PRIME callbacks > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices") > Tested-by: Pavel Machek <pavel@ucw.cz> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Acked-by: Christian König <christian.koenig@amd.com> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: <stable@vger.kernel.org> # v5.10+ > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > Documentation/gpu/todo.rst | 21 +++++++++++++++++++++ > drivers/gpu/drm/tiny/gm12u320.c | 28 ++++++++++++++++++++++++++-- > drivers/gpu/drm/udl/udl_drv.c | 17 +++++++++++++++++ > drivers/gpu/drm/udl/udl_drv.h | 1 + > drivers/gpu/drm/udl/udl_main.c | 9 +++++++++ > drivers/usb/core/usb.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/usb.h | 2 ++ > 7 files changed, 108 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > index 0b4f4f2af1ef..4fe372f43cf5 100644 > --- a/drivers/gpu/drm/tiny/gm12u320.c > +++ b/drivers/gpu/drm/tiny/gm12u320.c [...] > @@ -638,12 +656,15 @@ static int gm12u320_usb_probe(struct usb_interface *interface, > struct gm12u320_device, dev); > if (IS_ERR(gm12u320)) > return PTR_ERR(gm12u320); > + dev = &gm12u320->dev; > + > + gm12u320->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev)); > + if (!gm12u320->dmadev) > + drm_warn(dev, "buffer sharing not supported"); /* not an error */ > When implementing this in my own driver I discovered that this device ref will leak if probing fails after this. I've done it like this: gdrm->dmadev = usb_intf_get_dma_device(intf); if (!gdrm->dmadev) dev_warn(dev, "buffer sharing not supported"); ret = drm_dev_register(drm, 0); if (ret) { put_device(gdrm->dmadev); return ret; } An even better solution would be to have a devm_ version of the function. Noralf. > INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work); > mutex_init(&gm12u320->fb_update.lock); > > - dev = &gm12u320->dev; > - > ret = drmm_mode_config_init(dev); > if (ret) > return ret;
Hi, thanks for reviewing. This will be fixed in the next iteration. Best regards Thomas Am 03.03.21 um 11:58 schrieb Noralf Trønnes: > > > Den 03.03.2021 09.45, skrev Thomas Zimmermann: >> USB devices cannot perform DMA and hence have no dma_mask set in their >> device structure. Therefore importing dmabuf into a USB-based driver >> fails, which breaks joining and mirroring of display in X11. >> >> For USB devices, pick the associated USB controller as attachment device. >> This allows the DRM import helpers to perform the DMA setup. If the DMA >> controller does not support DMA transfers, we're out of luck and cannot >> import. Our current USB-based DRM drivers don't use DMA, so the actual >> DMA device is not important. >> >> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their >> instance of struct drm_driver. >> > > This doesn't seem to be the case anymore. > >> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11. > > s/un der/under/ > >> >> v7: >> * fix use-before-init bug in gm12u320 (Dan) >> v6: >> * implement workaround in DRM drivers and hold reference to >> DMA device while USB device is in use >> * remove dev_is_usb() (Greg) >> * collapse USB helper into usb_intf_get_dma_device() (Alan) >> * integrate Daniel's TODO statement (Daniel) >> * fix typos (Greg) >> v5: >> * provide a helper for USB interfaces (Alan) >> * add FIXME item to documentation and TODO list (Daniel) >> v4: >> * implement workaround with USB helper functions (Greg) >> * use struct usb_device->bus->sysdev as DMA device (Takashi) >> v3: >> * drop gem_create_object >> * use DMA mask of USB controller, if any (Daniel, Christian, Noralf) >> v2: >> * move fix to importer side (Christian, Daniel) >> * update SHMEM and CMA helpers for new PRIME callbacks >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices") >> Tested-by: Pavel Machek <pavel@ucw.cz> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Acked-by: Christian König <christian.koenig@amd.com> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: <stable@vger.kernel.org> # v5.10+ >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> Documentation/gpu/todo.rst | 21 +++++++++++++++++++++ >> drivers/gpu/drm/tiny/gm12u320.c | 28 ++++++++++++++++++++++++++-- >> drivers/gpu/drm/udl/udl_drv.c | 17 +++++++++++++++++ >> drivers/gpu/drm/udl/udl_drv.h | 1 + >> drivers/gpu/drm/udl/udl_main.c | 9 +++++++++ >> drivers/usb/core/usb.c | 32 ++++++++++++++++++++++++++++++++ >> include/linux/usb.h | 2 ++ >> 7 files changed, 108 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c >> index 0b4f4f2af1ef..4fe372f43cf5 100644 >> --- a/drivers/gpu/drm/tiny/gm12u320.c >> +++ b/drivers/gpu/drm/tiny/gm12u320.c > > [...] > >> @@ -638,12 +656,15 @@ static int gm12u320_usb_probe(struct usb_interface *interface, >> struct gm12u320_device, dev); >> if (IS_ERR(gm12u320)) >> return PTR_ERR(gm12u320); >> + dev = &gm12u320->dev; >> + >> + gm12u320->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev)); >> + if (!gm12u320->dmadev) >> + drm_warn(dev, "buffer sharing not supported"); /* not an error */ >> > > When implementing this in my own driver I discovered that this device > ref will leak if probing fails after this. > > I've done it like this: > > gdrm->dmadev = usb_intf_get_dma_device(intf); > if (!gdrm->dmadev) > dev_warn(dev, "buffer sharing not supported"); > > ret = drm_dev_register(drm, 0); > if (ret) { > put_device(gdrm->dmadev); > return ret; > } > > An even better solution would be to have a devm_ version of the function. > > Noralf. > >> INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work); >> mutex_init(&gm12u320->fb_update.lock); >> >> - dev = &gm12u320->dev; >> - >> ret = drmm_mode_config_init(dev); >> if (ret) >> return ret;
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 0631b9b323d5..fdfd6a1081ec 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -571,6 +571,27 @@ Contact: Daniel Vetter Level: Intermediate +Remove automatic page mapping from dma-buf importing +---------------------------------------------------- + +When importing dma-bufs, the dma-buf and PRIME frameworks automatically map +imported pages into the importer's DMA area. drm_gem_prime_fd_to_handle() and +drm_gem_prime_handle_to_fd() require that importers call dma_buf_attach() +even if they never do actual device DMA, but only CPU access through +dma_buf_vmap(). This is a problem for USB devices, which do not support DMA +operations. + +To fix the issue, automatic page mappings should be removed from the +buffer-sharing code. Fixing this is a bit more involved, since the import/export +cache is also tied to &drm_gem_object.import_attach. Meanwhile we paper over +this problem for USB devices by fishing out the USB host controller device, as +long as that supports DMA. Otherwise importing can still needlessly fail. + +Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter + +Level: Advanced + + Better Testing ============== diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c index 0b4f4f2af1ef..4fe372f43cf5 100644 --- a/drivers/gpu/drm/tiny/gm12u320.c +++ b/drivers/gpu/drm/tiny/gm12u320.c @@ -84,6 +84,7 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more silent)"); struct gm12u320_device { struct drm_device dev; + struct device *dmadev; struct drm_simple_display_pipe pipe; struct drm_connector conn; unsigned char *cmd_buf; @@ -599,6 +600,22 @@ static const uint64_t gm12u320_pipe_modifiers[] = { DRM_FORMAT_MOD_INVALID }; +/* + * FIXME: Dma-buf sharing requires DMA support by the importing device. + * This function is a workaround to make USB devices work as well. + * See todo.rst for how to fix the issue in the dma-buf framework. + */ +static struct drm_gem_object *gm12u320_gem_prime_import(struct drm_device *dev, + struct dma_buf *dma_buf) +{ + struct gm12u320_device *gm12u320 = to_gm12u320(dev); + + if (!gm12u320->dmadev) + return ERR_PTR(-ENODEV); + + return drm_gem_prime_import_dev(dev, dma_buf, gm12u320->dmadev); +} + DEFINE_DRM_GEM_FOPS(gm12u320_fops); static const struct drm_driver gm12u320_drm_driver = { @@ -612,6 +629,7 @@ static const struct drm_driver gm12u320_drm_driver = { .fops = &gm12u320_fops, DRM_GEM_SHMEM_DRIVER_OPS, + .gem_prime_import = gm12u320_gem_prime_import, }; static const struct drm_mode_config_funcs gm12u320_mode_config_funcs = { @@ -638,12 +656,15 @@ static int gm12u320_usb_probe(struct usb_interface *interface, struct gm12u320_device, dev); if (IS_ERR(gm12u320)) return PTR_ERR(gm12u320); + dev = &gm12u320->dev; + + gm12u320->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev)); + if (!gm12u320->dmadev) + drm_warn(dev, "buffer sharing not supported"); /* not an error */ INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work); mutex_init(&gm12u320->fb_update.lock); - dev = &gm12u320->dev; - ret = drmm_mode_config_init(dev); if (ret) return ret; @@ -691,7 +712,10 @@ static int gm12u320_usb_probe(struct usb_interface *interface, static void gm12u320_usb_disconnect(struct usb_interface *interface) { struct drm_device *dev = usb_get_intfdata(interface); + struct gm12u320_device *gm12u320 = to_gm12u320(dev); + put_device(gm12u320->dmadev); + gm12u320->dmadev = NULL; drm_dev_unplug(dev); drm_atomic_helper_shutdown(dev); } diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 9269092697d8..5703277c6f52 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -32,6 +32,22 @@ static int udl_usb_resume(struct usb_interface *interface) return drm_mode_config_helper_resume(dev); } +/* + * FIXME: Dma-buf sharing requires DMA support by the importing device. + * This function is a workaround to make USB devices work as well. + * See todo.rst for how to fix the issue in the dma-buf framework. + */ +static struct drm_gem_object *udl_driver_gem_prime_import(struct drm_device *dev, + struct dma_buf *dma_buf) +{ + struct udl_device *udl = to_udl(dev); + + if (!udl->dmadev) + return ERR_PTR(-ENODEV); + + return drm_gem_prime_import_dev(dev, dma_buf, udl->dmadev); +} + DEFINE_DRM_GEM_FOPS(udl_driver_fops); static const struct drm_driver driver = { @@ -40,6 +56,7 @@ static const struct drm_driver driver = { /* GEM hooks */ .fops = &udl_driver_fops, DRM_GEM_SHMEM_DRIVER_OPS, + .gem_prime_import = udl_driver_gem_prime_import, .name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 875e73551ae9..cc16a13316e4 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -50,6 +50,7 @@ struct urb_list { struct udl_device { struct drm_device drm; struct device *dev; + struct device *dmadev; struct drm_simple_display_pipe display_pipe; diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 0e2a376cb075..7c0338bcadea 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -315,6 +315,10 @@ int udl_init(struct udl_device *udl) DRM_DEBUG("\n"); + udl->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev)); + if (!udl->dmadev) + drm_warn(dev, "buffer sharing not supported"); /* not an error */ + mutex_init(&udl->gem_lock); if (!udl_parse_vendor_descriptor(udl)) { @@ -349,6 +353,11 @@ int udl_init(struct udl_device *udl) int udl_drop_usb(struct drm_device *dev) { + struct udl_device *udl = to_udl(dev); + udl_free_urb_list(dev); + put_device(udl->dmadev); + udl->dmadev = NULL; + return 0; } diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 8f07b0516100..a566bb494e24 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -748,6 +748,38 @@ void usb_put_intf(struct usb_interface *intf) } EXPORT_SYMBOL_GPL(usb_put_intf); +/** + * usb_intf_get_dma_device - acquire a reference on the usb interface's DMA endpoint + * @intf: the usb interface + * + * While a USB device cannot perform DMA operations by itself, many USB + * controllers can. A call to usb_intf_get_dma_device() returns the DMA endpoint + * for the given USB interface, if any. The returned device structure must be + * released with put_device(). + * + * See also usb_get_dma_device(). + * + * Returns: A reference to the usb interface's DMA endpoint; or NULL if none + * exists. + */ +struct device *usb_intf_get_dma_device(struct usb_interface *intf) +{ + struct usb_device *udev = interface_to_usbdev(intf); + struct device *dmadev; + + if (!udev->bus) + return NULL; + + dmadev = get_device(udev->bus->sysdev); + if (!dmadev || !dmadev->dma_mask) { + put_device(dmadev); + return NULL; + } + + return dmadev; +} +EXPORT_SYMBOL_GPL(usb_intf_get_dma_device); + /* USB device locking * * USB devices and interfaces are locked using the semaphore in their diff --git a/include/linux/usb.h b/include/linux/usb.h index 7d72c4e0713c..d6a41841b93e 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -746,6 +746,8 @@ extern int usb_lock_device_for_reset(struct usb_device *udev, extern int usb_reset_device(struct usb_device *dev); extern void usb_queue_reset_device(struct usb_interface *dev); +extern struct device *usb_intf_get_dma_device(struct usb_interface *intf); + #ifdef CONFIG_ACPI extern int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable);