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

["Christian König" <christian.koenig@amd.com>]

Am 17.09.20 um 09:16 schrieb Thomas Zimmermann:
> Hi Christian and Thomas
>
> Am 16.09.20 um 15:37 schrieb Thomas Hellström (Intel):
>> On 9/16/20 2:59 PM, Christian König wrote:
>>> 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.
>>>
>>> Christian.
>>>
>> I think this patchset looks good. Changing ttm_bo_kmap() over to
>> returning a struct dma-buf-map would probably work just fine. If we then
>> can have a set of helpers to operate on it, that's great.
>>
>> /Thomas
> Can I count this as an A-b by one of you?

For the the general approach, certainly yes.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel