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(©, 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_mapThat'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:
OpenPGP_signature
Description: OpenPGP digital signature
