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__ */

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__ */

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__ */

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

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

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

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

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

On 1/26/2022 12:36 PM, Lucas De Marchi wrote:

The ADS initialitazion was using 2 passes to calculate the regset sent to GuC to initialize each engine: the first pass to just have the final object size and the second to set each register in place in the final gem object.

However in order to maintain an ordered set of registers to pass to guc, each register needs to be added and moved in the final array. The second phase may actually happen in IO memory rather than system memory and accessing IO memory by simply dereferencing the pointer doesn't work on all architectures. Other places of the ADS initializaition were converted to use the dma_buf_map API, but here there may be a lot more accesses to IO memory. So, instead of following that same approach, convert the regset initialization to calculate the final array in 1 pass and in the second pass that array is just copied to its final location, updating the pointers for each engine written to the ADS blob.

One important thing is that struct temp_regset now have different semantics: `registers` continues to track the registers of a single engine, however the other fields are updated together, according to the newly added `storage`, which tracks the memory allocated for all the registers. So rename some of these fields and add a __mmio_reg_add(): this function (possibly) allocates memory and operates on the storage pointer while guc_mmio_reg_add() continues to manage the registers pointer.

On a Tiger Lake system using enable_guc=3, the following log message is now seen:

[ 187.334310] i915 0000:00:02.0: [drm:intel_guc_ads_create [i915]] Used 4 KB for temporary ADS regset

This change has also been tested on an ARM64 host with DG2 and other discrete graphics cards.

Cc: Matt Roper matthew.d.roper@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Daniel Vetter daniel@ffwll.ch Cc: John Harrison John.C.Harrison@Intel.com Cc: Matthew Brost matthew.brost@intel.com Cc: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 7 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 117 +++++++++++++-------- 2 files changed, 79 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index e2e0df1c3d91..4c852eee3ad8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -152,6 +152,13 @@ struct intel_guc { struct dma_buf_map ads_map; /** @ads_regset_size: size of the save/restore regsets in the ADS */ u32 ads_regset_size;

• /**

• * @ads_regset_count: number of save/restore registers in the ADS for
  

• * each engine
  

• */
  

• u32 ads_regset_count[I915_NUM_ENGINES];
• /** @ads_regset: save/restore regsets in the ADS */
• struct guc_mmio_reg *ads_regset; /** @ads_golden_ctxt_size: size of the golden contexts in the ADS */ u32 ads_golden_ctxt_size; /** @ads_engine_usage_size: size of engine usage in the ADS */

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c index 73ca34de44f7..390101ee3661 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c @@ -226,14 +226,13 @@ static void guc_mapping_table_init(struct intel_gt *gt,

/*

• The save/restore register list must be pre-calculated to a temporary

• • buffer of driver defined size before it can be generated in place
• • inside the ADS.

• • buffer before it can be copied inside the ADS.
  */

-#define MAX_MMIO_REGS 128 /* Arbitrary size, increase as needed */ struct temp_regset { struct guc_mmio_reg *registers;

• u32 used;
• u32 size;

• struct guc_mmio_reg *storage;

I think this could use a comment to distinguish between registers and storage. Something like.:

/* ptr to the base of the allocated storage for all engines */ struct guc_mmio_reg *storage;

/* ptr to the section of the storage for the engine currently being worked on */ struct guc_mmio_reg *registers;

• u32 storage_used;

• u32 storage_max; };

  static int guc_mmio_reg_cmp(const void *a, const void *b)

@@ -244,18 +243,44 @@ static int guc_mmio_reg_cmp(const void *a, const void *b) return (int)ra->offset - (int)rb->offset; }

+static struct guc_mmio_reg * __must_check +__mmio_reg_add(struct temp_regset *regset, struct guc_mmio_reg *reg) +{

• u32 pos = regset->storage_used;
• struct guc_mmio_reg *slot;
• if (pos >= regset->storage_max) {

• size_t size = ALIGN((pos + 1) * sizeof(*slot), PAGE_SIZE);
  

• struct guc_mmio_reg *r = krealloc(regset->storage,
  

• 				  size, GFP_KERNEL);
  

• if (!r) {
  

• 	WARN_ONCE(1, "Incomplete regset list: can't add register (%d)\n",
  

• 		  -ENOMEM);
  

• 	return ERR_PTR(-ENOMEM);
  

• }
  

• regset->registers = r + (regset->registers - regset->storage);
  

• regset->storage = r;
  

• regset->storage_max = size / sizeof(*slot);
  

• }
• slot = &regset->storage[pos];
• regset->storage_used++;
• *slot = *reg;
• return slot;

+}

• static long __must_check guc_mmio_reg_add(struct temp_regset *regset, u32 offset, u32 flags) {

• u32 count = regset->used;

• u32 count = regset->storage_used - (regset->registers - regset->storage); struct guc_mmio_reg reg = { .offset = offset, .flags = flags, }; struct guc_mmio_reg *slot;

• GEM_BUG_ON(count >= regset->size);
• /*
  • The mmio list is built using separate lists within the driver.
  • It's possible that at some point we may attempt to add the same

@@ -266,9 +291,9 @@ static long __must_check guc_mmio_reg_add(struct temp_regset *regset, sizeof(reg), guc_mmio_reg_cmp)) return 0;

• slot = &regset->registers[count];
• regset->used++;
• *slot = reg;

• slot = __mmio_reg_add(regset, &reg);

• if (IS_ERR(slot))

• return PTR_ERR(slot);
  
  

  while (slot-- > regset->registers) { GEM_BUG_ON(slot[0].offset == slot[1].offset);

@@ -295,7 +320,11 @@ static int guc_mmio_regset_init(struct temp_regset *regset, unsigned int i; int ret = 0;

• regset->used = 0;

• /*

• * Each engine's registers point to a new start relative to
  

• * storage
  

• */
  

• regset->registers = regset->storage + regset->storage_used;

  ret |= GUC_MMIO_REG_ADD(regset, RING_MODE_GEN7(base), true); ret |= GUC_MMIO_REG_ADD(regset, RING_HWS_PGA(base), false);

@@ -317,32 +346,28 @@ static int guc_mmio_regset_init(struct temp_regset *regset, return ret ? -1 : 0; }

-static int guc_mmio_reg_state_query(struct intel_guc *guc) +static long guc_mmio_reg_state_create(struct intel_guc *guc) { struct intel_gt *gt = guc_to_gt(guc); struct intel_engine_cs *engine; enum intel_engine_id id;

• struct temp_regset temp_set;
• u32 total;

• struct temp_regset temp_set = {};
• long total = 0;

• /*

• * Need to actually build the list in order to filter out
  

• * duplicates and other such data dependent constructions.
  

• */
  

• temp_set.size = MAX_MMIO_REGS;
• temp_set.registers = kmalloc_array(temp_set.size,

• 			   sizeof(*temp_set.registers),
  

• 			   GFP_KERNEL);
  

• if (!temp_set.registers)

• return -ENOMEM;
  

• total = 0; for_each_engine(engine, gt, id) {

• guc_mmio_regset_init(&temp_set, engine);
  

• total += temp_set.used;
  

• u32 used = temp_set.storage_used;
  

• if (guc_mmio_regset_init(&temp_set, engine) < 0)
  

• 	return -1;
  

If you fail here you're leaking temp_set.storage. Also, any reason not to just return the return code from guc_mmio_regset_init?

Apart from these minor comments, the change LGTM. IMO we could also merge this patch on its own ahead of the rest of the dma_buf code, because not having to recreate the regset on every reset/resume is still helpful.

Daniele

• guc->ads_regset_count[id] = temp_set.storage_used - used;
  

• total += guc->ads_regset_count[id];
  

  }

• kfree(temp_set.registers);

• guc->ads_regset = temp_set.storage;

• drm_dbg(&guc_to_gt(guc)->i915->drm, "Used %lu KB for temporary ADS regset\n",

• (temp_set.storage_max * sizeof(struct guc_mmio_reg)) >> 10);
  
  

  return total * sizeof(struct guc_mmio_reg); }

@@ -352,40 +377,38 @@ static void guc_mmio_reg_state_init(struct intel_guc *guc, { struct intel_gt *gt = guc_to_gt(guc); struct intel_engine_cs *engine;

• struct guc_mmio_reg *ads_registers; enum intel_engine_id id;

• struct temp_regset temp_set;

• struct guc_mmio_reg_set *ads_reg_set; u32 addr_ggtt, offset;

• u8 guc_class;

  offset = guc_ads_regset_offset(guc); addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;

• temp_set.registers = (struct guc_mmio_reg *)(((u8 *)blob) + offset);

• temp_set.size = guc->ads_regset_size / sizeof(temp_set.registers[0]);

• ads_registers = (struct guc_mmio_reg *)(((u8 *)blob) + offset);

• memcpy(ads_registers, guc->ads_regset, guc->ads_regset_size);

  for_each_engine(engine, gt, id) {

• u32 count = guc->ads_regset_count[id];
  

• struct guc_mmio_reg_set *ads_reg_set;
  

• u8 guc_class;
  

• /* Class index is checked in class converter */ GEM_BUG_ON(engine->instance >= GUC_MAX_INSTANCES_PER_CLASS);

  guc_class = engine_class_to_guc_class(engine->class); ads_reg_set = &blob->ads.reg_state_list[guc_class][engine->instance];

• guc_mmio_regset_init(&temp_set, engine);
  

• if (!temp_set.used) {
  

• if (!count) {
  ads_reg_set->address = 0;
  ads_reg_set->count = 0;
  continue;
  

  }

  ads_reg_set->address = addr_ggtt;

• ads_reg_set->count = temp_set.used;
  

• ads_reg_set->count = count;
  
  

• temp_set.size -= temp_set.used;
  

• temp_set.registers += temp_set.used;
  

• addr_ggtt += temp_set.used * sizeof(struct guc_mmio_reg);
  

• addr_ggtt += count * sizeof(struct guc_mmio_reg);
  

  }

• GEM_BUG_ON(temp_set.size); }

  static void fill_engine_enable_masks(struct intel_gt *gt,

@@ -634,8 +657,11 @@ int intel_guc_ads_create(struct intel_guc *guc)

GEM_BUG_ON(guc->ads_vma);

• /* Need to calculate the reg state size dynamically: */
• ret = guc_mmio_reg_state_query(guc);

• /*

• * Create reg state size dynamically on system memory to be copied to
  

• * the final ads blob on gt init/reset
  

• */
  

• ret = guc_mmio_reg_state_create(guc); if (ret < 0) return ret; guc->ads_regset_size = ret;

@@ -681,6 +707,7 @@ void intel_guc_ads_destroy(struct intel_guc *guc) i915_vma_unpin_and_release(&guc->ads_vma, I915_VMA_RELEASE_MAP); guc->ads_blob = NULL; dma_buf_map_clear(&guc->ads_map);

• kfree(guc->ads_regset); }

  static void guc_ads_private_data_reset(struct intel_guc *guc)