[Intel-gfx] [PATCH v2 5/7] drm/i915/selftests: use the memcpy_from_wc call from the drm

[Lucas De Marchi]

Now Cc'ing Daniel properly

Lucas De Marchi

On Mon, Mar 21, 2022 at 04:00:56PM -0700, Lucas De Marchi wrote:
>+Thomas Zimmermann and +Daniel Vetter
>
>Could you take a look below regarding the I/O to I/O memory access?
>
>On Thu, Mar 03, 2022 at 11:30:11PM +0530, Balasubramani Vivekanandan wrote:
>>memcpy_from_wc functions in i915_memcpy.c will be removed and replaced
>>by the implementation in drm_cache.c.
>>Updated to use the functions provided by drm_cache.c.
>>
>>v2: check if the source and destination memory address is from local
>>   memory or system memory and initialize the iosys_map accordingly
>>   (Lucas)
>>
>>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>>Cc: Matthew Auld <matthew.auld at intel.com>
>>Cc: Thomas Hellstr_m <thomas.hellstrom at linux.intel.com>
>>
>>Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com>
>>---
>>.../drm/i915/selftests/intel_memory_region.c  | 41 +++++++++++++------
>>1 file changed, 28 insertions(+), 13 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
>>index ba32893e0873..d16ecb905f3b 100644
>>--- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
>>+++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
>>@@ -7,6 +7,7 @@
>>#include <linux/sort.h>
>>
>>#include <drm/drm_buddy.h>
>>+#include <drm/drm_cache.h>
>>
>>#include "../i915_selftest.h"
>>
>>@@ -1133,7 +1134,7 @@ static const char *repr_type(u32 type)
>>
>>static struct drm_i915_gem_object *
>>create_region_for_mapping(struct intel_memory_region *mr, u64 size, u32 type,
>>-			  void **out_addr)
>>+			  struct iosys_map *out_addr)
>>{
>>	struct drm_i915_gem_object *obj;
>>	void *addr;
>>@@ -1153,7 +1154,11 @@ create_region_for_mapping(struct intel_memory_region *mr, u64 size, u32 type,
>>		return addr;
>>	}
>>
>>-	*out_addr = addr;
>>+	if (i915_gem_object_is_lmem(obj))
>>+		iosys_map_set_vaddr_iomem(out_addr, (void __iomem *)addr);
>>+	else
>>+		iosys_map_set_vaddr(out_addr, addr);
>>+
>>	return obj;
>>}
>>
>>@@ -1164,24 +1169,33 @@ static int wrap_ktime_compare(const void *A, const void *B)
>>	return ktime_compare(*a, *b);
>>}
>>
>>-static void igt_memcpy_long(void *dst, const void *src, size_t size)
>>+static void igt_memcpy_long(struct iosys_map *dst, struct iosys_map *src,
>>+			    size_t size)
>>{
>>-	unsigned long *tmp = dst;
>>-	const unsigned long *s = src;
>>+	unsigned long *tmp = dst->is_iomem ?
>>+				(unsigned long __force *)dst->vaddr_iomem :
>>+				dst->vaddr;
>
>if we access vaddr_iomem/vaddr we basically break the promise of
>abstracting system and I/O memory. There is no point in receiving
>struct iosys_map as argument and then break the abstraction.
>
>>+	const unsigned long *s = src->is_iomem ?
>>+				(unsigned long __force *)src->vaddr_iomem :
>>+				src->vaddr;
>>
>>	size = size / sizeof(unsigned long);
>>	while (size--)
>>		*tmp++ = *s++;
>
>
>so we basically want to copy from one place to the other on a word
>boundary. And it may be
>
>	a) I/O -> I/O or
>	b) system -> I/O or
>	c) I/O -> system
>
>(b) and (c) should work, but AFAICS (a) is not possible with the current
>iosys-map API. Not even the underlying APIs have that abstracted. Both
>memcpy_fromio() and memcpy_toio() expect one of them to be RAM (system
>memory)
>
>I remember seeing people using a temporary in buffer in system memory
>for proxying the copy. But maybe we need an abstraction for that?
>Also adding Thomas Zimmermann here for that question.
>
>and since this is a selftest testing the performance of the memcpy from
>one memory region to the other, it would be good to have this test
>executed to a) make sure it still works and b) record in the commit
>message any possible slow down we are incurring.
>
>thanks
>Lucas De Marchi
>
>
>>}
>>
>>-static inline void igt_memcpy(void *dst, const void *src, size_t size)
>>+static inline void igt_memcpy(struct iosys_map *dst, struct iosys_map *src,
>>+			      size_t size)
>>{
>>-	memcpy(dst, src, size);
>>+	memcpy(dst->is_iomem ? (void __force *)dst->vaddr_iomem : dst->vaddr,
>>+	       src->is_iomem ? (void __force *)src->vaddr_iomem : src->vaddr,
>>+	       size);
>>}
>>
>>-static inline void igt_memcpy_from_wc(void *dst, const void *src, size_t size)
>>+static inline void igt_memcpy_from_wc(struct iosys_map *dst, struct iosys_map *src,
>>+				      size_t size)
>>{
>>-	i915_memcpy_from_wc(dst, src, size);
>>+	drm_memcpy_from_wc(dst, src, size);
>>}
>>
>>static int _perf_memcpy(struct intel_memory_region *src_mr,
>>@@ -1191,7 +1205,8 @@ static int _perf_memcpy(struct intel_memory_region *src_mr,
>>	struct drm_i915_private *i915 = src_mr->i915;
>>	const struct {
>>		const char *name;
>>-		void (*copy)(void *dst, const void *src, size_t size);
>>+		void (*copy)(struct iosys_map *dst, struct iosys_map *src,
>>+			     size_t size);
>>		bool skip;
>>	} tests[] = {
>>		{
>>@@ -1205,11 +1220,11 @@ static int _perf_memcpy(struct intel_memory_region *src_mr,
>>		{
>>			"memcpy_from_wc",
>>			igt_memcpy_from_wc,
>>-			!i915_has_memcpy_from_wc(),
>>+			!drm_memcpy_fastcopy_supported(),
>>		},
>>	};
>>	struct drm_i915_gem_object *src, *dst;
>>-	void *src_addr, *dst_addr;
>>+	struct iosys_map src_addr, dst_addr;
>>	int ret = 0;
>>	int i;
>>
>>@@ -1237,7 +1252,7 @@ static int _perf_memcpy(struct intel_memory_region *src_mr,
>>
>>			t0 = ktime_get();
>>
>>-			tests[i].copy(dst_addr, src_addr, size);
>>+			tests[i].copy(&dst_addr, &src_addr, size);
>>
>>			t1 = ktime_get();
>>			t[pass] = ktime_sub(t1, t0);
>>-- 
>>2.25.1
>>