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

[Lucas De Marchi]

On Thu, Mar 03, 2022 at 11:30:10PM +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 log object allocated from local memory or system memory
>    and according setup the iosys_map (Lucas)
>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>
>Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com>
>---
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>index a24dc6441872..b9db765627ea 100644
>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>@@ -3,6 +3,7 @@
>  * Copyright © 2014-2019 Intel Corporation
>  */
>
>+#include <drm/drm_cache.h>
> #include <linux/debugfs.h>
> #include <linux/string_helpers.h>
>
>@@ -206,6 +207,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
> 	enum guc_log_buffer_type type;
> 	void *src_data, *dst_data;
> 	bool new_overflow;
>+	struct iosys_map src_map;
>
> 	mutex_lock(&log->relay.lock);
>
>@@ -282,14 +284,21 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
> 		}
>
> 		/* Just copy the newly written data */
>+		if (i915_gem_object_is_lmem(log->vma->obj))
>+			iosys_map_set_vaddr_iomem(&src_map, (void __iomem *)src_data);
>+		else
>+			iosys_map_set_vaddr(&src_map, src_data);

It would be better to keep this outside of the loop. So inside the loop
we can use only iosys_map_incr(&src_map, buffer_size). However you'd
also have to handle the read_offset. The iosys_map_ API has both a
src_offset and dst_offset due to situations like that. Maybe this is
missing in the new drm_memcpy_* function you're adding?

This function was not correct wrt to IO memory access with the other
2 places in this function doing plain memcpy(). Since we are starting to
use iosys_map here, we probably should handle this commit as "migrate to
iosys_map", and convert those. In your current final state
we have 3 variables aliasing the same memory location. IMO it will be
error prone to keep it like that

+Michal, some questions:

- I'm not very familiar with the relayfs API. Is the `dst_data += PAGE_SIZE;`
really correct?

- Could you double check this patch and ack if ok?

Heads up that since the log buffer is potentially in lmem, we will need
to convert this function to take that into account. All those accesses
to log_buf_state need to use the proper kernel abstraction for system vs
I/O memory.

thanks
Lucas De Marchi

>+
> 		if (read_offset > write_offset) {
>-			i915_memcpy_from_wc(dst_data, src_data, write_offset);
>+			drm_memcpy_from_wc_vaddr(dst_data, &src_map,
>+						 write_offset);
> 			bytes_to_copy = buffer_size - read_offset;
> 		} else {
> 			bytes_to_copy = write_offset - read_offset;
> 		}
>-		i915_memcpy_from_wc(dst_data + read_offset,
>-				    src_data + read_offset, bytes_to_copy);
>+		iosys_map_incr(&src_map, read_offset);
>+		drm_memcpy_from_wc_vaddr(dst_data + read_offset, &src_map,
>+					 bytes_to_copy);
>
> 		src_data += buffer_size;
> 		dst_data += buffer_size;
>-- 
>2.25.1
>