On 21.03.2022 14:14, 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
yes, it is a good suggestion to completely change the reading of the GuC
log for the relay to use the iosys_map interfaces.
Though it was planned eventually, doing it now with this series will
avoid mixing of memcpy() and drm_memcpy_*(which needs iosys_map
parameters) functions.
I will do the changes.
>
> +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
> >
