[PATCH 00/19] drm/i915/guc: Refactor ADS access to use dma_buf_map

[PATCH 00/19] drm/i915/guc: Refactor ADS access to use dma_buf_map

[Lucas De Marchi]

While porting i915 to arm64 we noticed some issues accessing lmem.
Some writes were getting corrupted and the final state of the buffer
didn't have exactly what we wrote. This became evident when enabling
GuC submission: depending on the number of engines the ADS struct was
being corrupted and GuC would reject it, refusin to initialize.

From Documentation/core-api/bus-virt-phys-mapping.rst:

	This memory is called "PCI memory" or "shared memory" or "IO memory" or
	whatever, and there is only one way to access it: the readb/writeb and
	related functions. You should never take the address of such memory, because
	there is really nothing you can do with such an address: it's not
	conceptually in the same memory space as "real memory" at all, so you cannot
	just dereference a pointer. (Sadly, on x86 it **is** in the same memory space,
	so on x86 it actually works to just deference a pointer, but it's not
	portable).

When reading or writing words directly to IO memory, in order to be portable
the Linux kernel provides the abstraction detailed in section "Differences
between I/O access functions" of Documentation/driver-api/device-io.rst.

This limits our ability to simply overlay our structs on top a buffer
and directly access it since that buffer may come from IO memory rather than
system memory. Hence the approach taken in intel_guc_ads.c needs to be
refactored. This is not the only place in i915 that neeed to be changed, but
the one causing the most problems, with a real reproducer. This first set of
patch focuses on fixing the gem object to pass the ADS

After the addition of a few helpers in the dma_buf_map API, most of
intel_guc_ads.c can be converted to use it. The exception is the regset
initialization: we'd incur into a lot of extra indirection when
reading/writting each register. So the regset is converted to use a
temporary buffer allocated on probe, which is then copied to its
final location when finishing the initialization or on gt reset.

Testing on some discrete cards, after this change we can correctly pass the
ADS struct to GuC and have it initialized correctly.

thanks
Lucas De Marchi

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
Cc: Christian König <christian.koenig@amd.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Lucas De Marchi (19):
  dma-buf-map: Add read/write helpers
  dma-buf-map: Add helper to initialize second map
  drm/i915/gt: Add helper for shmem copy to dma_buf_map
  drm/i915/guc: Keep dma_buf_map of ads_blob around
  drm/i915/guc: Add read/write helpers for ADS blob
  drm/i915/guc: Convert golden context init to dma_buf_map
  drm/i915/guc: Convert policies update to dma_buf_map
  drm/i915/guc: Convert engine record to dma_buf_map
  dma-buf-map: Add wrapper over memset
  drm/i915/guc: Convert guc_ads_private_data_reset to dma_buf_map
  drm/i915/guc: Convert golden context prep to dma_buf_map
  drm/i915/guc: Replace check for golden context size
  drm/i915/guc: Convert mapping table to dma_buf_map
  drm/i915/guc: Convert capture list to dma_buf_map
  drm/i915/guc: Prepare for error propagation
  drm/i915/guc: Use a single pass to calculate regset
  drm/i915/guc: Convert guc_mmio_reg_state_init to dma_buf_map
  drm/i915/guc: Convert __guc_ads_init to dma_buf_map
  drm/i915/guc: Remove plain ads_blob pointer

 drivers/gpu/drm/i915/gt/shmem_utils.c         |  32 ++
 drivers/gpu/drm/i915/gt/shmem_utils.h         |   3 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  14 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 374 +++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.h    |   3 +-
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  11 +-
 include/linux/dma-buf-map.h                   | 127 ++++++
 7 files changed, 405 insertions(+), 159 deletions(-)

-- 
2.35.0

[PATCH 01/19] dma-buf-map: Add read/write helpers

[Lucas De Marchi]

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.

Add a pair of macros dma_buf_map_read_field()/dma_buf_map_write_field()
to calculate the offset of a struct member and memcpy the data from/to
the dma_buf_map. 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 on IO memory or dereference the pointer.

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 | 81 +++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
index 19fa0b5ae5ec..65e927d9ce33 100644
--- a/include/linux/dma-buf-map.h
+++ b/include/linux/dma-buf-map.h
@@ -6,6 +6,7 @@
 #ifndef __DMA_BUF_MAP_H__
 #define __DMA_BUF_MAP_H__
 
+#include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/string.h>
 
@@ -229,6 +230,46 @@ static inline void dma_buf_map_clear(struct dma_buf_map *map)
 	}
 }
 
+/**
+ * dma_buf_map_memcpy_to_offset - Memcpy into offset of dma-buf mapping
+ * @dst:	The dma-buf mapping structure
+ * @offset:	The offset from which to copy
+ * @src:	The source buffer
+ * @len:	The number of byte in src
+ *
+ * Copies data into a dma-buf mapping 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 dma_buf_map_memcpy_to_offset(struct dma_buf_map *dst, size_t offset,
+						const void *src, size_t len)
+{
+	if (dst->is_iomem)
+		memcpy_toio(dst->vaddr_iomem + offset, src, len);
+	else
+		memcpy(dst->vaddr + offset, src, len);
+}
+
+/**
+ * dma_buf_map_memcpy_from_offset - Memcpy from offset of dma-buf mapping into system memory
+ * @dst:	Destination in system memory
+ * @src:	The dma-buf mapping structure
+ * @src:	The offset from which to copy
+ * @len:	The number of byte in src
+ *
+ * Copies data from a dma-buf mapping 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 dma_buf_map_memcpy_from_offset(void *dst, const struct dma_buf_map *src,
+						  size_t offset, size_t len)
+{
+	if (src->is_iomem)
+		memcpy_fromio(dst, src->vaddr_iomem + offset, len);
+	else
+		memcpy(dst, src->vaddr + offset, len);
+}
+
 /**
  * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
  * @dst:	The dma-buf mapping structure
@@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
 		map->vaddr += incr;
 }
 
+/**
+ * dma_buf_map_read_field - Read struct member from dma-buf mapping with
+ * arbitrary size and handling un-aligned accesses
+ *
+ * @map__:	The dma-buf mapping structure
+ * @type__:	The struct to be used containing the field to read
+ * @field__:	Member from struct we want to read
+ *
+ * Read a value from dma-buf mapping calculating the offset and size: this assumes
+ * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
+ * or u64 can be read, based on the offset and size of type__.field__.
+ */
+#define dma_buf_map_read_field(map__, type__, field__) ({				\
+	type__ *t__;									\
+	typeof(t__->field__) val__;							\
+	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
+				       sizeof(t__->field__));				\
+	val__;										\
+})
+
+/**
+ * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
+ * arbitrary size and handling un-aligned accesses
+ *
+ * @map__:	The dma-buf mapping structure
+ * @type__:	The struct to be used containing the field to write
+ * @field__:	Member from struct we want to write
+ * @val__:	Value to be written
+ *
+ * Write a value to the dma-buf mapping calculating the offset and size.
+ * A single u8, u16, u32 or u64 can be written based on the offset and size of
+ * type__.field__.
+ */
+#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
+	type__ *t__;									\
+	typeof(t__->field__) val____ = val__;						\
+	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
+				     &val____, sizeof(t__->field__));			\
+})
+
 #endif /* __DMA_BUF_MAP_H__ */
-- 
2.35.0

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

[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,				\
+	}
+
 /**
  * dma_buf_map_set_vaddr - Sets a dma-buf mapping structure to an address in system memory
  * @map:	The dma-buf mapping structure
-- 
2.35.0

[PATCH 09/19] dma-buf-map: Add wrapper over memset

[Lucas De Marchi]

Just like memcpy_toio(), there is also need to write a direct value to a
memory block. Add dma_buf_map_memset() to abstract memset() vs memset_io()

Cc: Matt Roper <matthew.d.roper@intel.com>
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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
index 3514a859f628..c9fb04264cd0 100644
--- a/include/linux/dma-buf-map.h
+++ b/include/linux/dma-buf-map.h
@@ -317,6 +317,23 @@ static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void *sr
 		memcpy(dst->vaddr, src, len);
 }
 
+/**
+ * dma_buf_map_memset - Memset into dma-buf mapping
+ * @dst:	The dma-buf mapping structure
+ * @value:	The value to set
+ * @len:	The number of bytes to set in dst
+ *
+ * Set value in dma-buf mapping. Depending on the buffer's location, the helper
+ * picks the correct method of accessing the memory.
+ */
+static inline void dma_buf_map_memset(struct dma_buf_map *dst, int value, size_t len)
+{
+	if (dst->is_iomem)
+		memset_io(dst->vaddr_iomem, value, len);
+	else
+		memset(dst->vaddr, value, len);
+}
+
 /**
  * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
  * @map:	The dma-buf mapping structure
-- 
2.35.0

Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

[Christian König]

Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
> 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.
>
> Add a pair of macros dma_buf_map_read_field()/dma_buf_map_write_field()
> to calculate the offset of a struct member and memcpy the data from/to
> the dma_buf_map. 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 on IO memory or dereference the pointer.
>
> 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 | 81 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 19fa0b5ae5ec..65e927d9ce33 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -6,6 +6,7 @@
>   #ifndef __DMA_BUF_MAP_H__
>   #define __DMA_BUF_MAP_H__
>   
> +#include <linux/kernel.h>
>   #include <linux/io.h>
>   #include <linux/string.h>
>   
> @@ -229,6 +230,46 @@ static inline void dma_buf_map_clear(struct dma_buf_map *map)
>   	}
>   }
>   
> +/**
> + * dma_buf_map_memcpy_to_offset - Memcpy into offset of dma-buf mapping
> + * @dst:	The dma-buf mapping structure
> + * @offset:	The offset from which to copy
> + * @src:	The source buffer
> + * @len:	The number of byte in src
> + *
> + * Copies data into a dma-buf mapping 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 dma_buf_map_memcpy_to_offset(struct dma_buf_map *dst, size_t offset,
> +						const void *src, size_t len)
> +{
> +	if (dst->is_iomem)
> +		memcpy_toio(dst->vaddr_iomem + offset, src, len);
> +	else
> +		memcpy(dst->vaddr + offset, src, len);
> +}
> +
> +/**
> + * dma_buf_map_memcpy_from_offset - Memcpy from offset of dma-buf mapping into system memory
> + * @dst:	Destination in system memory
> + * @src:	The dma-buf mapping structure
> + * @src:	The offset from which to copy
> + * @len:	The number of byte in src
> + *
> + * Copies data from a dma-buf mapping 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 dma_buf_map_memcpy_from_offset(void *dst, const struct dma_buf_map *src,
> +						  size_t offset, size_t len)
> +{
> +	if (src->is_iomem)
> +		memcpy_fromio(dst, src->vaddr_iomem + offset, len);
> +	else
> +		memcpy(dst, src->vaddr + offset, len);
> +}
> +

Well that's certainly a valid use case, but I suggest to change the 
implementation of the existing functions to call the new ones with offset=0.

This way we only have one implementation.

>   /**
>    * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
>    * @dst:	The dma-buf mapping structure
> @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
>   		map->vaddr += incr;
>   }
>   
> +/**
> + * dma_buf_map_read_field - Read struct member from dma-buf mapping with
> + * arbitrary size and handling un-aligned accesses
> + *
> + * @map__:	The dma-buf mapping structure
> + * @type__:	The struct to be used containing the field to read
> + * @field__:	Member from struct we want to read
> + *
> + * Read a value from dma-buf mapping calculating the offset and size: this assumes
> + * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
> + * or u64 can be read, based on the offset and size of type__.field__.
> + */
> +#define dma_buf_map_read_field(map__, type__, field__) ({				\
> +	type__ *t__;									\
> +	typeof(t__->field__) val__;							\
> +	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
> +				       sizeof(t__->field__));				\
> +	val__;										\
> +})
> +
> +/**
> + * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
> + * arbitrary size and handling un-aligned accesses
> + *
> + * @map__:	The dma-buf mapping structure
> + * @type__:	The struct to be used containing the field to write
> + * @field__:	Member from struct we want to write
> + * @val__:	Value to be written
> + *
> + * Write a value to the dma-buf mapping calculating the offset and size.
> + * A single u8, u16, u32 or u64 can be written based on the offset and size of
> + * type__.field__.
> + */
> +#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
> +	type__ *t__;									\
> +	typeof(t__->field__) val____ = val__;						\
> +	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
> +				     &val____, sizeof(t__->field__));			\
> +})
> +

Uff well that absolutely looks like overkill to me.

That's a rather special use case as far as I can see and I think we 
should only have this in the common framework if more than one driver is 
using it.

Regards,
Christian.

>   #endif /* __DMA_BUF_MAP_H__ */

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

[Christian König]

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.

Well what you propose here can lead to all kind of problems and is 
rather bad design as far as I can see.

The struct dma_buf_map is only to be filled in by the exporter and 
should not be modified in this way by the importer.

If you need to copy only a certain subset of the mapping use the 
functions you added in patch #1.

Regards,
Christian.

>
> 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,				\
> +	}
> +
>   /**
>    * dma_buf_map_set_vaddr - Sets a dma-buf mapping structure to an address in system memory
>    * @map:	The dma-buf mapping structure

Re: [PATCH 09/19] dma-buf-map: Add wrapper over memset

[Christian König]

Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
> Just like memcpy_toio(), there is also need to write a direct value to a
> memory block. Add dma_buf_map_memset() to abstract memset() vs memset_io()
>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> 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 | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 3514a859f628..c9fb04264cd0 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -317,6 +317,23 @@ static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void *sr
>   		memcpy(dst->vaddr, src, len);
>   }
>   
> +/**
> + * dma_buf_map_memset - Memset into dma-buf mapping
> + * @dst:	The dma-buf mapping structure
> + * @value:	The value to set
> + * @len:	The number of bytes to set in dst
> + *
> + * Set value in dma-buf mapping. Depending on the buffer's location, the helper
> + * picks the correct method of accessing the memory.
> + */
> +static inline void dma_buf_map_memset(struct dma_buf_map *dst, int value, size_t len)
> +{
> +	if (dst->is_iomem)
> +		memset_io(dst->vaddr_iomem, value, len);
> +	else
> +		memset(dst->vaddr, value, len);
> +}
> +

Yeah, that's certainly a valid use case. But maybe directly add a 
dma_buf_map_memset_with_offset() variant as well when that helps to 
avoid patch #2.

Regards,
Christian.

>   /**
>    * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
>    * @map:	The dma-buf mapping structure

Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

[Matthew Brost]

On Thu, Jan 27, 2022 at 08:24:04AM +0100, Christian König wrote:
> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
> > 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.
> > 
> > Add a pair of macros dma_buf_map_read_field()/dma_buf_map_write_field()
> > to calculate the offset of a struct member and memcpy the data from/to
> > the dma_buf_map. 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 on IO memory or dereference the pointer.
> > 
> > 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 | 81 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 81 insertions(+)
> > 
> > diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> > index 19fa0b5ae5ec..65e927d9ce33 100644
> > --- a/include/linux/dma-buf-map.h
> > +++ b/include/linux/dma-buf-map.h
> > @@ -6,6 +6,7 @@
> >   #ifndef __DMA_BUF_MAP_H__
> >   #define __DMA_BUF_MAP_H__
> > +#include <linux/kernel.h>
> >   #include <linux/io.h>
> >   #include <linux/string.h>
> > @@ -229,6 +230,46 @@ static inline void dma_buf_map_clear(struct dma_buf_map *map)
> >   	}
> >   }
> > +/**
> > + * dma_buf_map_memcpy_to_offset - Memcpy into offset of dma-buf mapping
> > + * @dst:	The dma-buf mapping structure
> > + * @offset:	The offset from which to copy
> > + * @src:	The source buffer
> > + * @len:	The number of byte in src
> > + *
> > + * Copies data into a dma-buf mapping 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 dma_buf_map_memcpy_to_offset(struct dma_buf_map *dst, size_t offset,
> > +						const void *src, size_t len)
> > +{
> > +	if (dst->is_iomem)
> > +		memcpy_toio(dst->vaddr_iomem + offset, src, len);
> > +	else
> > +		memcpy(dst->vaddr + offset, src, len);
> > +}
> > +
> > +/**
> > + * dma_buf_map_memcpy_from_offset - Memcpy from offset of dma-buf mapping into system memory
> > + * @dst:	Destination in system memory
> > + * @src:	The dma-buf mapping structure
> > + * @src:	The offset from which to copy
> > + * @len:	The number of byte in src
> > + *
> > + * Copies data from a dma-buf mapping 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 dma_buf_map_memcpy_from_offset(void *dst, const struct dma_buf_map *src,
> > +						  size_t offset, size_t len)
> > +{
> > +	if (src->is_iomem)
> > +		memcpy_fromio(dst, src->vaddr_iomem + offset, len);
> > +	else
> > +		memcpy(dst, src->vaddr + offset, len);
> > +}
> > +
> 
> Well that's certainly a valid use case, but I suggest to change the
> implementation of the existing functions to call the new ones with offset=0.
> 
> This way we only have one implementation.
> 
Trivial - but agree with Christian that is a good cleanup.

> >   /**
> >    * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> >    * @dst:	The dma-buf mapping structure
> > @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> >   		map->vaddr += incr;
> >   }
> > +/**
> > + * dma_buf_map_read_field - Read struct member from dma-buf mapping with
> > + * arbitrary size and handling un-aligned accesses
> > + *
> > + * @map__:	The dma-buf mapping structure
> > + * @type__:	The struct to be used containing the field to read
> > + * @field__:	Member from struct we want to read
> > + *
> > + * Read a value from dma-buf mapping calculating the offset and size: this assumes
> > + * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
> > + * or u64 can be read, based on the offset and size of type__.field__.
> > + */
> > +#define dma_buf_map_read_field(map__, type__, field__) ({				\
> > +	type__ *t__;									\
> > +	typeof(t__->field__) val__;							\
> > +	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
> > +				       sizeof(t__->field__));				\
> > +	val__;										\
> > +})
> > +
> > +/**
> > + * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
> > + * arbitrary size and handling un-aligned accesses
> > + *
> > + * @map__:	The dma-buf mapping structure
> > + * @type__:	The struct to be used containing the field to write
> > + * @field__:	Member from struct we want to write
> > + * @val__:	Value to be written
> > + *
> > + * Write a value to the dma-buf mapping calculating the offset and size.
> > + * A single u8, u16, u32 or u64 can be written based on the offset and size of
> > + * type__.field__.
> > + */
> > +#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
> > +	type__ *t__;									\
> > +	typeof(t__->field__) val____ = val__;						\
> > +	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
> > +				     &val____, sizeof(t__->field__));			\
> > +})
> > +
> 
> Uff well that absolutely looks like overkill to me.
> 

Hold on...

> That's a rather special use case as far as I can see and I think we should
> only have this in the common framework if more than one driver is using it.
>

I disagree, this is rather elegant.

The i915 can't be the *only* driver that defines a struct which
describes the layout of a dma_buf object.  

IMO this base macro allows *all* other drivers to build on this write
directly to fields in structures those drivers have defined. Patches
later in this series do this for the GuC ads.

Matt
 
> Regards,
> Christian.
> 
> >   #endif /* __DMA_BUF_MAP_H__ */
> 

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

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>
>Well what you propose here can lead to all kind of problems and is 
>rather bad design as far as I can see.
>
>The struct dma_buf_map is only to be filled in by the exporter and 
>should not be modified in this way by the importer.

humn... not sure if I was  clear. There is no importer and exporter here.
There is a role delegation on filling out and reading a buffer when
that buffer represents a struct layout.

struct bla {
	int a;
	int b;
	int c;
	struct foo foo;
	struct bar bar;
	int d;
}


This implementation allows you to have:

	fill_foo(struct dma_buf_map *bla_map) { ... }
	fill_bar(struct dma_buf_map *bla_map) { ... }

and the first thing these do is to make sure the map it's pointing to
is relative to the struct it's supposed to write/read. Otherwise you're
suggesting everything to be relative to struct bla, or to do the same
I'm doing it, but IMO more prone to error:

	struct dma_buf_map map = *bla_map;
	dma_buf_map_incr(map, offsetof(...));

IMO this construct is worse because at a point in time in the function
the map was pointing to the wrong thing the function was supposed to
read/write.

It's also useful when the function has double duty, updating a global
part of the struct and a table inside it (see example in patch 6)

thanks
Lucas De Marchi

Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

[Christian König]

Am 27.01.22 um 08:36 schrieb Matthew Brost:
> [SNIP]
>>>    /**
>>>     * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
>>>     * @dst:	The dma-buf mapping structure
>>> @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
>>>    		map->vaddr += incr;
>>>    }
>>> +/**
>>> + * dma_buf_map_read_field - Read struct member from dma-buf mapping with
>>> + * arbitrary size and handling un-aligned accesses
>>> + *
>>> + * @map__:	The dma-buf mapping structure
>>> + * @type__:	The struct to be used containing the field to read
>>> + * @field__:	Member from struct we want to read
>>> + *
>>> + * Read a value from dma-buf mapping calculating the offset and size: this assumes
>>> + * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
>>> + * or u64 can be read, based on the offset and size of type__.field__.
>>> + */
>>> +#define dma_buf_map_read_field(map__, type__, field__) ({				\
>>> +	type__ *t__;									\
>>> +	typeof(t__->field__) val__;							\
>>> +	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
>>> +				       sizeof(t__->field__));				\
>>> +	val__;										\
>>> +})
>>> +
>>> +/**
>>> + * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
>>> + * arbitrary size and handling un-aligned accesses
>>> + *
>>> + * @map__:	The dma-buf mapping structure
>>> + * @type__:	The struct to be used containing the field to write
>>> + * @field__:	Member from struct we want to write
>>> + * @val__:	Value to be written
>>> + *
>>> + * Write a value to the dma-buf mapping calculating the offset and size.
>>> + * A single u8, u16, u32 or u64 can be written based on the offset and size of
>>> + * type__.field__.
>>> + */
>>> +#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
>>> +	type__ *t__;									\
>>> +	typeof(t__->field__) val____ = val__;						\
>>> +	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
>>> +				     &val____, sizeof(t__->field__));			\
>>> +})
>>> +
>> Uff well that absolutely looks like overkill to me.
>>
> Hold on...
>
>> That's a rather special use case as far as I can see and I think we should
>> only have this in the common framework if more than one driver is using it.
>>
> I disagree, this is rather elegant.
>
> The i915 can't be the *only* driver that defines a struct which
> describes the layout of a dma_buf object.

That's not the problem, amdgpu as well as nouveau are doing that as 
well. The problem is DMA-buf is a buffer sharing framework between drivers.

In other words which importer is supposed to use this with a DMA-buf 
exported by another device?

> IMO this base macro allows *all* other drivers to build on this write
> directly to fields in structures those drivers have defined.

Exactly that's the point. This is something drivers should absolutely 
*NOT* do.

That are driver internals and it is extremely questionable to move this 
into the common framework.

Regards,
Christian.

>   Patches
> later in this series do this for the GuC ads.
>
> Matt
>   
>> Regards,
>> Christian.
>>
>>>    #endif /* __DMA_BUF_MAP_H__ */

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

[Christian König]

Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>
>> Well what you propose here can lead to all kind of problems and is 
>> rather bad design as far as I can see.
>>
>> The struct dma_buf_map is only to be filled in by the exporter and 
>> should not be modified in this way by the importer.
>
> humn... not sure if I was  clear. There is no importer and exporter here.

Yeah, and exactly that's what I'm pointing out as problem here.

You are using the inter driver framework for something internal to the 
driver. That is an absolutely clear NAK!

We could discuss that, but you guys are just sending around patches to 
do this without any consensus that this is a good idea.

Regards,
Christian.


> There is a role delegation on filling out and reading a buffer when
> that buffer represents a struct layout.
>
> struct bla {
>     int a;
>     int b;
>     int c;
>     struct foo foo;
>     struct bar bar;
>     int d;
> }
>
>
> This implementation allows you to have:
>
>     fill_foo(struct dma_buf_map *bla_map) { ... }
>     fill_bar(struct dma_buf_map *bla_map) { ... }
>
> and the first thing these do is to make sure the map it's pointing to
> is relative to the struct it's supposed to write/read. Otherwise you're
> suggesting everything to be relative to struct bla, or to do the same
> I'm doing it, but IMO more prone to error:
>
>     struct dma_buf_map map = *bla_map;
>     dma_buf_map_incr(map, offsetof(...));
>
> IMO this construct is worse because at a point in time in the function
> the map was pointing to the wrong thing the function was supposed to
> read/write.
>
> It's also useful when the function has double duty, updating a global
> part of the struct and a table inside it (see example in patch 6)
>
> thanks
> Lucas De Marchi

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

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>>
>>>Well what you propose here can lead to all kind of problems and is 
>>>rather bad design as far as I can see.
>>>
>>>The struct dma_buf_map is only to be filled in by the exporter and 
>>>should not be modified in this way by the importer.
>>
>>humn... not sure if I was  clear. There is no importer and exporter here.
>
>Yeah, and exactly that's what I'm pointing out as problem here.
>
>You are using the inter driver framework for something internal to the 
>driver. That is an absolutely clear NAK!
>
>We could discuss that, but you guys are just sending around patches to 
>do this without any consensus that this is a good idea.

s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in
these patches. I'm sending these to _build consensus_ on what may be a good
use for it showing a real problem it's helping to fix.

 From its documentation:

  * The type :c:type:`struct dma_buf_map <dma_buf_map>` and its helpers are
  * actually independent from the dma-buf infrastructure. When sharing buffers
  * among devices, drivers have to know the location of the memory to access
  * the buffers in a safe way. :c:type:`struct dma_buf_map <dma_buf_map>`
  * solves this problem for dma-buf and its users. If other drivers or
  * sub-systems require similar functionality, the type could be generalized
  * and moved to a more prominent header file.

if there is no consensus and a better alternative, I'm perfectly fine in
throwing it out and using the better approach.

Lucas De Marchi

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

[Christian König]

Am 27.01.22 um 09:18 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote:
>>>> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>>>> [SNIP]
>>> humn... not sure if I was  clear. There is no importer and exporter 
>>> here.
>>
>> Yeah, and exactly that's what I'm pointing out as problem here.
>>
>> You are using the inter driver framework for something internal to 
>> the driver. That is an absolutely clear NAK!
>>
>> We could discuss that, but you guys are just sending around patches 
>> to do this without any consensus that this is a good idea.
>
> s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in
> these patches. I'm sending these to _build consensus_ on what may be a 
> good
> use for it showing a real problem it's helping to fix.

Well a cover letter would have been helpful, my impression was that you 
have a larger set and just want to upstream some minor DMA-buf changes 
necessary for it.

Now I know why people are bugging me all the time to add cover letters 
to add more context to my sets.

>
> From its documentation:
>
>  * The type :c:type:`struct dma_buf_map <dma_buf_map>` and its helpers 
> are
>  * actually independent from the dma-buf infrastructure. When sharing 
> buffers
>  * among devices, drivers have to know the location of the memory to 
> access
>  * the buffers in a safe way. :c:type:`struct dma_buf_map <dma_buf_map>`
>  * solves this problem for dma-buf and its users. If other drivers or
>  * sub-systems require similar functionality, the type could be 
> generalized
>  * and moved to a more prominent header file.
>
> if there is no consensus and a better alternative, I'm perfectly fine in
> throwing it out and using the better approach.

When Thomas Zimmermann upstreamed the dma_buf_map work we had a 
discussion if that shouldn't be independent of the DMA-buf framework.

The consensus was that as soon as we have more widely use for it this 
should be made independent. So basically that is what's happening now.

I suggest the following approach:
1. Find a funky name for this, something like iomem_, kiomap_ or similar.
2. Separate this from all you driver dependent work and move the 
dma_buf_map structure out of DMA-buf into this new whatever_ prefix.
3. Ping Thomas, LKML, me and probably a couple of other core people if 
this is the right idea or not.
4. Work on dropping the map parameter from dma_buf_vunmap(). This is 
basically why we can't modify the pointers returned from dma_buf_vmap() 
and has already cause a few problems with dma_buf_map_incr().

Regards,
Christian.

>
> Lucas De Marchi

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

[Daniel Vetter]

On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > 
> > > Well what you propose here can lead to all kind of problems and is
> > > rather bad design as far as I can see.
> > > 
> > > The struct dma_buf_map is only to be filled in by the exporter and
> > > should not be modified in this way by the importer.
> > 
> > humn... not sure if I was  clear. There is no importer and exporter here.
> 
> Yeah, and exactly that's what I'm pointing out as problem here.
> 
> You are using the inter driver framework for something internal to the
> driver. That is an absolutely clear NAK!
> 
> We could discuss that, but you guys are just sending around patches to do
> this without any consensus that this is a good idea.

Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should allow
drivers to simplify some code where on igpu it's in normal kernel memory
and on dgpu it's behind some pci bar.

Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't come
up with anything better really.

> > There is a role delegation on filling out and reading a buffer when
> > that buffer represents a struct layout.
> > 
> > struct bla {
> >     int a;
> >     int b;
> >     int c;
> >     struct foo foo;
> >     struct bar bar;
> >     int d;
> > }
> > 
> > 
> > This implementation allows you to have:
> > 
> >     fill_foo(struct dma_buf_map *bla_map) { ... }
> >     fill_bar(struct dma_buf_map *bla_map) { ... }
> > 
> > and the first thing these do is to make sure the map it's pointing to
> > is relative to the struct it's supposed to write/read. Otherwise you're
> > suggesting everything to be relative to struct bla, or to do the same
> > I'm doing it, but IMO more prone to error:
> > 
> >     struct dma_buf_map map = *bla_map;
> >     dma_buf_map_incr(map, offsetof(...));

Wrt the issue at hand I think the above is perfectly fine code. The idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
-Daniel

> > IMO this construct is worse because at a point in time in the function
> > the map was pointing to the wrong thing the function was supposed to
> > read/write.
> > 
> > It's also useful when the function has double duty, updating a global
> > part of the struct and a table inside it (see example in patch 6)
> > 
> > thanks
> > Lucas De Marchi
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 01/19] dma-buf-map: Add read/write helpers

[Daniel Vetter]

On Thu, Jan 27, 2022 at 08:59:36AM +0100, Christian König wrote:
> Am 27.01.22 um 08:36 schrieb Matthew Brost:
> > [SNIP]
> > > >    /**
> > > >     * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> > > >     * @dst:	The dma-buf mapping structure
> > > > @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> > > >    		map->vaddr += incr;
> > > >    }
> > > > +/**
> > > > + * dma_buf_map_read_field - Read struct member from dma-buf mapping with
> > > > + * arbitrary size and handling un-aligned accesses
> > > > + *
> > > > + * @map__:	The dma-buf mapping structure
> > > > + * @type__:	The struct to be used containing the field to read
> > > > + * @field__:	Member from struct we want to read
> > > > + *
> > > > + * Read a value from dma-buf mapping calculating the offset and size: this assumes
> > > > + * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
> > > > + * or u64 can be read, based on the offset and size of type__.field__.
> > > > + */
> > > > +#define dma_buf_map_read_field(map__, type__, field__) ({				\
> > > > +	type__ *t__;									\
> > > > +	typeof(t__->field__) val__;							\
> > > > +	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
> > > > +				       sizeof(t__->field__));				\
> > > > +	val__;										\
> > > > +})
> > > > +
> > > > +/**
> > > > + * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
> > > > + * arbitrary size and handling un-aligned accesses
> > > > + *
> > > > + * @map__:	The dma-buf mapping structure
> > > > + * @type__:	The struct to be used containing the field to write
> > > > + * @field__:	Member from struct we want to write
> > > > + * @val__:	Value to be written
> > > > + *
> > > > + * Write a value to the dma-buf mapping calculating the offset and size.
> > > > + * A single u8, u16, u32 or u64 can be written based on the offset and size of
> > > > + * type__.field__.
> > > > + */
> > > > +#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
> > > > +	type__ *t__;									\
> > > > +	typeof(t__->field__) val____ = val__;						\
> > > > +	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
> > > > +				     &val____, sizeof(t__->field__));			\
> > > > +})
> > > > +
> > > Uff well that absolutely looks like overkill to me.
> > > 
> > Hold on...
> > 
> > > That's a rather special use case as far as I can see and I think we should
> > > only have this in the common framework if more than one driver is using it.
> > > 
> > I disagree, this is rather elegant.
> > 
> > The i915 can't be the *only* driver that defines a struct which
> > describes the layout of a dma_buf object.
> 
> That's not the problem, amdgpu as well as nouveau are doing that as well.
> The problem is DMA-buf is a buffer sharing framework between drivers.
> 
> In other words which importer is supposed to use this with a DMA-buf
> exported by another device?
> 
> > IMO this base macro allows *all* other drivers to build on this write
> > directly to fields in structures those drivers have defined.
> 
> Exactly that's the point. This is something drivers should absolutely *NOT*
> do.
> 
> That are driver internals and it is extremely questionable to move this into
> the common framework.

See my other reply.

This is about struct dma_buf_map, which is just a tagged pointer.

Which happens to be used by the dma_buf cross-driver interface, but it's
also used plenty internally in buffer allocation helpers, fbdev,
everything else. And it was _meant_ to be used like that - this thing is
my idea, I know :-)

I guess we could move/rename it, but like I said I really don't have any
good ideas. Got some?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 09:55:05AM +0100, Christian König wrote:
>Am 27.01.22 um 09:18 schrieb Lucas De Marchi:
>>On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote:
>>>>>Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>>>>>[SNIP]
>>>>humn... not sure if I was  clear. There is no importer and 
>>>>exporter here.
>>>
>>>Yeah, and exactly that's what I'm pointing out as problem here.
>>>
>>>You are using the inter driver framework for something internal to 
>>>the driver. That is an absolutely clear NAK!
>>>
>>>We could discuss that, but you guys are just sending around 
>>>patches to do this without any consensus that this is a good idea.
>>
>>s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in
>>these patches. I'm sending these to _build consensus_ on what may be 
>>a good
>>use for it showing a real problem it's helping to fix.
>
>Well a cover letter would have been helpful, my impression was that 
>you have a larger set and just want to upstream some minor DMA-buf 
>changes necessary for it.

I missed adding this sentence to the cover letter, as my impression was that
dma-buf-map was already used outside inter-driver framework. But there
is actually a cover letter:

https://lore.kernel.org/all/20220126203702.1784589-1-lucas.demarchi@intel.com/

And looking at it now, it seems I missed adding Thomas Zimmermann to Cc.

>
>Now I know why people are bugging me all the time to add cover letters 
>to add more context to my sets.
>
>>
>>From its documentation:
>>
>> * The type :c:type:`struct dma_buf_map <dma_buf_map>` and its 
>>helpers are
>> * actually independent from the dma-buf infrastructure. When 
>>sharing buffers
>> * among devices, drivers have to know the location of the memory to 
>>access
>> * the buffers in a safe way. :c:type:`struct dma_buf_map <dma_buf_map>`
>> * solves this problem for dma-buf and its users. If other drivers or
>> * sub-systems require similar functionality, the type could be 
>>generalized
>> * and moved to a more prominent header file.
>>
>>if there is no consensus and a better alternative, I'm perfectly fine in
>>throwing it out and using the better approach.
>
>When Thomas Zimmermann upstreamed the dma_buf_map work we had a 
>discussion if that shouldn't be independent of the DMA-buf framework.
>
>The consensus was that as soon as we have more widely use for it this 
>should be made independent. So basically that is what's happening now.
>
>I suggest the following approach:
>1. Find a funky name for this, something like iomem_, kiomap_ or similar.

iosys_map?

>2. Separate this from all you driver dependent work and move the 
>dma_buf_map structure out of DMA-buf into this new whatever_ prefix.

should this be a follow up to the driver work or a prerequisite?

thanks
Lucas De Marchi

>3. Ping Thomas, LKML, me and probably a couple of other core people if 
>this is the right idea or not.
>4. Work on dropping the map parameter from dma_buf_vunmap(). This is 
>basically why we can't modify the pointers returned from 
>dma_buf_vmap() and has already cause a few problems with 
>dma_buf_map_incr().
>
>Regards,
>Christian.
>
>>
>>Lucas De Marchi
>

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

[Christian König]

Am 27.01.22 um 10:12 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 09:55:05AM +0100, Christian König wrote:
>> Am 27.01.22 um 09:18 schrieb Lucas De Marchi:
>>> On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote:
>>>>>> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>>>>>> [SNIP]
>>>>> humn... not sure if I was  clear. There is no importer and 
>>>>> exporter here.
>>>>
>>>> Yeah, and exactly that's what I'm pointing out as problem here.
>>>>
>>>> You are using the inter driver framework for something internal to 
>>>> the driver. That is an absolutely clear NAK!
>>>>
>>>> We could discuss that, but you guys are just sending around patches 
>>>> to do this without any consensus that this is a good idea.
>>>
>>> s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in
>>> these patches. I'm sending these to _build consensus_ on what may be 
>>> a good
>>> use for it showing a real problem it's helping to fix.
>>
>> Well a cover letter would have been helpful, my impression was that 
>> you have a larger set and just want to upstream some minor DMA-buf 
>> changes necessary for it.
>
> I missed adding this sentence to the cover letter, as my impression 
> was that
> dma-buf-map was already used outside inter-driver framework. But there
> is actually a cover letter:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cb36def4a6ebd4879731c08d9e1753ccd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788715933199161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=gwW05OaUq%2FxlBWnY%2FPuPfl0YDdKp5VTbllaSmn45nE8%3D&reserved=0 
>
>
> And looking at it now, it seems I missed adding Thomas Zimmermann to Cc.
>
>>
>> Now I know why people are bugging me all the time to add cover 
>> letters to add more context to my sets.
>>
>>>
>>> From its documentation:
>>>
>>>  * The type :c:type:`struct dma_buf_map <dma_buf_map>` and its 
>>> helpers are
>>>  * actually independent from the dma-buf infrastructure. When 
>>> sharing buffers
>>>  * among devices, drivers have to know the location of the memory to 
>>> access
>>>  * the buffers in a safe way. :c:type:`struct dma_buf_map 
>>> <dma_buf_map>`
>>>  * solves this problem for dma-buf and its users. If other drivers or
>>>  * sub-systems require similar functionality, the type could be 
>>> generalized
>>>  * and moved to a more prominent header file.
>>>
>>> if there is no consensus and a better alternative, I'm perfectly 
>>> fine in
>>> throwing it out and using the better approach.
>>
>> When Thomas Zimmermann upstreamed the dma_buf_map work we had a 
>> discussion if that shouldn't be independent of the DMA-buf framework.
>>
>> The consensus was that as soon as we have more widely use for it this 
>> should be made independent. So basically that is what's happening now.
>>
>> I suggest the following approach:
>> 1. Find a funky name for this, something like iomem_, kiomap_ or 
>> similar.
>
> iosys_map?

Works for me.

>
>> 2. Separate this from all you driver dependent work and move the 
>> dma_buf_map structure out of DMA-buf into this new whatever_ prefix.
>
> should this be a follow up to the driver work or a prerequisite?

Prerequisite. Structural changes like this always separate to the 
actually work switching over to them because the later needs a much 
fewer audience for review.

Regards,
Christian.

>
> thanks
> Lucas De Marchi
>
>> 3. Ping Thomas, LKML, me and probably a couple of other core people 
>> if this is the right idea or not.
>> 4. Work on dropping the map parameter from dma_buf_vunmap(). This is 
>> basically why we can't modify the pointers returned from 
>> dma_buf_vmap() and has already cause a few problems with 
>> dma_buf_map_incr().
>>
>> Regards,
>> Christian.
>>
>>>
>>> Lucas De Marchi
>>

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

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
>On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>> > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>> > >
>> > > Well what you propose here can lead to all kind of problems and is
>> > > rather bad design as far as I can see.
>> > >
>> > > The struct dma_buf_map is only to be filled in by the exporter and
>> > > should not be modified in this way by the importer.
>> >
>> > humn... not sure if I was  clear. There is no importer and exporter here.
>>
>> Yeah, and exactly that's what I'm pointing out as problem here.
>>
>> You are using the inter driver framework for something internal to the
>> driver. That is an absolutely clear NAK!
>>
>> We could discuss that, but you guys are just sending around patches to do
>> this without any consensus that this is a good idea.
>
>Uh I suggested this, also we're already using dma_buf_map all over the
>place as a convenient abstraction. So imo that's all fine, it should allow
>drivers to simplify some code where on igpu it's in normal kernel memory
>and on dgpu it's behind some pci bar.
>
>Maybe we should have a better name for that struct (and maybe also a
>better place), but way back when we discussed that bikeshed I didn't come
>up with anything better really.

I suggest iosys_map since it abstracts access to IO and system memory.

>
>> > There is a role delegation on filling out and reading a buffer when
>> > that buffer represents a struct layout.
>> >
>> > struct bla {
>> >     int a;
>> >     int b;
>> >     int c;
>> >     struct foo foo;
>> >     struct bar bar;
>> >     int d;
>> > }
>> >
>> >
>> > This implementation allows you to have:
>> >
>> >     fill_foo(struct dma_buf_map *bla_map) { ... }
>> >     fill_bar(struct dma_buf_map *bla_map) { ... }
>> >
>> > and the first thing these do is to make sure the map it's pointing to
>> > is relative to the struct it's supposed to write/read. Otherwise you're
>> > suggesting everything to be relative to struct bla, or to do the same
>> > I'm doing it, but IMO more prone to error:
>> >
>> >     struct dma_buf_map map = *bla_map;
>> >     dma_buf_map_incr(map, offsetof(...));
>
>Wrt the issue at hand I think the above is perfectly fine code. The idea
>with dma_buf_map is really that it's just a special pointer, so writing
>the code exactly as pointer code feels best. Unfortunately you cannot make
>them typesafe (because of C), so the code sometimes looks a bit ugly.
>Otherwise we could do stuff like container_of and all that with
>typechecking in the macros.

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

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(..));

Lucas De Marchi

>-Daniel
>
>> > IMO this construct is worse because at a point in time in the function
>> > the map was pointing to the wrong thing the function was supposed to
>> > read/write.
>> >
>> > It's also useful when the function has double duty, updating a global
>> > part of the struct and a table inside it (see example in patch 6)
>> >
>> > thanks
>> > Lucas De Marchi
>>
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

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

[Daniel Vetter]

On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
> On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
> > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > > >
> > > > > Well what you propose here can lead to all kind of problems and is
> > > > > rather bad design as far as I can see.
> > > > >
> > > > > The struct dma_buf_map is only to be filled in by the exporter and
> > > > > should not be modified in this way by the importer.
> > > >
> > > > humn... not sure if I was  clear. There is no importer and exporter here.
> > > 
> > > Yeah, and exactly that's what I'm pointing out as problem here.
> > > 
> > > You are using the inter driver framework for something internal to the
> > > driver. That is an absolutely clear NAK!
> > > 
> > > We could discuss that, but you guys are just sending around patches to do
> > > this without any consensus that this is a good idea.
> > 
> > Uh I suggested this, also we're already using dma_buf_map all over the
> > place as a convenient abstraction. So imo that's all fine, it should allow
> > drivers to simplify some code where on igpu it's in normal kernel memory
> > and on dgpu it's behind some pci bar.
> > 
> > Maybe we should have a better name for that struct (and maybe also a
> > better place), but way back when we discussed that bikeshed I didn't come
> > up with anything better really.
> 
> I suggest iosys_map since it abstracts access to IO and system memory.
> 
> > 
> > > > There is a role delegation on filling out and reading a buffer when
> > > > that buffer represents a struct layout.
> > > >
> > > > struct bla {
> > > >     int a;
> > > >     int b;
> > > >     int c;
> > > >     struct foo foo;
> > > >     struct bar bar;
> > > >     int d;
> > > > }
> > > >
> > > >
> > > > This implementation allows you to have:
> > > >
> > > >     fill_foo(struct dma_buf_map *bla_map) { ... }
> > > >     fill_bar(struct dma_buf_map *bla_map) { ... }
> > > >
> > > > and the first thing these do is to make sure the map it's pointing to
> > > > is relative to the struct it's supposed to write/read. Otherwise you're
> > > > suggesting everything to be relative to struct bla, or to do the same
> > > > I'm doing it, but IMO more prone to error:
> > > >
> > > >     struct dma_buf_map map = *bla_map;
> > > >     dma_buf_map_incr(map, offsetof(...));
> > 
> > Wrt the issue at hand I think the above is perfectly fine code. The idea
> > with dma_buf_map is really that it's just a special pointer, so writing
> > the code exactly as pointer code feels best. Unfortunately you cannot make
> > them typesafe (because of C), so the code sometimes looks a bit ugly.
> > Otherwise we could do stuff like container_of and all that with
> > typechecking in the macros.
> 
> 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
> 
> 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(..));

Hm yeah that's a good point that this allows us to rely on the compiler to
check for uninitialized variables.

Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
-Daniel

> 
> Lucas De Marchi
> 
> > -Daniel
> > 
> > > > IMO this construct is worse because at a point in time in the function
> > > > the map was pointing to the wrong thing the function was supposed to
> > > > read/write.
> > > >
> > > > It's also useful when the function has double duty, updating a global
> > > > part of the struct and a table inside it (see example in patch 6)
> > > >
> > > > thanks
> > > > Lucas De Marchi
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Christian König]

Am 27.01.22 um 11:00 schrieb Daniel Vetter:
> On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
>> On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
>>> On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>>>>> Well what you propose here can lead to all kind of problems and is
>>>>>> rather bad design as far as I can see.
>>>>>>
>>>>>> The struct dma_buf_map is only to be filled in by the exporter and
>>>>>> should not be modified in this way by the importer.
>>>>> humn... not sure if I was  clear. There is no importer and exporter here.
>>>> Yeah, and exactly that's what I'm pointing out as problem here.
>>>>
>>>> You are using the inter driver framework for something internal to the
>>>> driver. That is an absolutely clear NAK!
>>>>
>>>> We could discuss that, but you guys are just sending around patches to do
>>>> this without any consensus that this is a good idea.
>>> Uh I suggested this, also we're already using dma_buf_map all over the
>>> place as a convenient abstraction. So imo that's all fine, it should allow
>>> drivers to simplify some code where on igpu it's in normal kernel memory
>>> and on dgpu it's behind some pci bar.
>>>
>>> Maybe we should have a better name for that struct (and maybe also a
>>> better place), but way back when we discussed that bikeshed I didn't come
>>> up with anything better really.
>> I suggest iosys_map since it abstracts access to IO and system memory.
>>
>>>>> There is a role delegation on filling out and reading a buffer when
>>>>> that buffer represents a struct layout.
>>>>>
>>>>> struct bla {
>>>>>      int a;
>>>>>      int b;
>>>>>      int c;
>>>>>      struct foo foo;
>>>>>      struct bar bar;
>>>>>      int d;
>>>>> }
>>>>>
>>>>>
>>>>> This implementation allows you to have:
>>>>>
>>>>>      fill_foo(struct dma_buf_map *bla_map) { ... }
>>>>>      fill_bar(struct dma_buf_map *bla_map) { ... }
>>>>>
>>>>> and the first thing these do is to make sure the map it's pointing to
>>>>> is relative to the struct it's supposed to write/read. Otherwise you're
>>>>> suggesting everything to be relative to struct bla, or to do the same
>>>>> I'm doing it, but IMO more prone to error:
>>>>>
>>>>>      struct dma_buf_map map = *bla_map;
>>>>>      dma_buf_map_incr(map, offsetof(...));
>>> Wrt the issue at hand I think the above is perfectly fine code. The idea
>>> with dma_buf_map is really that it's just a special pointer, so writing
>>> the code exactly as pointer code feels best. Unfortunately you cannot make
>>> them typesafe (because of C), so the code sometimes looks a bit ugly.
>>> Otherwise we could do stuff like container_of and all that with
>>> typechecking in the macros.
>> 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
>>
>> 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(..));
> Hm yeah that's a good point that this allows us to rely on the compiler to
> check for uninitialized variables.
>
> Maybe include the above (with editing, but keeping the examples) in the
> kerneldoc to explain why/how to use this? With that the concept at least
> has my
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> I'll leave it up to you & Christian to find a prettier color choice for
> the naming bikeshed.

There is one major issue remaining with this and that is dma_buf_vunmap():

void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);

Here we expect the original pointer as returned by dma_buf_map(), 
otherwise we vunmap() the wrong area!

For all TTM based driver this doesn't matter since we keep the vmap base 
separately in the BO anyway (IIRC), but we had at least one case where 
this made boom last year.

Christian.

> -Daniel
>
>> Lucas De Marchi
>>
>>> -Daniel
>>>
>>>>> IMO this construct is worse because at a point in time in the function
>>>>> the map was pointing to the wrong thing the function was supposed to
>>>>> read/write.
>>>>>
>>>>> It's also useful when the function has double duty, updating a global
>>>>> part of the struct and a table inside it (see example in patch 6)
>>>>>
>>>>> thanks
>>>>> Lucas De Marchi
>>> -- 
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0

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

[Daniel Vetter]

On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
> Am 27.01.22 um 11:00 schrieb Daniel Vetter:
> > On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
> > > On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
> > > > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> > > > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > > > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > > > > > Well what you propose here can lead to all kind of problems and is
> > > > > > > rather bad design as far as I can see.
> > > > > > > 
> > > > > > > The struct dma_buf_map is only to be filled in by the exporter and
> > > > > > > should not be modified in this way by the importer.
> > > > > > humn... not sure if I was  clear. There is no importer and exporter here.
> > > > > Yeah, and exactly that's what I'm pointing out as problem here.
> > > > > 
> > > > > You are using the inter driver framework for something internal to the
> > > > > driver. That is an absolutely clear NAK!
> > > > > 
> > > > > We could discuss that, but you guys are just sending around patches to do
> > > > > this without any consensus that this is a good idea.
> > > > Uh I suggested this, also we're already using dma_buf_map all over the
> > > > place as a convenient abstraction. So imo that's all fine, it should allow
> > > > drivers to simplify some code where on igpu it's in normal kernel memory
> > > > and on dgpu it's behind some pci bar.
> > > > 
> > > > Maybe we should have a better name for that struct (and maybe also a
> > > > better place), but way back when we discussed that bikeshed I didn't come
> > > > up with anything better really.
> > > I suggest iosys_map since it abstracts access to IO and system memory.
> > > 
> > > > > > There is a role delegation on filling out and reading a buffer when
> > > > > > that buffer represents a struct layout.
> > > > > > 
> > > > > > struct bla {
> > > > > >      int a;
> > > > > >      int b;
> > > > > >      int c;
> > > > > >      struct foo foo;
> > > > > >      struct bar bar;
> > > > > >      int d;
> > > > > > }
> > > > > > 
> > > > > > 
> > > > > > This implementation allows you to have:
> > > > > > 
> > > > > >      fill_foo(struct dma_buf_map *bla_map) { ... }
> > > > > >      fill_bar(struct dma_buf_map *bla_map) { ... }
> > > > > > 
> > > > > > and the first thing these do is to make sure the map it's pointing to
> > > > > > is relative to the struct it's supposed to write/read. Otherwise you're
> > > > > > suggesting everything to be relative to struct bla, or to do the same
> > > > > > I'm doing it, but IMO more prone to error:
> > > > > > 
> > > > > >      struct dma_buf_map map = *bla_map;
> > > > > >      dma_buf_map_incr(map, offsetof(...));
> > > > Wrt the issue at hand I think the above is perfectly fine code. The idea
> > > > with dma_buf_map is really that it's just a special pointer, so writing
> > > > the code exactly as pointer code feels best. Unfortunately you cannot make
> > > > them typesafe (because of C), so the code sometimes looks a bit ugly.
> > > > Otherwise we could do stuff like container_of and all that with
> > > > typechecking in the macros.
> > > 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
> > > 
> > > 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(..));
> > Hm yeah that's a good point that this allows us to rely on the compiler to
> > check for uninitialized variables.
> > 
> > Maybe include the above (with editing, but keeping the examples) in the
> > kerneldoc to explain why/how to use this? With that the concept at least
> > has my
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I'll leave it up to you & Christian to find a prettier color choice for
> > the naming bikeshed.
> 
> There is one major issue remaining with this and that is dma_buf_vunmap():
> 
> void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
> 
> Here we expect the original pointer as returned by dma_buf_map(), otherwise
> we vunmap() the wrong area!
> 
> For all TTM based driver this doesn't matter since we keep the vmap base
> separately in the BO anyway (IIRC), but we had at least one case where this
> made boom last year.

Yeah but isn't that the same if it's just a void *?

If you pass the wrong pointer to an unmap function and not exactly what
you go from the map function, then things go boom. This is like
complaining that the following code wont work

	u32 *stuff

	stuff = kmap_local(some_page);
	*stuff++ = 0;
	*stuff = 1;
	kunmap_locak(stuff);

It's just ... don't do that :-) Also since we pass dma_buf_map by value
and not by pointer anywhere, the risk of this happening is pretty low
since you tend to work on a copy. Same with void * pointers really.

Now if people start to pass around struct dma_buf_map * as pointers for
anything else than out parameters, then we're screwed. But that's like
passing around void ** for lolz, which is just wrong (except when it's an
out parameter or actually an array of pointers ofc).

Or I really don't get your concern and you mean something else?
-Daniel


> Christian.
> 
> > -Daniel
> > 
> > > Lucas De Marchi
> > > 
> > > > -Daniel
> > > > 
> > > > > > IMO this construct is worse because at a point in time in the function
> > > > > > the map was pointing to the wrong thing the function was supposed to
> > > > > > read/write.
> > > > > > 
> > > > > > It's also useful when the function has double duty, updating a global
> > > > > > part of the struct and a table inside it (see example in patch 6)
> > > > > > 
> > > > > > thanks
> > > > > > Lucas De Marchi
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Christian König]

Am 27.01.22 um 12:16 schrieb Daniel Vetter:
> On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
>> Am 27.01.22 um 11:00 schrieb Daniel Vetter:
>>> On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
>>>> On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
>>>>> On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>>>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>>>> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>>>>>>> Well what you propose here can lead to all kind of problems and is
>>>>>>>> rather bad design as far as I can see.
>>>>>>>>
>>>>>>>> The struct dma_buf_map is only to be filled in by the exporter and
>>>>>>>> should not be modified in this way by the importer.
>>>>>>> humn... not sure if I was  clear. There is no importer and exporter here.
>>>>>> Yeah, and exactly that's what I'm pointing out as problem here.
>>>>>>
>>>>>> You are using the inter driver framework for something internal to the
>>>>>> driver. That is an absolutely clear NAK!
>>>>>>
>>>>>> We could discuss that, but you guys are just sending around patches to do
>>>>>> this without any consensus that this is a good idea.
>>>>> Uh I suggested this, also we're already using dma_buf_map all over the
>>>>> place as a convenient abstraction. So imo that's all fine, it should allow
>>>>> drivers to simplify some code where on igpu it's in normal kernel memory
>>>>> and on dgpu it's behind some pci bar.
>>>>>
>>>>> Maybe we should have a better name for that struct (and maybe also a
>>>>> better place), but way back when we discussed that bikeshed I didn't come
>>>>> up with anything better really.
>>>> I suggest iosys_map since it abstracts access to IO and system memory.
>>>>
>>>>>>> There is a role delegation on filling out and reading a buffer when
>>>>>>> that buffer represents a struct layout.
>>>>>>>
>>>>>>> struct bla {
>>>>>>>       int a;
>>>>>>>       int b;
>>>>>>>       int c;
>>>>>>>       struct foo foo;
>>>>>>>       struct bar bar;
>>>>>>>       int d;
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> This implementation allows you to have:
>>>>>>>
>>>>>>>       fill_foo(struct dma_buf_map *bla_map) { ... }
>>>>>>>       fill_bar(struct dma_buf_map *bla_map) { ... }
>>>>>>>
>>>>>>> and the first thing these do is to make sure the map it's pointing to
>>>>>>> is relative to the struct it's supposed to write/read. Otherwise you're
>>>>>>> suggesting everything to be relative to struct bla, or to do the same
>>>>>>> I'm doing it, but IMO more prone to error:
>>>>>>>
>>>>>>>       struct dma_buf_map map = *bla_map;
>>>>>>>       dma_buf_map_incr(map, offsetof(...));
>>>>> Wrt the issue at hand I think the above is perfectly fine code. The idea
>>>>> with dma_buf_map is really that it's just a special pointer, so writing
>>>>> the code exactly as pointer code feels best. Unfortunately you cannot make
>>>>> them typesafe (because of C), so the code sometimes looks a bit ugly.
>>>>> Otherwise we could do stuff like container_of and all that with
>>>>> typechecking in the macros.
>>>> 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
>>>>
>>>> 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(..));
>>> Hm yeah that's a good point that this allows us to rely on the compiler to
>>> check for uninitialized variables.
>>>
>>> Maybe include the above (with editing, but keeping the examples) in the
>>> kerneldoc to explain why/how to use this? With that the concept at least
>>> has my
>>>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> I'll leave it up to you & Christian to find a prettier color choice for
>>> the naming bikeshed.
>> There is one major issue remaining with this and that is dma_buf_vunmap():
>>
>> void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>>
>> Here we expect the original pointer as returned by dma_buf_map(), otherwise
>> we vunmap() the wrong area!
>>
>> For all TTM based driver this doesn't matter since we keep the vmap base
>> separately in the BO anyway (IIRC), but we had at least one case where this
>> made boom last year.
> Yeah but isn't that the same if it's just a void *?
>
> If you pass the wrong pointer to an unmap function and not exactly what
> you go from the map function, then things go boom. This is like
> complaining that the following code wont work
>
> 	u32 *stuff
>
> 	stuff = kmap_local(some_page);
> 	*stuff++ = 0;
> 	*stuff = 1;
> 	kunmap_locak(stuff);
>
> It's just ... don't do that :-) Also since we pass dma_buf_map by value
> and not by pointer anywhere, the risk of this happening is pretty low
> since you tend to work on a copy. Same with void * pointers really.
>
> Now if people start to pass around struct dma_buf_map * as pointers for
> anything else than out parameters, then we're screwed. But that's like
> passing around void ** for lolz, which is just wrong (except when it's an
> out parameter or actually an array of pointers ofc).
>
> Or I really don't get your concern and you mean something else?

No that's pretty much it. It's just that we hide the pointer inside a 
structure and it is absolutely not obvious to a driver dev that you 
can't do:

dma_buf_vmap(.., &map);
dma_buf_map_inr(&map, x);
dma_buf_vunmap(.., &map);

As bare minimum I strongly suggest that we add some WARN_ONs to the 
framework to check that the pointer given to dma_buf_vunmap() is at 
least page aligned.

Christian.

> -Daniel
>
>
>> Christian.
>>
>>> -Daniel
>>>
>>>> Lucas De Marchi
>>>>
>>>>> -Daniel
>>>>>
>>>>>>> IMO this construct is worse because at a point in time in the function
>>>>>>> the map was pointing to the wrong thing the function was supposed to
>>>>>>> read/write.
>>>>>>>
>>>>>>> It's also useful when the function has double duty, updating a global
>>>>>>> part of the struct and a table inside it (see example in patch 6)
>>>>>>>
>>>>>>> thanks
>>>>>>> Lucas De Marchi
>>>>> -- 
>>>>> Daniel Vetter
>>>>> Software Engineer, Intel Corporation
>>>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0

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

[Daniel Vetter]

On Thu, Jan 27, 2022 at 12:44:21PM +0100, Christian König wrote:
> Am 27.01.22 um 12:16 schrieb Daniel Vetter:
> > On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
> > > Am 27.01.22 um 11:00 schrieb Daniel Vetter:
> > > > On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
> > > > > On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
> > > > > > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> > > > > > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > > > > > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > > > > > > > Well what you propose here can lead to all kind of problems and is
> > > > > > > > > rather bad design as far as I can see.
> > > > > > > > > 
> > > > > > > > > The struct dma_buf_map is only to be filled in by the exporter and
> > > > > > > > > should not be modified in this way by the importer.
> > > > > > > > humn... not sure if I was  clear. There is no importer and exporter here.
> > > > > > > Yeah, and exactly that's what I'm pointing out as problem here.
> > > > > > > 
> > > > > > > You are using the inter driver framework for something internal to the
> > > > > > > driver. That is an absolutely clear NAK!
> > > > > > > 
> > > > > > > We could discuss that, but you guys are just sending around patches to do
> > > > > > > this without any consensus that this is a good idea.
> > > > > > Uh I suggested this, also we're already using dma_buf_map all over the
> > > > > > place as a convenient abstraction. So imo that's all fine, it should allow
> > > > > > drivers to simplify some code where on igpu it's in normal kernel memory
> > > > > > and on dgpu it's behind some pci bar.
> > > > > > 
> > > > > > Maybe we should have a better name for that struct (and maybe also a
> > > > > > better place), but way back when we discussed that bikeshed I didn't come
> > > > > > up with anything better really.
> > > > > I suggest iosys_map since it abstracts access to IO and system memory.
> > > > > 
> > > > > > > > There is a role delegation on filling out and reading a buffer when
> > > > > > > > that buffer represents a struct layout.
> > > > > > > > 
> > > > > > > > struct bla {
> > > > > > > >       int a;
> > > > > > > >       int b;
> > > > > > > >       int c;
> > > > > > > >       struct foo foo;
> > > > > > > >       struct bar bar;
> > > > > > > >       int d;
> > > > > > > > }
> > > > > > > > 
> > > > > > > > 
> > > > > > > > This implementation allows you to have:
> > > > > > > > 
> > > > > > > >       fill_foo(struct dma_buf_map *bla_map) { ... }
> > > > > > > >       fill_bar(struct dma_buf_map *bla_map) { ... }
> > > > > > > > 
> > > > > > > > and the first thing these do is to make sure the map it's pointing to
> > > > > > > > is relative to the struct it's supposed to write/read. Otherwise you're
> > > > > > > > suggesting everything to be relative to struct bla, or to do the same
> > > > > > > > I'm doing it, but IMO more prone to error:
> > > > > > > > 
> > > > > > > >       struct dma_buf_map map = *bla_map;
> > > > > > > >       dma_buf_map_incr(map, offsetof(...));
> > > > > > Wrt the issue at hand I think the above is perfectly fine code. The idea
> > > > > > with dma_buf_map is really that it's just a special pointer, so writing
> > > > > > the code exactly as pointer code feels best. Unfortunately you cannot make
> > > > > > them typesafe (because of C), so the code sometimes looks a bit ugly.
> > > > > > Otherwise we could do stuff like container_of and all that with
> > > > > > typechecking in the macros.
> > > > > 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
> > > > > 
> > > > > 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(..));
> > > > Hm yeah that's a good point that this allows us to rely on the compiler to
> > > > check for uninitialized variables.
> > > > 
> > > > Maybe include the above (with editing, but keeping the examples) in the
> > > > kerneldoc to explain why/how to use this? With that the concept at least
> > > > has my
> > > > 
> > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > 
> > > > I'll leave it up to you & Christian to find a prettier color choice for
> > > > the naming bikeshed.
> > > There is one major issue remaining with this and that is dma_buf_vunmap():
> > > 
> > > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
> > > 
> > > Here we expect the original pointer as returned by dma_buf_map(), otherwise
> > > we vunmap() the wrong area!
> > > 
> > > For all TTM based driver this doesn't matter since we keep the vmap base
> > > separately in the BO anyway (IIRC), but we had at least one case where this
> > > made boom last year.
> > Yeah but isn't that the same if it's just a void *?
> > 
> > If you pass the wrong pointer to an unmap function and not exactly what
> > you go from the map function, then things go boom. This is like
> > complaining that the following code wont work
> > 
> > 	u32 *stuff
> > 
> > 	stuff = kmap_local(some_page);
> > 	*stuff++ = 0;
> > 	*stuff = 1;
> > 	kunmap_locak(stuff);
> > 
> > It's just ... don't do that :-) Also since we pass dma_buf_map by value
> > and not by pointer anywhere, the risk of this happening is pretty low
> > since you tend to work on a copy. Same with void * pointers really.
> > 
> > Now if people start to pass around struct dma_buf_map * as pointers for
> > anything else than out parameters, then we're screwed. But that's like
> > passing around void ** for lolz, which is just wrong (except when it's an
> > out parameter or actually an array of pointers ofc).
> > 
> > Or I really don't get your concern and you mean something else?
> 
> No that's pretty much it. It's just that we hide the pointer inside a
> structure and it is absolutely not obvious to a driver dev that you can't
> do:
> 
> dma_buf_vmap(.., &map);
> dma_buf_map_inr(&map, x);
> dma_buf_vunmap(.., &map);
> 
> As bare minimum I strongly suggest that we add some WARN_ONs to the
> framework to check that the pointer given to dma_buf_vunmap() is at least
> page aligned.

Yeah that might be a good idea. But then we also have to add that check to
dma_buf_vmap, just in case a driver does something really funny :-)
-Daniel

> 
> Christian.
> 
> > -Daniel
> > 
> > 
> > > Christian.
> > > 
> > > > -Daniel
> > > > 
> > > > > Lucas De Marchi
> > > > > 
> > > > > > -Daniel
> > > > > > 
> > > > > > > > IMO this construct is worse because at a point in time in the function
> > > > > > > > the map was pointing to the wrong thing the function was supposed to
> > > > > > > > read/write.
> > > > > > > > 
> > > > > > > > It's also useful when the function has double duty, updating a global
> > > > > > > > part of the struct and a table inside it (see example in patch 6)
> > > > > > > > 
> > > > > > > > thanks
> > > > > > > > Lucas De Marchi
> > > > > > -- 
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

[Thomas Zimmermann]

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

Hi

Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
> 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.
> 
> Add a pair of macros dma_buf_map_read_field()/dma_buf_map_write_field()
> to calculate the offset of a struct member and memcpy the data from/to
> the dma_buf_map. 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 on IO memory or dereference the pointer.
> 
> 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 | 81 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
> 
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 19fa0b5ae5ec..65e927d9ce33 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -6,6 +6,7 @@
>   #ifndef __DMA_BUF_MAP_H__
>   #define __DMA_BUF_MAP_H__
>   
> +#include <linux/kernel.h>
>   #include <linux/io.h>
>   #include <linux/string.h>
>   
> @@ -229,6 +230,46 @@ static inline void dma_buf_map_clear(struct dma_buf_map *map)
>   	}
>   }
>   
> +/**
> + * dma_buf_map_memcpy_to_offset - Memcpy into offset of dma-buf mapping
> + * @dst:	The dma-buf mapping structure
> + * @offset:	The offset from which to copy
> + * @src:	The source buffer
> + * @len:	The number of byte in src
> + *
> + * Copies data into a dma-buf mapping 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 dma_buf_map_memcpy_to_offset(struct dma_buf_map *dst, size_t offset,
> +						const void *src, size_t len)
> +{
> +	if (dst->is_iomem)
> +		memcpy_toio(dst->vaddr_iomem + offset, src, len);
> +	else
> +		memcpy(dst->vaddr + offset, src, len);
> +}

Please don't add a new function. Rather please add the offset parameter 
to dma_buf_map_memcpy_to() and update the callers. There are only two 
calls to dma_buf_map_memcpy_to() within the kernel. To make it clear 
what the offset applies to, I'd call the parameter 'dst_offset'.

> +
> +/**
> + * dma_buf_map_memcpy_from_offset - Memcpy from offset of dma-buf mapping into system memory
> + * @dst:	Destination in system memory
> + * @src:	The dma-buf mapping structure
> + * @src:	The offset from which to copy
> + * @len:	The number of byte in src
> + *
> + * Copies data from a dma-buf mapping 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 dma_buf_map_memcpy_from_offset(void *dst, const struct dma_buf_map *src,
> +						  size_t offset, size_t len)
> +{
> +	if (src->is_iomem)
> +		memcpy_fromio(dst, src->vaddr_iomem + offset, len);
> +	else
> +		memcpy(dst, src->vaddr + offset, len);
> +}
> +

With the dma_buf_map_memcpy_to() changes, please just call this function 
dma_buf_map_memcpy_from().

>   /**
>    * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
>    * @dst:	The dma-buf mapping structure
> @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
>   		map->vaddr += incr;
>   }
>   
> +/**
> + * dma_buf_map_read_field - Read struct member from dma-buf mapping with
> + * arbitrary size and handling un-aligned accesses
> + *
> + * @map__:	The dma-buf mapping structure
> + * @type__:	The struct to be used containing the field to read
> + * @field__:	Member from struct we want to read
> + *
> + * Read a value from dma-buf mapping calculating the offset and size: this assumes
> + * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
> + * or u64 can be read, based on the offset and size of type__.field__.
> + */
> +#define dma_buf_map_read_field(map__, type__, field__) ({				\
> +	type__ *t__;									\
> +	typeof(t__->field__) val__;							\
> +	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
> +				       sizeof(t__->field__));				\
> +	val__;										\
> +})
> +
> +/**
> + * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
> + * arbitrary size and handling un-aligned accesses
> + *
> + * @map__:	The dma-buf mapping structure
> + * @type__:	The struct to be used containing the field to write
> + * @field__:	Member from struct we want to write
> + * @val__:	Value to be written
> + *
> + * Write a value to the dma-buf mapping calculating the offset and size.
> + * A single u8, u16, u32 or u64 can be written based on the offset and size of
> + * type__.field__.
> + */
> +#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
> +	type__ *t__;									\
> +	typeof(t__->field__) val____ = val__;						\
> +	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
> +				     &val____, sizeof(t__->field__));			\
> +})

As the original author of this file, I feel like this shouldn't be here. 
At least not until we have another driver using that pattern.

Best regards
Thomas

> +
>   #endif /* __DMA_BUF_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 02/19] dma-buf-map: Add helper to initialize second map

[Thomas Zimmermann]

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



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'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.

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

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

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

[Thomas Zimmermann]

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

Hi

Am 27.01.22 um 11:21 schrieb Christian König:
> Am 27.01.22 um 11:00 schrieb Daniel Vetter:
>> On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
>>> On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
>>>> On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>>> Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>>> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>>>>>> Well what you propose here can lead to all kind of problems and is
>>>>>>> rather bad design as far as I can see.
>>>>>>>
>>>>>>> The struct dma_buf_map is only to be filled in by the exporter and
>>>>>>> should not be modified in this way by the importer.
>>>>>> humn... not sure if I was  clear. There is no importer and 
>>>>>> exporter here.
>>>>> Yeah, and exactly that's what I'm pointing out as problem here.
>>>>>
>>>>> You are using the inter driver framework for something internal to the
>>>>> driver. That is an absolutely clear NAK!
>>>>>
>>>>> We could discuss that, but you guys are just sending around patches 
>>>>> to do
>>>>> this without any consensus that this is a good idea.
>>>> Uh I suggested this, also we're already using dma_buf_map all over the
>>>> place as a convenient abstraction. So imo that's all fine, it should 
>>>> allow
>>>> drivers to simplify some code where on igpu it's in normal kernel 
>>>> memory
>>>> and on dgpu it's behind some pci bar.
>>>>
>>>> Maybe we should have a better name for that struct (and maybe also a
>>>> better place), but way back when we discussed that bikeshed I didn't 
>>>> come
>>>> up with anything better really.
>>> I suggest iosys_map since it abstracts access to IO and system memory.
>>>
>>>>>> There is a role delegation on filling out and reading a buffer when
>>>>>> that buffer represents a struct layout.
>>>>>>
>>>>>> struct bla {
>>>>>>      int a;
>>>>>>      int b;
>>>>>>      int c;
>>>>>>      struct foo foo;
>>>>>>      struct bar bar;
>>>>>>      int d;
>>>>>> }
>>>>>>
>>>>>>
>>>>>> This implementation allows you to have:
>>>>>>
>>>>>>      fill_foo(struct dma_buf_map *bla_map) { ... }
>>>>>>      fill_bar(struct dma_buf_map *bla_map) { ... }
>>>>>>
>>>>>> and the first thing these do is to make sure the map it's pointing to
>>>>>> is relative to the struct it's supposed to write/read. Otherwise 
>>>>>> you're
>>>>>> suggesting everything to be relative to struct bla, or to do the same
>>>>>> I'm doing it, but IMO more prone to error:
>>>>>>
>>>>>>      struct dma_buf_map map = *bla_map;
>>>>>>      dma_buf_map_incr(map, offsetof(...));
>>>> Wrt the issue at hand I think the above is perfectly fine code. The 
>>>> idea
>>>> with dma_buf_map is really that it's just a special pointer, so writing
>>>> the code exactly as pointer code feels best. Unfortunately you 
>>>> cannot make
>>>> them typesafe (because of C), so the code sometimes looks a bit ugly.
>>>> Otherwise we could do stuff like container_of and all that with
>>>> typechecking in the macros.
>>> 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
>>>
>>> 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(..));
>> Hm yeah that's a good point that this allows us to rely on the 
>> compiler to
>> check for uninitialized variables.
>>
>> Maybe include the above (with editing, but keeping the examples) in the
>> kerneldoc to explain why/how to use this? With that the concept at least
>> has my
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> I'll leave it up to you & Christian to find a prettier color choice for
>> the naming bikeshed.
> 
> There is one major issue remaining with this and that is dma_buf_vunmap():
> 
> void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
> 
> Here we expect the original pointer as returned by dma_buf_map(), 
> otherwise we vunmap() the wrong area!

Indeed. It's always been a problem with that API, even when it still 
took raw pointers.

The IMHO correct solution would distinguish between a buffer (struct 
dma_buf_map) and a pointer into that buffer (struct dma_buf_ptr).

I don't feel like typing that.

Best regards
Thomas

> 
> For all TTM based driver this doesn't matter since we keep the vmap base 
> separately in the BO anyway (IIRC), but we had at least one case where 
> this made boom last year.
> 
> Christian.
> 
>> -Daniel
>>
>>> Lucas De Marchi
>>>
>>>> -Daniel
>>>>
>>>>>> IMO this construct is worse because at a point in time in the 
>>>>>> function
>>>>>> the map was pointing to the wrong thing the function was supposed to
>>>>>> read/write.
>>>>>>
>>>>>> It's also useful when the function has double duty, updating a global
>>>>>> part of the struct and a table inside it (see example in patch 6)
>>>>>>
>>>>>> thanks
>>>>>> Lucas De Marchi
>>>> -- 
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0 
>>>>
> 

-- 
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 09/19] dma-buf-map: Add wrapper over memset

[Thomas Zimmermann]

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

Hi

Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
> Just like memcpy_toio(), there is also need to write a direct value to a
> memory block. Add dma_buf_map_memset() to abstract memset() vs memset_io()
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> 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 | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 3514a859f628..c9fb04264cd0 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -317,6 +317,23 @@ static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void *sr
>   		memcpy(dst->vaddr, src, len);
>   }
>   
> +/**
> + * dma_buf_map_memset - Memset into dma-buf mapping
> + * @dst:	The dma-buf mapping structure
> + * @value:	The value to set
> + * @len:	The number of bytes to set in dst
> + *
> + * Set value in dma-buf mapping. Depending on the buffer's location, the helper
> + * picks the correct method of accessing the memory.
> + */
> +static inline void dma_buf_map_memset(struct dma_buf_map *dst, int value, size_t len)
> +{
> +	if (dst->is_iomem)
> +		memset_io(dst->vaddr_iomem, value, len);
> +	else
> +		memset(dst->vaddr, value, len);
> +}

Maybe add an offset parameter here.

Best regards
Thomas

> +
>   /**
>    * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
>    * @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

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

Re: [Intel-gfx] [PATCH 09/19] dma-buf-map: Add wrapper over memset

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 03:54:21PM +0100, Thomas Zimmermann wrote:
>Hi
>
>Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>Just like memcpy_toio(), there is also need to write a direct value to a
>>memory block. Add dma_buf_map_memset() to abstract memset() vs memset_io()
>>
>>Cc: Matt Roper <matthew.d.roper@intel.com>
>>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 | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>>diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>>index 3514a859f628..c9fb04264cd0 100644
>>--- a/include/linux/dma-buf-map.h
>>+++ b/include/linux/dma-buf-map.h
>>@@ -317,6 +317,23 @@ static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void *sr
>>  		memcpy(dst->vaddr, src, len);
>>  }
>>+/**
>>+ * dma_buf_map_memset - Memset into dma-buf mapping
>>+ * @dst:	The dma-buf mapping structure
>>+ * @value:	The value to set
>>+ * @len:	The number of bytes to set in dst
>>+ *
>>+ * Set value in dma-buf mapping. Depending on the buffer's location, the helper
>>+ * picks the correct method of accessing the memory.
>>+ */
>>+static inline void dma_buf_map_memset(struct dma_buf_map *dst, int value, size_t len)
>>+{
>>+	if (dst->is_iomem)
>>+		memset_io(dst->vaddr_iomem, value, len);
>>+	else
>>+		memset(dst->vaddr, value, len);
>>+}
>
>Maybe add an offset parameter here.

yep, on v2 I will have 2 APIs, one with and one without offset.

thanks
Lucas De Marchi

>
>Best regards
>Thomas
>
>>+
>>  /**
>>   * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
>>   * @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

Re: [Intel-gfx] [PATCH 09/19] dma-buf-map: Add wrapper over memset

[Thomas Zimmermann]

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



Am 27.01.22 um 16:38 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 03:54:21PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>> Just like memcpy_toio(), there is also need to write a direct value to a
>>> memory block. Add dma_buf_map_memset() to abstract memset() vs 
>>> memset_io()
>>>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> 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 | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>>> index 3514a859f628..c9fb04264cd0 100644
>>> --- a/include/linux/dma-buf-map.h
>>> +++ b/include/linux/dma-buf-map.h
>>> @@ -317,6 +317,23 @@ static inline void dma_buf_map_memcpy_to(struct 
>>> dma_buf_map *dst, const void *sr
>>>          memcpy(dst->vaddr, src, len);
>>>  }
>>> +/**
>>> + * dma_buf_map_memset - Memset into dma-buf mapping
>>> + * @dst:    The dma-buf mapping structure
>>> + * @value:    The value to set
>>> + * @len:    The number of bytes to set in dst
>>> + *
>>> + * Set value in dma-buf mapping. Depending on the buffer's location, 
>>> the helper
>>> + * picks the correct method of accessing the memory.
>>> + */
>>> +static inline void dma_buf_map_memset(struct dma_buf_map *dst, int 
>>> value, size_t len)
>>> +{
>>> +    if (dst->is_iomem)
>>> +        memset_io(dst->vaddr_iomem, value, len);
>>> +    else
>>> +        memset(dst->vaddr, value, len);
>>> +}
>>
>> Maybe add an offset parameter here.
> 
> yep, on v2 I will have 2 APIs, one with and one without offset.

Please, no. Just add the parameter here and pass 0 if yo don't need it.

Best regards
Thomas

> 
> thanks
> Lucas De Marchi
> 
>>
>> Best regards
>> Thomas
>>
>>> +
>>>  /**
>>>   * dma_buf_map_incr - Increments the address stored in a dma-buf 
>>> mapping
>>>   * @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 --]

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

[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

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.

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

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

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
>Am 27.01.22 um 11:00 schrieb Daniel Vetter:
>>On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
>>>On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
>>>>On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>>>Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>>>On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>>>>>>Well what you propose here can lead to all kind of problems and is
>>>>>>>rather bad design as far as I can see.
>>>>>>>
>>>>>>>The struct dma_buf_map is only to be filled in by the exporter and
>>>>>>>should not be modified in this way by the importer.
>>>>>>humn... not sure if I was  clear. There is no importer and exporter here.
>>>>>Yeah, and exactly that's what I'm pointing out as problem here.
>>>>>
>>>>>You are using the inter driver framework for something internal to the
>>>>>driver. That is an absolutely clear NAK!
>>>>>
>>>>>We could discuss that, but you guys are just sending around patches to do
>>>>>this without any consensus that this is a good idea.
>>>>Uh I suggested this, also we're already using dma_buf_map all over the
>>>>place as a convenient abstraction. So imo that's all fine, it should allow
>>>>drivers to simplify some code where on igpu it's in normal kernel memory
>>>>and on dgpu it's behind some pci bar.
>>>>
>>>>Maybe we should have a better name for that struct (and maybe also a
>>>>better place), but way back when we discussed that bikeshed I didn't come
>>>>up with anything better really.
>>>I suggest iosys_map since it abstracts access to IO and system memory.
>>>
>>>>>>There is a role delegation on filling out and reading a buffer when
>>>>>>that buffer represents a struct layout.
>>>>>>
>>>>>>struct bla {
>>>>>>     int a;
>>>>>>     int b;
>>>>>>     int c;
>>>>>>     struct foo foo;
>>>>>>     struct bar bar;
>>>>>>     int d;
>>>>>>}
>>>>>>
>>>>>>
>>>>>>This implementation allows you to have:
>>>>>>
>>>>>>     fill_foo(struct dma_buf_map *bla_map) { ... }
>>>>>>     fill_bar(struct dma_buf_map *bla_map) { ... }
>>>>>>
>>>>>>and the first thing these do is to make sure the map it's pointing to
>>>>>>is relative to the struct it's supposed to write/read. Otherwise you're
>>>>>>suggesting everything to be relative to struct bla, or to do the same
>>>>>>I'm doing it, but IMO more prone to error:
>>>>>>
>>>>>>     struct dma_buf_map map = *bla_map;
>>>>>>     dma_buf_map_incr(map, offsetof(...));
>>>>Wrt the issue at hand I think the above is perfectly fine code. The idea
>>>>with dma_buf_map is really that it's just a special pointer, so writing
>>>>the code exactly as pointer code feels best. Unfortunately you cannot make
>>>>them typesafe (because of C), so the code sometimes looks a bit ugly.
>>>>Otherwise we could do stuff like container_of and all that with
>>>>typechecking in the macros.
>>>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
>>>
>>>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(..));
>>Hm yeah that's a good point that this allows us to rely on the compiler to
>>check for uninitialized variables.
>>
>>Maybe include the above (with editing, but keeping the examples) in the
>>kerneldoc to explain why/how to use this? With that the concept at least
>>has my
>>
>>Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>I'll leave it up to you & Christian to find a prettier color choice for
>>the naming bikeshed.
>
>There is one major issue remaining with this and that is dma_buf_vunmap():
>
>void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>
>Here we expect the original pointer as returned by dma_buf_map(), 
>otherwise we vunmap() the wrong area!

yeah... I think the most confusing aspect here is about the name.

	void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);

this function is the implementation of the dma_buf, not dma_buf_map,
which is another thing entirely. I think the rename will be benefitial
for this to be cleared out, because then it's more obvious the shallow
copy of the map is the equivalent of having

	u8 *p = buffer;
	...
	p += 10;

Etc. You can't kfree(p) and expect it to work.

Lucas De Marchi

>For all TTM based driver this doesn't matter since we keep the vmap 
>base separately in the BO anyway (IIRC), but we had at least one case 
>where this made boom last year.
>
>Christian.
>
>>-Daniel
>>
>>>Lucas De Marchi
>>>
>>>>-Daniel
>>>>
>>>>>>IMO this construct is worse because at a point in time in the function
>>>>>>the map was pointing to the wrong thing the function was supposed to
>>>>>>read/write.
>>>>>>
>>>>>>It's also useful when the function has double duty, updating a global
>>>>>>part of the struct and a table inside it (see example in patch 6)
>>>>>>
>>>>>>thanks
>>>>>>Lucas De Marchi
>>>>-- 
>>>>Daniel Vetter
>>>>Software Engineer, Intel Corporation
>>>>https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0
>

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

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 12:44:21PM +0100, Christian König wrote:
>Am 27.01.22 um 12:16 schrieb Daniel Vetter:
>>On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
>>>Am 27.01.22 um 11:00 schrieb Daniel Vetter:
>>>>On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
>>>>>On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
>>>>>>On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
>>>>>>>Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
>>>>>>>>On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
>>>>>>>>>Well what you propose here can lead to all kind of problems and is
>>>>>>>>>rather bad design as far as I can see.
>>>>>>>>>
>>>>>>>>>The struct dma_buf_map is only to be filled in by the exporter and
>>>>>>>>>should not be modified in this way by the importer.
>>>>>>>>humn... not sure if I was  clear. There is no importer and exporter here.
>>>>>>>Yeah, and exactly that's what I'm pointing out as problem here.
>>>>>>>
>>>>>>>You are using the inter driver framework for something internal to the
>>>>>>>driver. That is an absolutely clear NAK!
>>>>>>>
>>>>>>>We could discuss that, but you guys are just sending around patches to do
>>>>>>>this without any consensus that this is a good idea.
>>>>>>Uh I suggested this, also we're already using dma_buf_map all over the
>>>>>>place as a convenient abstraction. So imo that's all fine, it should allow
>>>>>>drivers to simplify some code where on igpu it's in normal kernel memory
>>>>>>and on dgpu it's behind some pci bar.
>>>>>>
>>>>>>Maybe we should have a better name for that struct (and maybe also a
>>>>>>better place), but way back when we discussed that bikeshed I didn't come
>>>>>>up with anything better really.
>>>>>I suggest iosys_map since it abstracts access to IO and system memory.
>>>>>
>>>>>>>>There is a role delegation on filling out and reading a buffer when
>>>>>>>>that buffer represents a struct layout.
>>>>>>>>
>>>>>>>>struct bla {
>>>>>>>>      int a;
>>>>>>>>      int b;
>>>>>>>>      int c;
>>>>>>>>      struct foo foo;
>>>>>>>>      struct bar bar;
>>>>>>>>      int d;
>>>>>>>>}
>>>>>>>>
>>>>>>>>
>>>>>>>>This implementation allows you to have:
>>>>>>>>
>>>>>>>>      fill_foo(struct dma_buf_map *bla_map) { ... }
>>>>>>>>      fill_bar(struct dma_buf_map *bla_map) { ... }
>>>>>>>>
>>>>>>>>and the first thing these do is to make sure the map it's pointing to
>>>>>>>>is relative to the struct it's supposed to write/read. Otherwise you're
>>>>>>>>suggesting everything to be relative to struct bla, or to do the same
>>>>>>>>I'm doing it, but IMO more prone to error:
>>>>>>>>
>>>>>>>>      struct dma_buf_map map = *bla_map;
>>>>>>>>      dma_buf_map_incr(map, offsetof(...));
>>>>>>Wrt the issue at hand I think the above is perfectly fine code. The idea
>>>>>>with dma_buf_map is really that it's just a special pointer, so writing
>>>>>>the code exactly as pointer code feels best. Unfortunately you cannot make
>>>>>>them typesafe (because of C), so the code sometimes looks a bit ugly.
>>>>>>Otherwise we could do stuff like container_of and all that with
>>>>>>typechecking in the macros.
>>>>>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
>>>>>
>>>>>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(..));
>>>>Hm yeah that's a good point that this allows us to rely on the compiler to
>>>>check for uninitialized variables.
>>>>
>>>>Maybe include the above (with editing, but keeping the examples) in the
>>>>kerneldoc to explain why/how to use this? With that the concept at least
>>>>has my
>>>>
>>>>Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>>I'll leave it up to you & Christian to find a prettier color choice for
>>>>the naming bikeshed.
>>>There is one major issue remaining with this and that is dma_buf_vunmap():
>>>
>>>void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>>>
>>>Here we expect the original pointer as returned by dma_buf_map(), otherwise
>>>we vunmap() the wrong area!
>>>
>>>For all TTM based driver this doesn't matter since we keep the vmap base
>>>separately in the BO anyway (IIRC), but we had at least one case where this
>>>made boom last year.
>>Yeah but isn't that the same if it's just a void *?
>>
>>If you pass the wrong pointer to an unmap function and not exactly what
>>you go from the map function, then things go boom. This is like
>>complaining that the following code wont work
>>
>>	u32 *stuff
>>
>>	stuff = kmap_local(some_page);
>>	*stuff++ = 0;
>>	*stuff = 1;
>>	kunmap_locak(stuff);
>>
>>It's just ... don't do that :-) Also since we pass dma_buf_map by value
>>and not by pointer anywhere, the risk of this happening is pretty low
>>since you tend to work on a copy. Same with void * pointers really.
>>
>>Now if people start to pass around struct dma_buf_map * as pointers for
>>anything else than out parameters, then we're screwed. But that's like
>>passing around void ** for lolz, which is just wrong (except when it's an
>>out parameter or actually an array of pointers ofc).
>>
>>Or I really don't get your concern and you mean something else?
>
>No that's pretty much it. It's just that we hide the pointer inside a 
>structure and it is absolutely not obvious to a driver dev that you 
>can't do:
>
>dma_buf_vmap(.., &map);
>dma_buf_map_inr(&map, x);
>dma_buf_vunmap(.., &map);
>
>As bare minimum I strongly suggest that we add some WARN_ONs to the 
>framework to check that the pointer given to dma_buf_vunmap() is at 
>least page aligned.

Agreed, that should cover most of the cases. I can add a patch doing
that.

thanks
Lucas De Marchi

Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

[Lucas De Marchi]

On Thu, Jan 27, 2022 at 03:26:43PM +0100, Thomas Zimmermann wrote:
>Hi
>
>Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>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.
>>
>>Add a pair of macros dma_buf_map_read_field()/dma_buf_map_write_field()
>>to calculate the offset of a struct member and memcpy the data from/to
>>the dma_buf_map. 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 on IO memory or dereference the pointer.
>>
>>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 | 81 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>
>>diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>>index 19fa0b5ae5ec..65e927d9ce33 100644
>>--- a/include/linux/dma-buf-map.h
>>+++ b/include/linux/dma-buf-map.h
>>@@ -6,6 +6,7 @@
>>  #ifndef __DMA_BUF_MAP_H__
>>  #define __DMA_BUF_MAP_H__
>>+#include <linux/kernel.h>
>>  #include <linux/io.h>
>>  #include <linux/string.h>
>>@@ -229,6 +230,46 @@ static inline void dma_buf_map_clear(struct dma_buf_map *map)
>>  	}
>>  }
>>+/**
>>+ * dma_buf_map_memcpy_to_offset - Memcpy into offset of dma-buf mapping
>>+ * @dst:	The dma-buf mapping structure
>>+ * @offset:	The offset from which to copy
>>+ * @src:	The source buffer
>>+ * @len:	The number of byte in src
>>+ *
>>+ * Copies data into a dma-buf mapping 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 dma_buf_map_memcpy_to_offset(struct dma_buf_map *dst, size_t offset,
>>+						const void *src, size_t len)
>>+{
>>+	if (dst->is_iomem)
>>+		memcpy_toio(dst->vaddr_iomem + offset, src, len);
>>+	else
>>+		memcpy(dst->vaddr + offset, src, len);
>>+}
>
>Please don't add a new function. Rather please add the offset 
>parameter to dma_buf_map_memcpy_to() and update the callers. There are 
>only two calls to dma_buf_map_memcpy_to() within the kernel. To make 
>it clear what the offset applies to, I'd call the parameter 
>'dst_offset'.
>
>>+
>>+/**
>>+ * dma_buf_map_memcpy_from_offset - Memcpy from offset of dma-buf mapping into system memory
>>+ * @dst:	Destination in system memory
>>+ * @src:	The dma-buf mapping structure
>>+ * @src:	The offset from which to copy
>>+ * @len:	The number of byte in src
>>+ *
>>+ * Copies data from a dma-buf mapping 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 dma_buf_map_memcpy_from_offset(void *dst, const struct dma_buf_map *src,
>>+						  size_t offset, size_t len)
>>+{
>>+	if (src->is_iomem)
>>+		memcpy_fromio(dst, src->vaddr_iomem + offset, len);
>>+	else
>>+		memcpy(dst, src->vaddr + offset, len);
>>+}
>>+
>
>With the dma_buf_map_memcpy_to() changes, please just call this 
>function dma_buf_map_memcpy_from().
>
>>  /**
>>   * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
>>   * @dst:	The dma-buf mapping structure
>>@@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
>>  		map->vaddr += incr;
>>  }
>>+/**
>>+ * dma_buf_map_read_field - Read struct member from dma-buf mapping with
>>+ * arbitrary size and handling un-aligned accesses
>>+ *
>>+ * @map__:	The dma-buf mapping structure
>>+ * @type__:	The struct to be used containing the field to read
>>+ * @field__:	Member from struct we want to read
>>+ *
>>+ * Read a value from dma-buf mapping calculating the offset and size: this assumes
>>+ * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
>>+ * or u64 can be read, based on the offset and size of type__.field__.
>>+ */
>>+#define dma_buf_map_read_field(map__, type__, field__) ({				\
>>+	type__ *t__;									\
>>+	typeof(t__->field__) val__;							\
>>+	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
>>+				       sizeof(t__->field__));				\
>>+	val__;										\
>>+})
>>+
>>+/**
>>+ * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
>>+ * arbitrary size and handling un-aligned accesses
>>+ *
>>+ * @map__:	The dma-buf mapping structure
>>+ * @type__:	The struct to be used containing the field to write
>>+ * @field__:	Member from struct we want to write
>>+ * @val__:	Value to be written
>>+ *
>>+ * Write a value to the dma-buf mapping calculating the offset and size.
>>+ * A single u8, u16, u32 or u64 can be written based on the offset and size of
>>+ * type__.field__.
>>+ */
>>+#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
>>+	type__ *t__;									\
>>+	typeof(t__->field__) val____ = val__;						\
>>+	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
>>+				     &val____, sizeof(t__->field__));			\
>>+})
>
>As the original author of this file, I feel like this shouldn't be 
>here. At least not until we have another driver using that pattern.

Let me try to clear out the confusion. Then maybe I can extend
the documentation of this function in v2 if I'm able to convince this is
useful here.

This is not about importer/exporter, having this to work cross-driver.
This is about using dma_buf_map (which we are talking about on renaming
to iosys_map or something else) for inner driver
allocations/abstractions. The abstraction added by iosys_map helps on
sharing the same functions we had before.  And this macro here is very
useful when the buffer is described by a struct layout. Example:

	struct bla {
		struct inner inner1;
		struct inner inner2;
		u32 x, y ,z;
	};

Functions that would previously do:

	struct bla *bla = ...;

	bla->x = 100;
	bla->y = 200;
	bla->inner1.inner_inner_field = 30;

Can do the below, having the system/IO memory abstracted away
(calling it iosys_map here instead of dma_buf_map, hopeful it helps):

	struct iosys_map *map = ...;

	iosys_map_write_field(map, struct bla, x, 100);
	iosys_map_write_field(map, struct bla, y, 200);
	iosys_map_write_field(map, struct bla,
			      inner1.inner_inner_field, 30);

When we are using mostly the same map, the individual drivers can add
quick helpers on top. See the ads_blob_write() added in this series,
which guarantees the map it's working on is always the guc->ads_map,
while reducing verbosity to use the API. From patch
"drm/i915/guc: Add read/write helpers for ADS blob":

#define ads_blob_read(guc_, field_)                                    \
        dma_buf_map_read_field(&(guc_)->ads_map, struct __guc_ads_blob, \
                               field_)

#define ads_blob_write(guc_, field_, val_)                             \
        dma_buf_map_write_field(&(guc_)->ads_map, struct __guc_ads_blob,\
                                field_, val_)

So in intel_guc_ads, we can have a lot of:

-	bla->x = 100;
+	ads_blob_write(guc, x, 10);

thanks
Lucas De Marchi

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

[Thomas Zimmermann]

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

Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

[Thomas Zimmermann]

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

Hi

Am 27.01.22 um 17:34 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 03:26:43PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
>>> 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.
>>>
>>> Add a pair of macros dma_buf_map_read_field()/dma_buf_map_write_field()
>>> to calculate the offset of a struct member and memcpy the data from/to
>>> the dma_buf_map. 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 on IO memory or dereference the pointer.
>>>
>>> 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 | 81 +++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 81 insertions(+)
>>>
>>> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>>> index 19fa0b5ae5ec..65e927d9ce33 100644
>>> --- a/include/linux/dma-buf-map.h
>>> +++ b/include/linux/dma-buf-map.h
>>> @@ -6,6 +6,7 @@
>>>  #ifndef __DMA_BUF_MAP_H__
>>>  #define __DMA_BUF_MAP_H__
>>> +#include <linux/kernel.h>
>>>  #include <linux/io.h>
>>>  #include <linux/string.h>
>>> @@ -229,6 +230,46 @@ static inline void dma_buf_map_clear(struct 
>>> dma_buf_map *map)
>>>      }
>>>  }
>>> +/**
>>> + * dma_buf_map_memcpy_to_offset - Memcpy into offset of dma-buf mapping
>>> + * @dst:    The dma-buf mapping structure
>>> + * @offset:    The offset from which to copy
>>> + * @src:    The source buffer
>>> + * @len:    The number of byte in src
>>> + *
>>> + * Copies data into a dma-buf mapping 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 dma_buf_map_memcpy_to_offset(struct dma_buf_map 
>>> *dst, size_t offset,
>>> +                        const void *src, size_t len)
>>> +{
>>> +    if (dst->is_iomem)
>>> +        memcpy_toio(dst->vaddr_iomem + offset, src, len);
>>> +    else
>>> +        memcpy(dst->vaddr + offset, src, len);
>>> +}
>>
>> Please don't add a new function. Rather please add the offset 
>> parameter to dma_buf_map_memcpy_to() and update the callers. There are 
>> only two calls to dma_buf_map_memcpy_to() within the kernel. To make 
>> it clear what the offset applies to, I'd call the parameter 'dst_offset'.
>>
>>> +
>>> +/**
>>> + * dma_buf_map_memcpy_from_offset - Memcpy from offset of dma-buf 
>>> mapping into system memory
>>> + * @dst:    Destination in system memory
>>> + * @src:    The dma-buf mapping structure
>>> + * @src:    The offset from which to copy
>>> + * @len:    The number of byte in src
>>> + *
>>> + * Copies data from a dma-buf mapping 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 dma_buf_map_memcpy_from_offset(void *dst, const 
>>> struct dma_buf_map *src,
>>> +                          size_t offset, size_t len)
>>> +{
>>> +    if (src->is_iomem)
>>> +        memcpy_fromio(dst, src->vaddr_iomem + offset, len);
>>> +    else
>>> +        memcpy(dst, src->vaddr + offset, len);
>>> +}
>>> +
>>
>> With the dma_buf_map_memcpy_to() changes, please just call this 
>> function dma_buf_map_memcpy_from().
>>
>>>  /**
>>>   * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
>>>   * @dst:    The dma-buf mapping structure
>>> @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct 
>>> dma_buf_map *map, size_t incr)
>>>          map->vaddr += incr;
>>>  }
>>> +/**
>>> + * dma_buf_map_read_field - Read struct member from dma-buf mapping 
>>> with
>>> + * arbitrary size and handling un-aligned accesses
>>> + *
>>> + * @map__:    The dma-buf mapping structure
>>> + * @type__:    The struct to be used containing the field to read
>>> + * @field__:    Member from struct we want to read
>>> + *
>>> + * Read a value from dma-buf mapping calculating the offset and 
>>> size: this assumes
>>> + * the dma-buf mapping is aligned with a a struct type__. A single 
>>> u8, u16, u32
>>> + * or u64 can be read, based on the offset and size of type__.field__.
>>> + */
>>> +#define dma_buf_map_read_field(map__, type__, field__) 
>>> ({                \
>>> +    type__ *t__;                                    \
>>> +    typeof(t__->field__) val__;                            \
>>> +    dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, 
>>> field__),    \
>>> +                       sizeof(t__->field__));                \
>>> +    val__;                                        \
>>> +})
>>> +
>>> +/**
>>> + * dma_buf_map_write_field - Write struct member to the dma-buf 
>>> mapping with
>>> + * arbitrary size and handling un-aligned accesses
>>> + *
>>> + * @map__:    The dma-buf mapping structure
>>> + * @type__:    The struct to be used containing the field to write
>>> + * @field__:    Member from struct we want to write
>>> + * @val__:    Value to be written
>>> + *
>>> + * Write a value to the dma-buf mapping calculating the offset and 
>>> size.
>>> + * A single u8, u16, u32 or u64 can be written based on the offset 
>>> and size of
>>> + * type__.field__.
>>> + */
>>> +#define dma_buf_map_write_field(map__, type__, field__, val__) 
>>> ({            \
>>> +    type__ *t__;                                    \
>>> +    typeof(t__->field__) val____ = val__;                        \
>>> +    dma_buf_map_memcpy_to_offset(map__, offsetof(type__, 
>>> field__),            \
>>> +                     &val____, sizeof(t__->field__));            \
>>> +})
>>
>> As the original author of this file, I feel like this shouldn't be 
>> here. At least not until we have another driver using that pattern.
> 
> Let me try to clear out the confusion. Then maybe I can extend
> the documentation of this function in v2 if I'm able to convince this is
> useful here.
> 
> This is not about importer/exporter, having this to work cross-driver.
> This is about using dma_buf_map (which we are talking about on renaming
> to iosys_map or something else) for inner driver
> allocations/abstractions. The abstraction added by iosys_map helps on
> sharing the same functions we had before.  And this macro here is very
> useful when the buffer is described by a struct layout. Example:
> 
>      struct bla {
>          struct inner inner1;
>          struct inner inner2;
>          u32 x, y ,z;
>      };
> 
> Functions that would previously do:
> 
>      struct bla *bla = ...;
> 
>      bla->x = 100;
>      bla->y = 200;
>      bla->inner1.inner_inner_field = 30;
> 
> Can do the below, having the system/IO memory abstracted away
> (calling it iosys_map here instead of dma_buf_map, hopeful it helps):
> 
>      struct iosys_map *map = ...;

Please don't start renaming anything here. If we want to do this, let's 
have a separate mail thread for coloring the bike shed.

> 
>      iosys_map_write_field(map, struct bla, x, 100);
>      iosys_map_write_field(map, struct bla, y, 200);
>      iosys_map_write_field(map, struct bla,
>                    inner1.inner_inner_field, 30);

I don't have strong feelings about these macros. They just seemed not 
needed in general. But I we want to add them here, I 'd like to propose 
a few small changes.

Again, please add an offset parameter for the map's pointer.

Then I'd call them either dma_buf_map_rd/dma_buf_map_wr for read/write 
OR dma_buf_map_ld/dma_buf_map_st for load/store. They should take a C 
type. Something like this

   dma_buf_map_wr(map, offset, int32, 0x01234);
   val = dam_buf_map_rd(map, offset, int32);

Hopefully, that's flexible enough for all users. On top of that, you can 
build additional helpers like dma_buf_map_rd_field() and 
dma_buf_map_wr_field().

Ok?

Best regards
Thomas

> 
> When we are using mostly the same map, the individual drivers can add
> quick helpers on top. See the ads_blob_write() added in this series,
> which guarantees the map it's working on is always the guc->ads_map,
> while reducing verbosity to use the API. From patch
> "drm/i915/guc: Add read/write helpers for ADS blob":
> 
> #define ads_blob_read(guc_, field_)                                    \
>         dma_buf_map_read_field(&(guc_)->ads_map, struct __guc_ads_blob, \
>                                field_)
> 
> #define ads_blob_write(guc_, field_, val_)                             \
>         dma_buf_map_write_field(&(guc_)->ads_map, struct __guc_ads_blob,\
>                                 field_, val_)
> 
> So in intel_guc_ads, we can have a lot of:
> 
> -    bla->x = 100;
> +    ads_blob_write(guc, x, 10);
> 
> thanks
> Lucas De Marchi

-- 
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: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map

[Thomas Zimmermann]

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

Hi

Am 28.01.22 um 09:15 schrieb Thomas Zimmermann:
...
> 
>>
>> 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.

And maybe put all changes to the dma_buf_map interface into a single 
patch. It makes it easier to review and discuss.

Best regards
Thomas

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