Re: [Intel-gfx] [PATCH 0/3] dma-buf: Flag vmap'ed memory as system or I/O memory

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 7539 bytes --]

Hi

Am 16.09.20 um 14:59 schrieb Christian König:
> Am 16.09.20 um 14:24 schrieb Daniel Vetter:
>> On Wed, Sep 16, 2020 at 12:48:20PM +0200, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 16.09.20 um 11:37 schrieb Daniel Vetter:
>>>> On Mon, Sep 14, 2020 at 01:25:18PM +0200, Thomas Zimmermann wrote:
>>>>> Dma-buf provides vmap() and vunmap() for retrieving and releasing
>>>>> mappings
>>>>> of dma-buf memory in kernel address space. The functions operate
>>>>> with plain
>>>>> addresses and the assumption is that the memory can be accessed
>>>>> with load
>>>>> and store operations. This is not the case on some architectures
>>>>> (e.g.,
>>>>> sparc64) where I/O memory can only be accessed with dedicated
>>>>> instructions.
>>>>>
>>>>> This patchset introduces struct dma_buf_map, which contains the
>>>>> address of
>>>>> a buffer and a flag that tells whether system- or I/O-memory
>>>>> instructions
>>>>> are required.
>>>>>
>>>>> Some background: updating the DRM framebuffer console on sparc64
>>>>> makes the
>>>>> kernel panic. This is because the framebuffer memory cannot be
>>>>> accessed with
>>>>> system-memory instructions. We currently employ a workaround in DRM to
>>>>> address this specific problem. [1]
>>>>>
>>>>> To resolve the problem, we'd like to address it at the most common
>>>>> point,
>>>>> which is the dma-buf framework. The dma-buf mapping ideally knows
>>>>> if I/O
>>>>> instructions are required and exports this information to it's
>>>>> users. The
>>>>> new structure struct dma_buf_map stores the buffer address and a
>>>>> flag that
>>>>> signals I/O memory. Affected users of the buffer (e.g., drivers,
>>>>> frameworks)
>>>>> can then access the memory accordingly.
>>>>>
>>>>> This patchset only introduces struct dma_buf_map, and updates
>>>>> struct dma_buf
>>>>> and it's interfaces. Further patches can update dma-buf users. For
>>>>> example,
>>>>> there's a prototype patchset for DRM that fixes the framebuffer
>>>>> problem. [2]
>>>>>
>>>>> Further work: TTM, one of DRM's memory managers, already exports an
>>>>> is_iomem flag of its own. It could later be switched over to
>>>>> exporting struct
>>>>> dma_buf_map, thus simplifying some code. Several DRM drivers expect
>>>>> their
>>>>> fbdev console to operate on I/O memory. These could possibly be
>>>>> switched over
>>>>> to the generic fbdev emulation, as soon as the generic code uses
>>>>> struct
>>>>> dma_buf_map.
>>>>>
>>>>> [1]
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200725191012.GA434957%40ravnborg.org%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=wTmFuB95GhKUU%2F2Q91V0%2BtzAu4%2BEe3VBUcriBy3jx2g%3D&reserved=0
>>>>>
>>>>> [2]
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20200806085239.4606-1-tzimmermann%40suse.de%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C04e3cc3e03ae40f1fa0f08d85a3b6a68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637358558524732385&sdata=L4rBHmegO63b%2FiTQdTyH158KNxAZwSuJCQOaFszo5L0%3D&reserved=0
>>>>>
>>>> lgtm, imo ready to convert the follow-up patches over to this. But I
>>>> think
>>>> would be good to get at least some ack from the ttm side for the
>>>> overall
>>>> plan.
>>> Yup, it would be nice if TTM could had out these types automatically.
>>> Then all TTM-based drivers would automatically support it.
>>>
>>>> Also, I think we should put all the various helpers (writel/readl,
>>>> memset,
>>>> memcpy, whatever else) into the dma-buf-map.h helper, so that most code
>>>> using this can just treat it as an abstract pointer type and never look
>>>> underneath it.
>>> We have some framebuffer helpers that rely on pointer arithmetic, so
>>> we'd need that too. No big deal wrt code, but I was worried about the
>>> overhead. If a loop goes over framebuffer memory, there's an if/else
>>> branch for each access to the memory buffer.
>> If we make all the helpers static inline, then the compiler should be
>> able
>> to see that dma_buf_map.is_iomem is always the same, and produced really
>> optimized code for it by pulling that check out from all the loops.
>>
>> So should only result in somewhat verbose code of having to call
>> dma_buf_map pointer arthimetic helpers, but not in bad generated code.
>> Still worth double-checking I think, since e.g. on x86 the generated code
>> should be the same for both cases (but maybe the compiler doesn't see
>> through the inline asm to realize that, so we might end up with 2
>> copies).
> 
> Can we have that even independent of DMA-buf? We have essentially the
> same problem in TTM and the code around that is a complete mess if you
> ask me.

I already put this into dma-buf because it's at the intersection of all
the affected modules. For non-dma-buf pointers (say in framebuffer
damage handling), the idea is to initialize struct dma_buf_map by hand
and use this.

Where would you want to put it?

Best regards
Thomas

> 
> Christian.
> 
>> -Daniel
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>> -Daniel
>>>>
>>>>> Thomas Zimmermann (3):
>>>>>    dma-buf: Add struct dma-buf-map for storing struct
>>>>> dma_buf.vaddr_ptr
>>>>>    dma-buf: Use struct dma_buf_map in dma_buf_vmap() interfaces
>>>>>    dma-buf: Use struct dma_buf_map in dma_buf_vunmap() interfaces
>>>>>
>>>>>   Documentation/driver-api/dma-buf.rst          |   3 +
>>>>>   drivers/dma-buf/dma-buf.c                     |  40 +++---
>>>>>   drivers/gpu/drm/drm_gem_cma_helper.c          |  16 ++-
>>>>>   drivers/gpu/drm/drm_gem_shmem_helper.c        |  17 ++-
>>>>>   drivers/gpu/drm/drm_prime.c                   |  14 +-
>>>>>   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  13 +-
>>>>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  13 +-
>>>>>   .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  18 ++-
>>>>>   drivers/gpu/drm/tegra/gem.c                   |  23 ++--
>>>>>   .../common/videobuf2/videobuf2-dma-contig.c   |  17 ++-
>>>>>   .../media/common/videobuf2/videobuf2-dma-sg.c |  19 ++-
>>>>>   .../common/videobuf2/videobuf2-vmalloc.c      |  21 ++-
>>>>>   include/drm/drm_prime.h                       |   5 +-
>>>>>   include/linux/dma-buf-map.h                   | 126
>>>>> ++++++++++++++++++
>>>>>   include/linux/dma-buf.h                       |  11 +-
>>>>>   15 files changed, 274 insertions(+), 82 deletions(-)
>>>>>   create mode 100644 include/linux/dma-buf-map.h
>>>>>
>>>>> -- 
>>>>> 2.28.0
>>>>>
>>> -- 
>>> 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
>>>
>>
>>
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx