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

Tue Feb 23 08:19:31 UTC 2021

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

-- 
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/lima/attachments/20210223/9381dbd1/attachment.sig>