[PATCH 02/19] iosys-map: Add offset to iosys_map_memcpy_to()

[PATCH 02/19] iosys-map: Add offset to iosys_map_memcpy_to()

[Lucas De Marchi]

In certain situations it's useful to be able to write to an
offset of the mapping. Add a dst_offset to iosys_map_memcpy_to().

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/drm_cache.c     |  2 +-
 drivers/gpu/drm/drm_fb_helper.c |  2 +-
 include/linux/iosys-map.h       | 17 +++++++++--------
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 66597e411764..c3e6e615bf09 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -218,7 +218,7 @@ static void memcpy_fallback(struct iosys_map *dst,
 	if (!dst->is_iomem && !src->is_iomem) {
 		memcpy(dst->vaddr, src->vaddr, len);
 	} else if (!src->is_iomem) {
-		iosys_map_memcpy_to(dst, src->vaddr, len);
+		iosys_map_memcpy_to(dst, 0, src->vaddr, len);
 	} else if (!dst->is_iomem) {
 		memcpy_fromio(dst->vaddr, src->vaddr_iomem, len);
 	} else {
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 238f815cb2a0..bf5cc9a42e5a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -385,7 +385,7 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
 	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
 
 	for (y = clip->y1; y < clip->y2; y++) {
-		iosys_map_memcpy_to(dst, src, len);
+		iosys_map_memcpy_to(dst, 0, src, len);
 		iosys_map_incr(dst, fb->pitches[0]);
 		src += fb->pitches[0];
 	}
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index f4186f91caa6..edd7fa3be9e9 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -220,22 +220,23 @@ static inline void iosys_map_clear(struct iosys_map *map)
 }
 
 /**
- * iosys_map_memcpy_to - Memcpy into iosys mapping
+ * iosys_map_memcpy_to_offset - Memcpy into offset of iosys_map
  * @dst:	The iosys_map structure
+ * @dst_offset:	The offset from which to copy
  * @src:	The source buffer
  * @len:	The number of byte in src
  *
- * Copies data into a iosys mapping. The source buffer is in system
- * memory. Depending on the buffer's location, the helper picks the correct
- * method of accessing the memory.
+ * Copies data into a iosys_map with an offset. The source buffer is in
+ * system memory. Depending on the buffer's location, the helper picks the
+ * correct method of accessing the memory.
  */
-static inline void iosys_map_memcpy_to(struct iosys_map *dst, const void *src,
-				       size_t len)
+static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
+				       const void *src, size_t len)
 {
 	if (dst->is_iomem)
-		memcpy_toio(dst->vaddr_iomem, src, len);
+		memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
 	else
-		memcpy(dst->vaddr, src, len);
+		memcpy(dst->vaddr + dst_offset, src, len);
 }
 
 /**
-- 
2.35.1

Re: [PATCH 02/19] iosys-map: Add offset to iosys_map_memcpy_to()

[Christian König]

Am 04.02.22 um 18:44 schrieb Lucas De Marchi:
> In certain situations it's useful to be able to write to an
> offset of the mapping. Add a dst_offset to iosys_map_memcpy_to().
>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_cache.c     |  2 +-
>   drivers/gpu/drm/drm_fb_helper.c |  2 +-
>   include/linux/iosys-map.h       | 17 +++++++++--------
>   3 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
> index 66597e411764..c3e6e615bf09 100644
> --- a/drivers/gpu/drm/drm_cache.c
> +++ b/drivers/gpu/drm/drm_cache.c
> @@ -218,7 +218,7 @@ static void memcpy_fallback(struct iosys_map *dst,
>   	if (!dst->is_iomem && !src->is_iomem) {
>   		memcpy(dst->vaddr, src->vaddr, len);
>   	} else if (!src->is_iomem) {
> -		iosys_map_memcpy_to(dst, src->vaddr, len);
> +		iosys_map_memcpy_to(dst, 0, src->vaddr, len);
>   	} else if (!dst->is_iomem) {
>   		memcpy_fromio(dst->vaddr, src->vaddr_iomem, len);
>   	} else {
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 238f815cb2a0..bf5cc9a42e5a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -385,7 +385,7 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
>   	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
>   
>   	for (y = clip->y1; y < clip->y2; y++) {
> -		iosys_map_memcpy_to(dst, src, len);
> +		iosys_map_memcpy_to(dst, 0, src, len);
>   		iosys_map_incr(dst, fb->pitches[0]);
>   		src += fb->pitches[0];
>   	}
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index f4186f91caa6..edd7fa3be9e9 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -220,22 +220,23 @@ static inline void iosys_map_clear(struct iosys_map *map)
>   }
>   
>   /**
> - * iosys_map_memcpy_to - Memcpy into iosys mapping
> + * iosys_map_memcpy_to_offset - Memcpy into offset of iosys_map
>    * @dst:	The iosys_map structure
> + * @dst_offset:	The offset from which to copy
>    * @src:	The source buffer
>    * @len:	The number of byte in src
>    *
> - * Copies data into a iosys mapping. The source buffer is in system
> - * memory. Depending on the buffer's location, the helper picks the correct
> - * method of accessing the memory.
> + * Copies data into a iosys_map with an offset. The source buffer is in
> + * system memory. Depending on the buffer's location, the helper picks the
> + * correct method of accessing the memory.
>    */
> -static inline void iosys_map_memcpy_to(struct iosys_map *dst, const void *src,
> -				       size_t len)
> +static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
> +				       const void *src, size_t len)
>   {
>   	if (dst->is_iomem)
> -		memcpy_toio(dst->vaddr_iomem, src, len);
> +		memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
>   	else
> -		memcpy(dst->vaddr, src, len);
> +		memcpy(dst->vaddr + dst_offset, src, len);
>   }
>   
>   /**

Re: [PATCH 02/19] iosys-map: Add offset to iosys_map_memcpy_to()

[Thomas Zimmermann]

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

Hi

Am 04.02.22 um 18:44 schrieb Lucas De Marchi:
> In certain situations it's useful to be able to write to an
> offset of the mapping. Add a dst_offset to iosys_map_memcpy_to().
> 
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   drivers/gpu/drm/drm_cache.c     |  2 +-
>   drivers/gpu/drm/drm_fb_helper.c |  2 +-
>   include/linux/iosys-map.h       | 17 +++++++++--------
>   3 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
> index 66597e411764..c3e6e615bf09 100644
> --- a/drivers/gpu/drm/drm_cache.c
> +++ b/drivers/gpu/drm/drm_cache.c
> @@ -218,7 +218,7 @@ static void memcpy_fallback(struct iosys_map *dst,
>   	if (!dst->is_iomem && !src->is_iomem) {
>   		memcpy(dst->vaddr, src->vaddr, len);
>   	} else if (!src->is_iomem) {
> -		iosys_map_memcpy_to(dst, src->vaddr, len);
> +		iosys_map_memcpy_to(dst, 0, src->vaddr, len);
>   	} else if (!dst->is_iomem) {
>   		memcpy_fromio(dst->vaddr, src->vaddr_iomem, len);
>   	} else {
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 238f815cb2a0..bf5cc9a42e5a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -385,7 +385,7 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
>   	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
>   
>   	for (y = clip->y1; y < clip->y2; y++) {
> -		iosys_map_memcpy_to(dst, src, len);
> +		iosys_map_memcpy_to(dst, 0, src, len);
>   		iosys_map_incr(dst, fb->pitches[0]);
>   		src += fb->pitches[0];
>   	}
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> index f4186f91caa6..edd7fa3be9e9 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -220,22 +220,23 @@ static inline void iosys_map_clear(struct iosys_map *map)
>   }
>   
>   /**
> - * iosys_map_memcpy_to - Memcpy into iosys mapping
> + * iosys_map_memcpy_to_offset - Memcpy into offset of iosys_map

'iosys_map_memcpy_to'

With that fixed:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

>    * @dst:	The iosys_map structure
> + * @dst_offset:	The offset from which to copy
>    * @src:	The source buffer
>    * @len:	The number of byte in src
>    *
> - * Copies data into a iosys mapping. The source buffer is in system
> - * memory. Depending on the buffer's location, the helper picks the correct
> - * method of accessing the memory.
> + * Copies data into a iosys_map with an offset. The source buffer is in
> + * system memory. Depending on the buffer's location, the helper picks the
> + * correct method of accessing the memory.
>    */
> -static inline void iosys_map_memcpy_to(struct iosys_map *dst, const void *src,
> -				       size_t len)
> +static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset,
> +				       const void *src, size_t len)
>   {
>   	if (dst->is_iomem)
> -		memcpy_toio(dst->vaddr_iomem, src, len);
> +		memcpy_toio(dst->vaddr_iomem + dst_offset, src, len);
>   	else
> -		memcpy(dst->vaddr, src, len);
> +		memcpy(dst->vaddr + dst_offset, src, len);
>   }
>   
>   /**

-- 
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 --]

Re: [PATCH 02/19] iosys-map: Add offset to iosys_map_memcpy_to()

[Lucas De Marchi]

On Fri, Feb 04, 2022 at 07:48:10PM +0100, Thomas Zimmermann wrote:
>Hi
>
>Am 04.02.22 um 18:44 schrieb Lucas De Marchi:
>>In certain situations it's useful to be able to write to an
>>offset of the mapping. Add a dst_offset to iosys_map_memcpy_to().
>>
>>Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>Cc: Christian König <christian.koenig@amd.com>
>>Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>Cc: dri-devel@lists.freedesktop.org
>>Cc: linux-kernel@vger.kernel.org
>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>---
>>  drivers/gpu/drm/drm_cache.c     |  2 +-
>>  drivers/gpu/drm/drm_fb_helper.c |  2 +-
>>  include/linux/iosys-map.h       | 17 +++++++++--------
>>  3 files changed, 11 insertions(+), 10 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
>>index 66597e411764..c3e6e615bf09 100644
>>--- a/drivers/gpu/drm/drm_cache.c
>>+++ b/drivers/gpu/drm/drm_cache.c
>>@@ -218,7 +218,7 @@ static void memcpy_fallback(struct iosys_map *dst,
>>  	if (!dst->is_iomem && !src->is_iomem) {
>>  		memcpy(dst->vaddr, src->vaddr, len);
>>  	} else if (!src->is_iomem) {
>>-		iosys_map_memcpy_to(dst, src->vaddr, len);
>>+		iosys_map_memcpy_to(dst, 0, src->vaddr, len);
>>  	} else if (!dst->is_iomem) {
>>  		memcpy_fromio(dst->vaddr, src->vaddr_iomem, len);
>>  	} else {
>>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>index 238f815cb2a0..bf5cc9a42e5a 100644
>>--- a/drivers/gpu/drm/drm_fb_helper.c
>>+++ b/drivers/gpu/drm/drm_fb_helper.c
>>@@ -385,7 +385,7 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
>>  	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
>>  	for (y = clip->y1; y < clip->y2; y++) {
>>-		iosys_map_memcpy_to(dst, src, len);
>>+		iosys_map_memcpy_to(dst, 0, src, len);
>>  		iosys_map_incr(dst, fb->pitches[0]);
>>  		src += fb->pitches[0];
>>  	}
>>diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>>index f4186f91caa6..edd7fa3be9e9 100644
>>--- a/include/linux/iosys-map.h
>>+++ b/include/linux/iosys-map.h
>>@@ -220,22 +220,23 @@ static inline void iosys_map_clear(struct iosys_map *map)
>>  }
>>  /**
>>- * iosys_map_memcpy_to - Memcpy into iosys mapping
>>+ * iosys_map_memcpy_to_offset - Memcpy into offset of iosys_map
>
>'iosys_map_memcpy_to'
>
>With that fixed:
>
>Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

thanks, I noticed that, but looks like I squashed to the wrong patch.

Lucas De Marchi

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

[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@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 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,				\
+	}
+
 /**
  * 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
  * @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);
+}
+
+/**
+ * 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__) ({			\
+	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__) ({		\
+	struct_type__ *s;							\
+	iosys_map_wr(map__, offsetof(struct_type__, field__),			\
+		     typeof(s->field__), val__);				\
+})
+
 #endif /* __IOSYS_MAP_H__ */
-- 
2.35.1

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

[Thomas Zimmermann]

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

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@linaro.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   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.

However, you won't need the offset'ed iosys_map because the 
memcpy_to/from helpers now have the offset parameter.



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

>    * @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.

> +	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

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

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

[Lucas De Marchi]

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@linaro.org>
>>Cc: Christian König <christian.koenig@amd.com>
>>Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>Cc: dri-devel@lists.freedesktop.org
>>Cc: linux-kernel@vger.kernel.org
>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>---
>>  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

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

[Thomas Zimmermann]

[-- 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 --]