On 3/21/2022 2:14 PM, Lucas De Marchi wrote:
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@xxxxxxxxx>
Signed-off-by: Balasubramani Vivekanandan
<balasubramani.vivekanandan@xxxxxxxxx>
---
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?
This is a bit weird due to how i915 uses the relay for the GuC logs, but
it should be functionally correct. Each relay buffer is the same size of
the GuC log buffer in i915 (which is guaranteed to be greater than
PAGE_SIZE) and we always switch to a new relay buffer each time we dump
new data, so we're guaranteed to have the space we need. We do some
pointer magic because instead of just blindly copying the whole local
log buffer to the relay buffer, we copy the header (which is in the
first page) first, then we copy the rest of the logs (2nd page and
onwards) based on what the header tells us has been filled out.
- Could you double check this patch and ack if ok?
The approach looks good to me, but I agree that at this point we might
as well do a full conversion to iosys map. As you already mentioned, the
memcpy that copies the header would also need to be updated for that,
because it accesses the same memory as src_data, while the other memcpy
is from the local copy of the header to the relay, so it should be safe
to not convert.
Daniele
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
