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

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

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@intel.com>
> > 
> > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@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
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
> >