| Message ID | 20210222124328.27340-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm/prime: Only call dma_map_sgtable() for devices with DMA support | expand |
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 >> > > > >