[Intel-gfx] [PATCH] drm/i915/guc: Compact init params debug to a single line

[Daniele Ceraolo Spurio]

On 6/25/19 11:47 AM, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2019-06-25 19:30:18)
>> On Tue, 25 Jun 2019 19:45:47 +0200, Chris Wilson
>> <chris at chris-wilson.co.uk> wrote:
>>
>>> Use hex_dump_to_buffer() to compress the parameter debug into a single
>>> line for less verbose debug logs.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_guc.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>>> b/drivers/gpu/drm/i915/intel_guc.c
>>> index c40a6efdd33a..447f1de15289 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>>> @@ -367,6 +367,7 @@ static u32 guc_ctl_ads_flags(struct intel_guc *guc)
>>>   void intel_guc_init_params(struct intel_guc *guc)
>>>   {
>>>        struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> +     char buf[GUC_CTL_MAX_DWORDS * 10];
>>>        u32 params[GUC_CTL_MAX_DWORDS];
>>>        int i;
>>> @@ -378,8 +379,9 @@ void intel_guc_init_params(struct intel_guc *guc)
>>>        params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
>>>        params[GUC_CTL_ADS] = guc_ctl_ads_flags(guc);
>>> -     for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
>>> -             DRM_DEBUG_DRIVER("param[%2d] = %#x\n", i, params[i]);
>>> +     hex_dump_to_buffer(params, sizeof(params),
>>> +                        32, 4, buf, sizeof(buf), false);
>>
>> hmm, GUC_CTL_MAX_DWORDS is 14, so it will be 56 bytes in total,
>> but hex_dump_to_buffer will dump only 32 bytes ... unless
>> we explicitly limit our dump to currently used just 5 entries

To be pedantic we use 6 entries (0..5), but doesn't really change the 
point :)

>> (20 bytes) but then this might be not future proof if new fw
>> will require/use more then 8 parameters
> 
> Ah, it only does one line. I completely forgot about that.
> 
>>> +     DRM_DEBUG_DRIVER("params[%s]\n", buf);
>>
>> use of [%s] may make this less readable, so maybe:
>>
>>          DRM_DEBUG_DRIVER("GuC params %s\n", buf);
>>
>> but I'm still not sure if we should go that partial way, Daniele ?
> 
> How about,
> 
> if (drm_debug & DRM_UT_DRIVER)
> 	print_hex_dump(KERN_DEBUG, "guc init params: ", 0, 16, 4,
> 		       params, sizeof(params), false);
> 
>> ps. we can aslo use two lines or two bufs for 0..7 and 8..13 params
> 
> Would also be an improvement over 14 :)
> 
> Do we even need to dump them? They are almost all static, with the
> exception of debug level and ads address? Is it useful?

In my experience it can be useful when we get a bug report where guc 
failed to load or when we're testing an interface change to double-check 
that the parameters are set as expected. But I agree there is no need to 
dump all the dwords we don't set. Maybe we can reduce GUC_CTL_MAX_DWORDS 
to the number of used dwords, or add a new define set to that and use it 
for buf size?

Daniele

> -Chris
>