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

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

[Chris Wilson]

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@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@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);
+	DRM_DEBUG_DRIVER("params[%s]\n", buf);
 
 	/*
 	 * All SOFT_SCRATCH registers are in FORCEWAKE_BLITTER domain and
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Chris Wilson (2019-06-26 00:08:06)
> Use hex_dump_to_buffer() to compress the parameter debug into a single
> line for less verbose debug logs.

Please excuse the noise, git send-email typo.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/guc: Compact init params debug to a single line (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Compact init params debug to a single line (rev2)
URL   : https://patchwork.freedesktop.org/series/62725/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6351 -> Patchwork_13427
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/

Known issues
------------

  Here are the changes found in Patchwork_13427 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6351/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries:
    - fi-ilk-650:         [DMESG-WARN][3] ([fdo#106387]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6351/fi-ilk-650/igt@debugfs_test@read_all_entries.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/fi-ilk-650/igt@debugfs_test@read_all_entries.html

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u3:          [INCOMPLETE][5] ([fdo#107713] / [fdo#109100]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6351/fi-icl-u3/igt@gem_ctx_create@basic-files.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/fi-icl-u3/igt@gem_ctx_create@basic-files.html

  * igt@gem_ctx_switch@basic-default:
    - fi-icl-guc:         [INCOMPLETE][7] ([fdo#107713] / [fdo#108569]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6351/fi-icl-guc/igt@gem_ctx_switch@basic-default.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/fi-icl-guc/igt@gem_ctx_switch@basic-default.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][9] ([fdo#109485]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6351/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (51 -> 46)
------------------------------

  Additional (2): fi-gdg-551 fi-bwr-2160 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6351 -> Patchwork_13427

  CI_DRM_6351: 841a5e9b5d0b617cea39d2de4492cf3474d3f555 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5068: 15ad664534413628f06c0f172aac11598bfdb895 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13427: e6b763b0578bb1755867bf841e5bcc0893d7c368 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e6b763b0578b drm/i915/guc: Compact init params debug to a single line

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13427/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniele Ceraolo Spurio]

On 6/25/19 1:11 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2019-06-25 21:06:10)
>>
>>
>> On 6/25/19 11:47 AM, Chris Wilson wrote:
>>> 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?
> 
> If you have used them, then by definition they are useful :)
> 
> If you know when you need the information, e.g. the init sequence
> failed; then dump then?
> -Chris
> 

Having them dumped only on guc loading error sounds ok to me, but we 
might have to store them somewhere, because once we've started the load 
GuC is free to overwrite those registers to return information to us. I 
think at the moment it only writes to SCRATCH_13 and SCRATCH_15, which 
we do not reach with the parameters, but better to be future proof.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-06-25 21:06:10)
> 
> 
> On 6/25/19 11:47 AM, Chris Wilson wrote:
> > 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?

If you have used them, then by definition they are useful :)

If you know when you need the information, e.g. the init sequence
failed; then dump then?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@chris-wilson.co.uk>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Michal Wajdeczko (2019-06-25 19:30:18)
> On Tue, 25 Jun 2019 19:45:47 +0200, Chris Wilson  
> <chris@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@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@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
> (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?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Tue, 25 Jun 2019 19:45:47 +0200, Chris Wilson  
<chris@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@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@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
(20 bytes) but then this might be not future proof if new fw
will require/use more then 8 parameters

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

Michal

ps. we can aslo use two lines or two bufs for 0..7 and 8..13 params

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@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);
+	DRM_DEBUG_DRIVER("params[%s]\n", buf);
 
 	/*
 	 * All SOFT_SCRATCH registers are in FORCEWAKE_BLITTER domain and
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx