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?

Best regards
Thomas

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