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

From: Lucas De Marchi
Date: Fri Feb 04 2022 - 14:45:00 EST

Next message: Dmitry Osipenko: "Re: [PATCH 6/6] dt-bindings: memory: lpddr2-timings: convert to dtschema"
Previous message: Dmitry Osipenko: "Re: [PATCH 6/6] dt-bindings: memory: lpddr2-timings: convert to dtschema"
In reply to: Thomas Zimmermann: "Re: [PATCH 03/19] iosys-map: Add a few more helpers"
Next in thread: Thomas Zimmermann: "Re: [PATCH 03/19] iosys-map: Add a few more helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Feb 04, 2022 at 08:05:56PM +0100, Thomas Zimmermann wrote:

Hi

Am 04.02.22 um 18:44 schrieb Lucas De Marchi:

First the simplest ones:

- iosys_map_memset(): when abstracting system and I/O memory,
just like the memcpy() use case, memset() also has dedicated
functions to be called for using IO memory.
- iosys_map_memcpy_from(): we may need to copy data from I/O
memory, not only to.

In certain situations it's useful to be able to read or write to an
offset that is calculated by having the memory layout given by a struct
declaration. Usually we are going to read/write a u8, u16, u32 or u64.

As a pre-requisite for the implementation, add iosys_map_memcpy_from()
to be the equivalent of iosys_map_memcpy_to(), but in the other
direction. Then add 2 pairs of macros:

- iosys_map_rd() / iosys_map_wr()
- iosys_map_rd_field() / iosys_map_wr_field()

The first pair takes the C-type and offset to read/write. The second
pair uses a struct describing the layout of the mapping in order to
calculate the offset and size being read/written.

We could use readb, readw, readl, readq and the write* counterparts,
however due to alignment issues this may not work on all architectures.
If alignment needs to be checked to call the right function, it's not
possible to decide at compile-time which function to call: so just leave
the decision to the memcpy function that will do exactly that.

Finally, in order to use the above macros with a map derived from
another, add another initializer: IOSYS_MAP_INIT_OFFSET().

Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
---
include/linux/iosys-map.h | 154 +++++++++++++++++++++++++++++++++++++-
1 file changed, 153 insertions(+), 1 deletion(-)

diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index edd7fa3be9e9..96f8b61ac6fb 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -6,6 +6,7 @@
#ifndef __IOSYS_MAP_H__
#define __IOSYS_MAP_H__
+#include <linux/kernel.h>
#include <linux/io.h>
#include <linux/string.h>
@@ -133,6 +134,45 @@ static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
map->is_iomem = false;
}
+/**
+ * IOSYS_MAP_INIT_OFFSET - Initializes struct iosys_map from another iosys_map
+ * @map_: The dma-buf mapping structure to copy from
+ * @offset_: Offset to add to the other mapping
+ *
+ * Initializes a new iosys_map struct based on another passed as argument. It
+ * does a shallow copy of the struct so it's possible to update the back storage
+ * without changing where the original map points to. It is the equivalent of
+ * doing:
+ *
+ * .. code-block: c
+ *
+ * iosys_map map = other_map;
+ * iosys_map_incr(&map, &offset);
+ *
+ * Example usage:
+ *
+ * .. code-block: c
+ *
+ * void foo(struct device *dev, struct iosys_map *base_map)
+ * {
+ * ...
+ * struct iosys_map map = IOSYS_MAP_INIT_OFFSET(base_map, FIELD_OFFSET);
+ * ...
+ * }
+ *
+ * The advantage of using the initializer over just increasing the offset with
+ * ``iosys_map_incr()`` like above is that the new map will always point to the
+ * right place of the buffer during its scope. It reduces the risk of updating
+ * the wrong part of the buffer and having no compiler warning about that. If
+ * the assignment to IOSYS_MAP_INIT_OFFSET() is forgotten, the compiler can warn
+ * using a uninitialized variable.
+ */
+#define IOSYS_MAP_INIT_OFFSET(map_, offset_) (struct iosys_map) \
+ { \
+ .vaddr = (map_)->vaddr + (offset_), \
+ .is_iomem = (map_)->is_iomem, \
+ }

I already nak'ed this macro. This works because of the aliasing rules within the union and the fact that there are only plain pointers. But this is fragile. Do anything more complicated and it breaks. There's not even a test that would tell you that it failed.

Therefore, struct iosys_map should only be initialized by the code that creates the stored pointer.

I wonder if there is an alternative that is not fragile that allows us
to have a macro like that. In that thread I and Daniel continued
chatting and after my additional explanations he was convinced about
that.

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.

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.

+
/**
* 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.

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

Next message: Dmitry Osipenko: "Re: [PATCH 6/6] dt-bindings: memory: lpddr2-timings: convert to dtschema"
Previous message: Dmitry Osipenko: "Re: [PATCH 6/6] dt-bindings: memory: lpddr2-timings: convert to dtschema"
In reply to: Thomas Zimmermann: "Re: [PATCH 03/19] iosys-map: Add a few more helpers"
Next in thread: Thomas Zimmermann: "Re: [PATCH 03/19] iosys-map: Add a few more helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]