Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 27.01.22 um 16:59 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 03:33:12PM +0100, Thomas Zimmermann wrote:
>>
>>
>> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>> When dma_buf_map struct is passed around, it's useful to be able to
>>> initialize a second map that takes care of reading/writing to an offset
>>> of the original map.
>>>
>>> Add a helper that copies the struct and add the offset to the proper
>>> address.
>>>
>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: linux-media@vger.kernel.org
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: linaro-mm-sig@lists.linaro.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>>  include/linux/dma-buf-map.h | 29 +++++++++++++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>>> index 65e927d9ce33..3514a859f628 100644
>>> --- a/include/linux/dma-buf-map.h
>>> +++ b/include/linux/dma-buf-map.h
>>> @@ -131,6 +131,35 @@ struct dma_buf_map {
>>>          .is_iomem = false, \
>>>      }
>>> +/**
>>> + * DMA_BUF_MAP_INIT_OFFSET - Initializes struct dma_buf_map from 
>>> another dma_buf_map
>>> + * @map_:    The dma-buf mapping structure to copy from
>>> + * @offset:    Offset to add to the other mapping
>>> + *
>>> + * Initializes a new dma_buf_struct based on another. This is the 
>>> equivalent of doing:
>>> + *
>>> + * .. code-block: c
>>> + *
>>> + *    dma_buf_map map = other_map;
>>> + *    dma_buf_map_incr(&map, &offset);
>>> + *
>>> + * Example usage:
>>> + *
>>> + * .. code-block: c
>>> + *
>>> + *    void foo(struct device *dev, struct dma_buf_map *base_map)
>>> + *    {
>>> + *        ...
>>> + *        struct dma_buf_map = DMA_BUF_MAP_INIT_OFFSET(base_map, 
>>> FIELD_OFFSET);
>>> + *        ...
>>> + *    }
>>> + */
>>> +#define DMA_BUF_MAP_INIT_OFFSET(map_, offset_)    (struct 
>>> dma_buf_map)    \
>>> +    {                                \
>>> +        .vaddr = (map_)->vaddr + (offset_),            \
>>> +        .is_iomem = (map_)->is_iomem,                \
>>> +    }
>>> +
>>
>> It's illegal to access .vaddr  with raw pointer. Always use a 
>> dma_buf_memcpy_() interface. So why would you need this macro when you 
>> have dma_buf_memcpy_*() with an offset parameter?
> 
> I did a better job with an example in 
> 20220127093332.wnkd2qy4tvwg5i5l@ldmartin-desk2
> 
> While doing this series I had code like this when using the API in a 
> function to
> parse/update part of the struct mapped:
> 
>      int bla_parse_foo(struct dma_buf_map *bla_map)
>      {
>          struct dma_buf_map foo_map = *bla_map;
>          ...
> 
>          dma_buf_map_incr(&foo_map, offsetof(struct bla, foo));
> 
>          ...
>      }
> 
> Pasting the rest of the reply here:
> 
> I had exactly this code above, but after writting quite a few patches
> using it, particularly with functions that have to write to 2 maps (see
> patch 6 for example), it felt much better to have something to
> initialize correctly from the start
> 
>          struct dma_buf_map other_map = *bla_map;
>          /* poor Lucas forgetting dma_buf_map_incr(map, offsetof(...)); */
> 
> is error prone and hard to debug since you will be reading/writting
> from/to another location rather than exploding

Indeed. We have soem very specific use cases in graphics code, when 
dma_buf_map_incr() makes sense. But it's really bad for others. I guess 
that the docs should talk about this.

> 
> While with the construct below
> 
>          other_map;
>          ...
>          other_map = INITIALIZER()
> 
> I can rely on the compiler complaining about uninitialized var. And
> in most of the cases I can just have this single line in the beggining 
> of the
> function when the offset is constant:
> 
>          struct dma_buf_map other_map = INITIALIZER(bla_map, offsetof(..));
> 
> 
> This is useful when you have several small functions in charge of
> updating/reading inner struct members.

You won't need an extra variable or the initializer macro if you add an 
offset parameter to dma_buf_memcpy_{from,to}.  Simple pass offsetof(..) 
to that parameter and it will do the right thing.

It avoids the problems of the current macro and is even more flexible. 
On top of that, you can build whatever convenience macros you need for i915.

Best regards
Thomas

> 
>>
>> I've also been very careful to distinguish between .vaddr and 
>> .vaddr_iomem, even in places where I wouldn't have to. This macro 
>> breaks the assumption.
> 
> That's one reason I think if we have this macro, it should be in the
> dma_buf_map.h header (or whatever we rename these APIs to). It's the
> only place where we can safely add code that relies on the implementation
> of the "private" fields in struct dma_buf_map.
> 
> Lucas De Marchi
> 
>>
>> Best regards
>> Thomas
>>
>>>  /**
>>>   * dma_buf_map_set_vaddr - Sets a dma-buf mapping structure to an 
>>> address in system memory
>>>   * @map:    The dma-buf mapping structure
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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