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

[Michal Wajdeczko]

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 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:

@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
>>