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

[Christian König]

Am 29.09.20 um 11:14 schrieb Daniel Vetter:
> On Mon, Sep 28, 2020 at 01:22:13PM +0200, Christian König wrote:
>> Am 28.09.20 um 09:37 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 28.09.20 um 08:50 schrieb Christian König:
>>>> Am 27.09.20 um 21:16 schrieb Sam Ravnborg:
>>>>> Hi Thomas.
>>>>>
>>>>>>> struct simap {
>>>>>>>           union {
>>>>>>>                   void __iomem *vaddr_iomem;
>>>>>>>                   void *vaddr;
>>>>>>>           };
>>>>>>>           bool is_iomem;
>>>>>>> };
>>>>>>>
>>>>>>> Where simap is a shorthand for system_iomem_map
>>>>>>> And it could al be stuffed into a include/linux/simap.h file.
>>>>>>>
>>>>>>> Not totally sold on the simap name - but wanted to come up with
>>>>>>> something.
>>>>>> Yes. Others, myself included, have suggested to use a name that does not
>>>>>> imply a connection to the dma-buf framework, but no one has come up with
>>>>>>     a good name.
>>>>>>
>>>>>> I strongly dislike simap, as it's entirely non-obvious what it does.
>>>>>> dma-buf-map is not actually wrong. The structures represents the mapping
>>>>>> of a dma-able buffer in most cases.
>>>>>>
>>>>>>> With this approach users do not have to pull in dma-buf to use it and
>>>>>>> users will not confuse that this is only for dma-buf usage.
>>>>>> There's no need to enable dma-buf. It's all in the header file without
>>>>>> dependencies on dma-buf. It's really just the name.
>>>>>>
>>>>>> But there's something else to take into account. The whole issue here is
>>>>>> that the buffer is disconnected from its originating driver, so we don't
>>>>>> know which kind of memory ops we have to use. Thinking about it, I
>>>>>> realized that no one else seemed to have this problem until now.
>>>>>> Otherwise there would be a solution already. So maybe the dma-buf
>>>>>> framework *is* the native use case for this data structure.
>>>>> We have at least:
>>>>> linux/fb.h:
>>>>>       union {
>>>>>           char __iomem *screen_base;      /* Virtual address */
>>>>>           char *screen_buffer;
>>>>>       };
>>>>>
>>>>> Which solve more or less the same problem.
>>> I thought this was for convenience. The important is_iomem bit is missing.
>>>
>>>> I also already noted that in TTM we have exactly the same problem and a
>>>> whole bunch of helpers to allow operations on those pointers.
>>> How do you call this within TTM?
>> ttm_bus_placement, but I really don't like that name.
>>
>>> The data structure represents a pointer to either system or I/O memory,
>>> but not necessatrily device memory. It contains raw data. That would
>>> give something like
>>>
>>>     struct databuf_map
>>>     struct databuf_ptr
>>>     struct dbuf_map
>>>     struct dbuf_ptr
>>>
>>> My favorite would be dbuf_ptr. It's short and the API names would make
>>> sense: dbuf_ptr_clear() for clearing, dbuf_ptr_set_vaddr() to set an
>>> address, dbuf_ptr_incr() to increment, etc. Also, the _ptr indicates
>>> that it's a single address; not an offset with length.
>> Puh, no idea. All of that doesn't sound like it 100% hits the underlying
>> meaning of the structure.
> Imo first let's merge this and then second with more users we might come
> up with a better name. And cocci is fairly good at large-scale rename, to
> the point where we manged to rename struct fence to struct dma_fence.

Agreed, renaming things later on is easy if somebody comes up with 
something better.

But blocking a necessary technical change just because of the naming is 
usually not a good idea.

Christian.

>
> Also this entire thread here imo shows that we haven't yet figured out the
> perfect name anyway, and I don't think it's worth it to hold this up
> because of this bikeshed :-)
>
> Cheers, Daniel
>
>> Christian.
>>
>>> Best regards
>>> Thomas
>>>
>>>> Christian.
>>>>
>>>>>> Anyway, if a better name than dma-buf-map comes in, I'm willing to
>>>>>> rename the thing. Otherwise I intend to merge the patchset by the end of
>>>>>> the week.
>>>>> Well, the main thing is that I think this shoud be moved away from
>>>>> dma-buf. But if indeed dma-buf is the only relevant user in drm then
>>>>> I am totally fine with the current naming.
>>>>>
>>>>> One alternative named that popped up in my head: struct sys_io_map {}
>>>>> But again, if this is kept in dma-buf then I am fine with the current
>>>>> naming.
>>>>>
>>>>> In other words, if you continue to think this is mostly a dma-buf
>>>>> thing all three patches are:
>>>>> Acked-by: Sam Ravnborg <sam at ravnborg.org>
>>>>>
>>>>>       Sam
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7C71c630a7ca1d48476eed08d864581e4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637369676925032848&sdata=CsekzASvj2lY%2B74FIiLc9B5QG7AHma8B2M5y8Cassj4%3D&reserved=0