mbox series

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

Message ID 20210222124328.27340-1-tzimmermann@suse.de (mailing list archive)
Headers show
Series drm/prime: Only call dma_map_sgtable() for devices with DMA support | expand

Message

Thomas Zimmermann Feb. 22, 2021, 12:43 p.m. UTC
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.

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

Comments

Christian König Feb. 22, 2021, 1:09 p.m. UTC | #1
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.

Have you thought about doing that instead?

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 Feb. 22, 2021, 1:24 p.m. UTC | #2
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. :)

> 
> 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.

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
>>
>
Daniel Vetter Feb. 22, 2021, 4:10 p.m. UTC | #3
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.

> > 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.
-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 Feb. 22, 2021, 4:25 p.m. UTC | #4
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.

> 
>>> 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().

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
>>
> 
>
Daniel Vetter Feb. 22, 2021, 4:34 p.m. UTC | #5
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
>
Thomas Zimmermann Feb. 23, 2021, 8:19 a.m. UTC | #6
Hi

Am 22.02.21 um 17:34 schrieb Daniel Vetter:
> 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.

OK, I give up. I'll send out the patch with the usb controller later today.

Best regards
Thomas

> 
>>>>> 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
>>
> 
> 
> 
>