Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 22, 2021 at 05:25:46PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 22.02.21 um 17:10 schrieb Daniel Vetter:
> > On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > 
> > > Hi
> > > 
> > > Am 22.02.21 um 14:09 schrieb Christian König:
> > > > 
> > > > 
> > > > Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> > > > > USB-based drivers cannot use DMA, so the importing of dma-buf attachments
> > > > > currently fails for udl and gm12u320. This breaks joining/mirroring of
> > > > > displays.
> > > > > 
> > > > > The fix is now a little series. To solve the issue on the importer
> > > > > side (i.e., the affected USB-based driver), patch 1 introduces a new
> > > > > PRIME callback, struct drm_driver.gem_prime_create_object, which creates
> > > > > an object and gives more control to the importing driver. Specifically,
> > > > > udl and gm12u320 can now avoid the creation of a scatter/gather table
> > > > > for the imported pages. Patch 1 is self-contained in the sense that it
> > > > > can be backported into older kernels.
> > > > 
> > > > Mhm, that sounds like a little overkill to me.
> > > > 
> > > > Drivers can already import the DMA-bufs all by them selves without the
> > > > help of the DRM functions. See amdgpu for an example.
> > > > 
> > > > Daniel also already noted to me that he sees the DRM helper as a bit
> > > > questionable middle layer.
> > > 
> > > And this bug proves that it is. :)
> > 
> > The trouble here is actually gem_bo->import_attach, which isn't really
> > part of the questionable midlayer, but fairly mandatory (only
> > exception is vmwgfx because not using gem) caching to make sure we
> > don't end up with duped imports and fun stuff like that.
> > 
> > And dma_buf_attach now implicitly creates the sg table already, so
> > we're already in game over land. I think we'd need to make
> > import_attach a union with import_buf or something like that, so that
> > you can do attachment-less importing.
> 
> Creating the sg table is not the problem; mapping it is. So dma_buf_attach
> shouldn't be a problem.

dma_buf_attach will create a cached sg-mapping for you if the exporter is
dynamic. Currently that's only the case for amdgpu, I guess you didn't
test with that.

So yeah dma_buf_attach is a problem already. And if we can't attach, the
entire obj->import_attach logic in drm_prime.c falls over, and we get all
kinds of fun with double import and re-export.

> > > > Have you thought about doing that instead?
> > > 
> > > There appears to be some useful code in drm_gem_prime_import_dev(). But
> > > if the general sentiment goes towards removing
> > > gem_prime_import_sg_table, we can work towards that as well.
> > 
> > I still think this part is a bit a silly midlayer for no good reason,
> > but I think that's orthogonal to the issue at hand here.
> > 
> > I'd suggest we first try to paper over the issue by using
> > prime_import_dev with the host controller (which hopefully is
> > dma-capable for most systems). And then, at leisure, try to untangle
> > the obj->import_attach issue.
> 
> I really don't want to do this. My time is also limited, and I''ll spend
> time papering over the thing. And then more time for the real fix. I'd
> rather pull drm_gem_prime_import_dev() in to USB drivers and avoid the
> dma_buf_map().

Yeah I understand, it's just (as usual :-/) more complex than it seems ...
-Daniel

> 
> Best regard
> Thomas
> 
> > -Daniel
> > 
> > > 
> > > Best regards
> > > Thomas
> > > 
> > > > 
> > > > Christian.
> > > > 
> > > > > 
> > > > > Patches 2 and 3 update SHMEM and CMA helpers to use the new callback.
> > > > > Effectively this moves the sg table setup from the PRIME helpers into
> > > > > the memory managers. SHMEM now supports devices without DMA support,
> > > > > so custom code can be removed from udl and g12u320.
> > > > > 
> > > > > Tested by joining/mirroring displays of udl and radeon under Gnome/X11.
> > > > > 
> > > > > v2:
> > > > >      * move fix to importer side (Christian, Daniel)
> > > > >      * update SHMEM and CMA helpers for new PRIME callbacks
> > > > > 
> > > > > Thomas Zimmermann (3):
> > > > >     drm: Support importing dmabufs into drivers without DMA
> > > > >     drm/shmem-helper: Implement struct drm_driver.gem_prime_create_object
> > > > >     drm/cma-helper: Implement struct drm_driver.gem_prime_create_object
> > > > > 
> > > > >    drivers/gpu/drm/drm_gem_cma_helper.c    | 62 ++++++++++++++-----------
> > > > >    drivers/gpu/drm/drm_gem_shmem_helper.c  | 38 ++++++++++-----
> > > > >    drivers/gpu/drm/drm_prime.c             | 43 +++++++++++------
> > > > >    drivers/gpu/drm/lima/lima_drv.c         |  2 +-
> > > > >    drivers/gpu/drm/panfrost/panfrost_drv.c |  2 +-
> > > > >    drivers/gpu/drm/panfrost/panfrost_gem.c |  6 +--
> > > > >    drivers/gpu/drm/panfrost/panfrost_gem.h |  4 +-
> > > > >    drivers/gpu/drm/pl111/pl111_drv.c       |  8 ++--
> > > > >    drivers/gpu/drm/v3d/v3d_bo.c            |  6 +--
> > > > >    drivers/gpu/drm/v3d/v3d_drv.c           |  2 +-
> > > > >    drivers/gpu/drm/v3d/v3d_drv.h           |  5 +-
> > > > >    include/drm/drm_drv.h                   | 12 +++++
> > > > >    include/drm/drm_gem_cma_helper.h        | 12 ++---
> > > > >    include/drm/drm_gem_shmem_helper.h      |  6 +--
> > > > >    14 files changed, 120 insertions(+), 88 deletions(-)
> > > > > 
> > > > > --
> > > > > 2.30.1
> > > > > 
> > > > 
> > > 
> > > --
> > > 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
> > > 
> > 
> > 
> 
> -- 
> 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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch