Re: [Intel-gfx] [PATCH 5/7] drm/i915/guc: Use streaming loads to speed up dumping the guc log

[John Harrison <john.c.harrison@intel.com>]

On 8/2/2022 11:48, Teres Alexis, Alan Previn wrote:
> One concern below. Else, nice, simple yet good optimization here. :)
>
> In the interest of quicker progression, I will provide a conditional R-B if you can either fix the issue raised below on
> the way in or provide a reason why that's not an issue:
Not an issue, but code changes like that can't be 'fixed on the way in'. 
Tweaking a commit message can potentially happen when merging patches 
but not code changes. For that you have to repost for CI.

John.

>
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>
> On Wed, 2022-07-27 at 19:20 -0700, John.C.Harrison@Intel.com wrote:
>> From: Chris Wilson <chris.p.wilson@intel.com>
>>
>> Use a temporary page and mempy_from_wc to reduce the time it takes to
>> dump the guc log to debugfs.
>>
>> Signed-off-by: Chris Wilson <chris.p.wilson@intel.com>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 24 ++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 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 07d31ae32f765..4722d4b18ed19 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>> @@ -750,8 +750,9 @@ int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p,
>>   	struct intel_guc *guc = log_to_guc(log);
>>   	struct intel_uc *uc = container_of(guc, struct intel_uc, guc);
>>   	struct drm_i915_gem_object *obj = NULL;
>> -	u32 *map;
>> -	int i = 0;
>> +	void *map;
>> +	u32 *page;
>> +	int i, j;
>>   
>>   	if (!intel_guc_is_supported(guc))
>>   		return -ENODEV;
>> @@ -764,23 +765,34 @@ int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p,
>>   	if (!obj)
>>   		return 0;
>>   
>> +	page = (u32 *)__get_free_page(GFP_KERNEL);
>> +	if (!page)
>> +		return -ENOMEM;
> Alan: although unlikely, its possible that user could trigger debugfs mid of a gt reset - not sure if we need to use the
> "uc->reset_in_progress" before calling this allocation and return a different error in that case like EAGAIN or EBUSY or
> ECONNRESET.
Doesn't matter.

The issue of thou shalt not allocate memory during a reset is only 
relevant to code that can be called from within the reset path. As in, 
you must not do something that could block the reset from completing. 
This is only debugfs code. It is not called from within the reset path. 
So if it gets blocked waiting for memory to be released, no-one cares.

John.


>
>> +
>>   	intel_guc_dump_time_info(guc, p);
>>   
>>   	map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WC);
>>   	if (IS_ERR(map)) {
>>   		DRM_DEBUG("Failed to pin object\n");
>>   		drm_puts(p, "(log data unaccessible)\n");
>> +		free_page((unsigned long)page);
>>   		return PTR_ERR(map);
>>   	}
>>   
>> -	for (i = 0; i < obj->base.size / sizeof(u32); i += 4)
>> -		drm_printf(p, "0x%08x 0x%08x 0x%08x 0x%08x\n",
>> -			   *(map + i), *(map + i + 1),
>> -			   *(map + i + 2), *(map + i + 3));
>> +	for (i = 0; i < obj->base.size; i += PAGE_SIZE) {
>> +		if (!i915_memcpy_from_wc(page, map + i, PAGE_SIZE))
>> +			memcpy(page, map + i, PAGE_SIZE);
>> +
>> +		for (j = 0; j < PAGE_SIZE / sizeof(u32); j += 4)
>> +			drm_printf(p, "0x%08x 0x%08x 0x%08x 0x%08x\n",
>> +				   *(page + j + 0), *(page + j + 1),
>> +				   *(page + j + 2), *(page + j + 3));
>> +	}
>>   
>>   	drm_puts(p, "\n");
>>   
>>   	i915_gem_object_unpin_map(obj);
>> +	free_page((unsigned long)page);
>>   
>>   	return 0;
>>   }
>> -- 
>> 2.37.1
>>