On 21.03.2022 22: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
>
> +Michal, some questions:
@Lucas, better to ask Alan who is making some changes around GuC log
@Alan, can you help answer below questions?
thanks,
Michal
>
> - 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
>>
