[PATCH v3] drm: Use USB controller's DMA mask when importing dmabufs
Thomas Zimmermann
tzimmermann at suse.de
Tue Feb 23 12:51:09 UTC 2021
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 <gregkh at linuxfoundation.org> 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 <tzimmermann at suse.de>
>>>>>> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
>>>>>> Cc: Christoph Hellwig <hch at lst.de>
>>>>>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>>>>>> Cc: Johan Hovold <johan at kernel.org>
>>>>>> Cc: Alan Stern <stern at rowland.harvard.edu>
>>>>>> Cc: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
>>>>>> Cc: Sebastian Andrzej Siewior <bigeasy at linutronix.de>
>>>>>> Cc: Mathias Nyman <mathias.nyman at linux.intel.com>
>>>>>> Cc: Oliver Neukum <oneukum at suse.com>
>>>>>> Cc: Thomas Gleixner <tglx at linutronix.de>
>>>>>> Cc: <stable at vger.kernel.org> # 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
> dri-devel at lists.freedesktop.org
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210223/a4e77c11/attachment.sig>
More information about the dri-devel
mailing list