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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

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