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