Re: [Intel-gfx] [PATCH 03/19] iosys-map: Add a few more helpers

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 04.02.22 um 20:44 schrieb Lucas De Marchi:
[...]
> I only came up with such a macro after doing the rest of the patches and
> noticing a pattern that is hard to debug otherwise. I expanded the
> explanation in the doc above this macro.
> 
> Maybe something like:
> 
> #define IOSYS_MAP_INIT_OFFSET(map_, offset_) ({        \
>      struct iosys_map copy = *(map_);        \
>      iosys_map_incr(&copy, offset_);            \
>      copy;                        \
> })
> 
> Hopefully the compiler elides the additional copy, but I need to check.

I would accept this implementation of the macro. Don't worry about 
possible extra copies.

> 
> 
>>
>> However, you won't need the offset'ed iosys_map because the 
>> memcpy_to/from helpers now have the offset parameter.
> 
> I can't see how the offset would help. The idea is to use a shallow copy
> of the map so another function or even compilation unit can be
> designated to read/write part of the struct overlayed in the map... not
> even have knowledge of the outer struct.

I totally see your point. I still don't think it's something you should 
do. These functions don't operate on data types, but on raw memory that 
has to be unpacked into memory that has a data type assigned. Types are 
concepts of C, the I/O memory only knows reads and writes of different 
sizes.

> 
>>
>>
>>
>>> +
>>>  /**
>>>   * iosys_map_set_vaddr_iomem - Sets a iosys mapping structure to an 
>>> address in I/O memory
>>>   * @map:        The iosys_map structure
>>> @@ -220,7 +260,7 @@ static inline void iosys_map_clear(struct 
>>> iosys_map *map)
>>>  }
>>>  /**
>>> - * iosys_map_memcpy_to_offset - Memcpy into offset of iosys_map
>>> + * iosys_map_memcpy_to - Memcpy into iosys_map
>>
>> That's the fix for the other patch. :)
> 
> yep :-/
> 
>>
>>>   * @dst:    The iosys_map structure
>>>   * @dst_offset:    The offset from which to copy
>>>   * @src:    The source buffer
>>> @@ -239,6 +279,26 @@ static inline void iosys_map_memcpy_to(struct 
>>> iosys_map *dst, size_t dst_offset,
>>>          memcpy(dst->vaddr + dst_offset, src, len);
>>>  }
>>> +/**
>>> + * iosys_map_memcpy_from - Memcpy from iosys_map into system memory
>>> + * @dst:    Destination in system memory
>>> + * @src:    The iosys_map structure
>>> + * @src_offset:    The offset from which to copy
>>> + * @len:    The number of byte in src
>>> + *
>>> + * Copies data from a iosys_map with an offset. The dest buffer is in
>>> + * system memory. Depending on the mapping location, the helper 
>>> picks the
>>> + * correct method of accessing the memory.
>>> + */
>>> +static inline void iosys_map_memcpy_from(void *dst, const struct 
>>> iosys_map *src,
>>> +                     size_t src_offset, size_t len)
>>> +{
>>> +    if (src->is_iomem)
>>> +        memcpy_fromio(dst, src->vaddr_iomem + src_offset, len);
>>> +    else
>>> +        memcpy(dst, src->vaddr + src_offset, len);
>>> +}
>>> +
>>>  /**
>>>   * iosys_map_incr - Increments the address stored in a iosys mapping
>>>   * @map:    The iosys_map structure
>>> @@ -255,4 +315,96 @@ static inline void iosys_map_incr(struct 
>>> iosys_map *map, size_t incr)
>>>          map->vaddr += incr;
>>>  }
>>> +/**
>>> + * iosys_map_memset - Memset iosys_map
>>> + * @dst:    The iosys_map structure
>>> + * @offset:    Offset from dst where to start setting value
>>> + * @value:    The value to set
>>> + * @len:    The number of bytes to set in dst
>>> + *
>>> + * Set value in iosys_map. Depending on the buffer's location, the 
>>> helper
>>> + * picks the correct method of accessing the memory.
>>> + */
>>> +static inline void iosys_map_memset(struct iosys_map *dst, size_t 
>>> offset,
>>> +                    int value, size_t len)
>>> +{
>>> +    if (dst->is_iomem)
>>> +        memset_io(dst->vaddr_iomem + offset, value, len);
>>> +    else
>>> +        memset(dst->vaddr + offset, value, len);
>>> +}
>>
>> I've found that memset32() and memset64() can significantly faster. If 
>> ever needed, we can add variants here as well.
>>
>>> +
>>> +/**
>>> + * iosys_map_rd - Read a C-type value from the iosys_map
>>> + *
>>> + * @map__:    The iosys_map structure
>>> + * @offset__:    The offset from which to read
>>> + * @type__:    Type of the value being read
>>> + *
>>> + * Read a C type value from iosys_map, handling possible un-aligned 
>>> accesses to
>>> + * the mapping.
>>> + *
>>> + * Returns:
>>> + * The value read from the mapping.
>>> + */
>>> +#define iosys_map_rd(map__, offset__, type__) ({            \
>>> +    type__ val;                            \
>>> +    iosys_map_memcpy_from(&val, map__, offset__, sizeof(val));    \
>>> +    val;                                \
>>> +})
>>> +
>>> +/**
>>> + * iosys_map_wr - Write a C-type value to the iosys_map
>>> + *
>>> + * @map__:    The iosys_map structure
>>> + * @offset__:    The offset from the mapping to write to
>>> + * @type__:    Type of the value being written
>>> + * @val__:    Value to write
>>> + *
>>> + * Write a C-type value to the iosys_map, handling possible 
>>> un-aligned accesses
>>> + * to the mapping.
>>> + */
>>> +#define iosys_map_wr(map__, offset__, type__, val__) ({            \
>>> +    type__ val = (val__);                        \
>>> +    iosys_map_memcpy_to(map__, offset__, &val, sizeof(val));    \
>>> +})
>>> +
>>> +/**
>>> + * iosys_map_rd_field - Read a member from a struct in the iosys_map
>>> + *
>>> + * @map__:        The iosys_map structure
>>> + * @struct_type__:    The struct describing the layout of the mapping
>>> + * @field__:        Member of the struct to read
>>> + *
>>> + * Read a value from iosys_map assuming its layout is described by a 
>>> struct,
>>> + * passed as argument. The offset and size to the struct member is 
>>> calculated
>>> + * and possible un-aligned accesses to the mapping handled.
>>> + *
>>> + * Returns:
>>> + * The value read from the mapping.
>>> + */
>>> +#define iosys_map_rd_field(map__, struct_type__, field__) 
>>> ({            \
>>
>> This macro should also have an offset__ parameter and forward it to 
>> iosys_map_rd.
> 
> offset is actually this macro helps calculating:
> 
> struct foo {
>      struct bla { ... };
>      struct bla2 { ... };
>      int something_else;
> };
> 
> 
> iosys_map_rd_field(&map, struct foo, bla.x);
> 
> I feel an offset to the map, where struct foo would be located, would be
> redundant if you delegated a function to update, say, struct bla and
> that part alone.

It's not redundant, it's easy to add, consistent with the overall 
interface and the next user of this code might have use for it. And as I 
said above, you're not operating on struct types here. It's raw memory 
that has to be packed/unpacked. The wr and rd macros are bending the 
rules already. Using them carelessly can make the code behave 
differently on different types of memory.

Anyway, please add the offset parameter to these macros.

Best regards
Thomas

> 
> This pattern happens in patch "drm/i915/guc: Convert engine record to
> iosys_map" if it helps as an example.
> 
> 
> thanks
> Lucas De Marchi
> 
>>
>>> +    struct_type__ *s;                            \
>>> +    iosys_map_rd(map__, offsetof(struct_type__, field__),            \
>>> +             typeof(s->field__));                    \
>>> +})
>>> +
>>> +/**
>>> + * iosys_map_wr_field - Write to a member of a struct in the iosys_map
>>> + *
>>> + * @map__:        The iosys_map structure
>>> + * @struct_type__:    The struct describing the layout of the mapping
>>> + * @field__:        Member of the struct to read
>>> + * @val__:        Value to write
>>> + *
>>> + * Write a value to the iosys_map assuming its layout is described 
>>> by a struct,
>>> + * passed as argument. The offset and size to the struct member is 
>>> calculated
>>> + * and possible un-aligned accesses to the mapping handled.
>>> + */
>>> +#define iosys_map_wr_field(map__, struct_type__, field__, val__) 
>>> ({        \
>>
>> And this one should also have an offset__ parameter.
>>
>> Best regards
>> Thomas
>>
>>> +    struct_type__ *s;                            \
>>> +    iosys_map_wr(map__, offsetof(struct_type__, field__),            \
>>> +             typeof(s->field__), val__);                \
>>> +})
>>> +
>>>  #endif /* __IOSYS_MAP_H__ */
>>
>> -- 
>> 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 --]