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

Wed Sep 16 13:37:57 UTC 2020

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