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

Tue Sep 29 11:14:21 UTC 2020

Am 29.09.20 um 13:09 schrieb 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.

OK, merged now.

Best regards
Thomas

> 
> 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
>>>>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20200929/904d60c4/attachment.sig>