From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78C94C433DB for ; Tue, 23 Feb 2021 12:05:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 52D6F64DE8 for ; Tue, 23 Feb 2021 12:05:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232564AbhBWMFa (ORCPT ); Tue, 23 Feb 2021 07:05:30 -0500 Received: from mail.kernel.org ([198.145.29.99]:40800 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232902AbhBWMD1 (ORCPT ); Tue, 23 Feb 2021 07:03:27 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id A8B6A61481; Tue, 23 Feb 2021 12:02:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1614081765; bh=0Zt1cKt1K7xf/htjgE0vulHmO/nRk4RAiLjk1zOipPw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=oAypFQfY3N+T56ZJZHnSp/V5cXlRmQHKlCor4CseuG3Rl87MNm/yAMOv6jPLv0YSS Aa4UHpFFyVrKeMrWGFgY/e9pdNxTo2AcBYg9doKA1hNEM1cWuGwup/DAHByAoSbNur 37sA/yNuFdQGQN0IAXZpdO/Vz1U+oPeRChJ5JJY8= Date: Tue, 23 Feb 2021 13:02:42 +0100 From: Greg KH To: Daniel Vetter Cc: Thomas Zimmermann , airlied@linux.ie, maarten.lankhorst@linux.intel.com, mripard@kernel.org, sumit.semwal@linaro.org, christian.koenig@amd.com, hdegoede@redhat.com, sean@poorly.run, noralf@tronnes.org, dri-devel@lists.freedesktop.org, Christoph Hellwig , Johan Hovold , Alan Stern , Andy Shevchenko , Sebastian Andrzej Siewior , Mathias Nyman , Oliver Neukum , Thomas Gleixner , stable@vger.kernel.org Subject: Re: [PATCH v3] drm: Use USB controller's DMA mask when importing dmabufs Message-ID: References: <20210223105842.27011-1-tzimmermann@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org 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 > > > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices") > > > Cc: Christoph Hellwig > > > Cc: Greg Kroah-Hartman > > > Cc: Johan Hovold > > > Cc: Alan Stern > > > Cc: Andy Shevchenko > > > Cc: Sebastian Andrzej Siewior > > > Cc: Mathias Nyman > > > Cc: Oliver Neukum > > > Cc: Thomas Gleixner > > > Cc: # 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 > > > #include > > > #include > > > +#include > > > > > > #include > > > #include > > > @@ -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