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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3817 bytes --]

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?

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.

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@ravnborg.org>
>>
>>     Sam
> 
> _______________________________________________
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]