[v3,04/12] drm/i915/guc: Add description and comments about guc_log_level parameter
diff mbox

Message ID 1515082914-4111-5-git-send-email-sagar.a.kamble@intel.com
State New
Headers show

Commit Message

sagar.a.kamble@intel.com Jan. 4, 2018, 4:21 p.m. UTC
guc_log_level parameter takes effect when GuC is loaded which is
controlled through enable_guc parameter. Add this relation info.
in parameter description and documentation.
Earlier, this patch was added to sanitize guc_log_level like old
GuC parameters enable_guc_loading/submission. With new parameter
enable_guc, sanitization of guc_log_level is no more needed.

v2: Added documentation to intel_guc_log.c and param description
about GuC loading dependency. (Michal Wajdeczko)

v3: Removed sanitization of module parameter guc_log_level.
Previous review comments not applicable now.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
---
 drivers/gpu/drm/i915/i915_params.c   | 3 ++-
 drivers/gpu/drm/i915/intel_guc_log.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Michal Wajdeczko Jan. 4, 2018, 4:52 p.m. UTC | #1
On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> guc_log_level parameter takes effect when GuC is loaded which is
> controlled through enable_guc parameter. Add this relation info.
                                                                  ^^^
Extra "."

> in parameter description and documentation.
> Earlier, this patch was added to sanitize guc_log_level like old
> GuC parameters enable_guc_loading/submission. With new parameter
> enable_guc, sanitization of guc_log_level is no more needed.

Hmm, I think we still need to sanitize log_level if it was wrongly
enabled without enabling GuC first (in intel_uc_sanitize_options).

>
> v2: Added documentation to intel_guc_log.c and param description
> about GuC loading dependency. (Michal Wajdeczko)
>
> v3: Removed sanitization of module parameter guc_log_level.
> Previous review comments not applicable now.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
> ---
>  drivers/gpu/drm/i915/i915_params.c   | 3 ++-
>  drivers/gpu/drm/i915/intel_guc_log.c | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c  
> b/drivers/gpu/drm/i915/i915_params.c
> index b5f3eb4..a93a6ca 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>  	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
> i915_param_named(guc_log_level, int, 0400,
> -	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> +	"GuC firmware logging level. This takes effect only if GuC is to be "
> +	"loaded (depends on enable_guc) (-1:disabled (default), 0-3:enabled)");

Btw, I was planing to change above values to follow schema used in other  
modparams:

-1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC)
  0: disabled
  1: enabled (legacy level 0)
  2: enabled (legacy level 1)
  3: enabled (legacy level 2)
  4: enabled (legacy level 3)

So now I'm not sure that I want your patch ;)

> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>  	"GuC firmware path to use instead of the default one");
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 59a9021..d0131bc 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -34,6 +34,7 @@
>   * DOC: GuC firmware log
>   *
>   * Firmware log is enabled by setting i915.guc_log_level to  
> non-negative level.
> + * This takes effect only if GuC is to be loaded based on enable_guc.

... based on i915.enable_guc modparam.

>   * Log data is printed out via reading debugfs i915_guc_log_dump.  
> Reading from
>   * i915_guc_load_status will print out firmware loading status and  
> scratch
>   * registers value.
sagar.a.kamble@intel.com Jan. 5, 2018, 4:54 a.m. UTC | #2
On 1/4/2018 10:22 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> guc_log_level parameter takes effect when GuC is loaded which is
>> controlled through enable_guc parameter. Add this relation info.
> ^^^
> Extra "."
>
>> in parameter description and documentation.
>> Earlier, this patch was added to sanitize guc_log_level like old
>> GuC parameters enable_guc_loading/submission. With new parameter
>> enable_guc, sanitization of guc_log_level is no more needed.
>
> Hmm, I think we still need to sanitize log_level if it was wrongly
> enabled without enabling GuC first (in intel_uc_sanitize_options).
I think it would not be harmful as all decisions based on it will be 
gated by USES_GUC.
>
>>
>> v2: Added documentation to intel_guc_log.c and param description
>> about GuC loading dependency. (Michal Wajdeczko)
>>
>> v3: Removed sanitization of module parameter guc_log_level.
>> Previous review comments not applicable now.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
>> ---
>>  drivers/gpu/drm/i915/i915_params.c   | 3 ++-
>>  drivers/gpu/drm/i915/intel_guc_log.c | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index b5f3eb4..a93a6ca 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>>      "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>> i915_param_named(guc_log_level, int, 0400,
>> -    "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>> +    "GuC firmware logging level. This takes effect only if GuC is to 
>> be "
>> +    "loaded (depends on enable_guc) (-1:disabled (default), 
>> 0-3:enabled)");
>
> Btw, I was planing to change above values to follow schema used in 
> other modparams:
>
> -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC)
>  0: disabled
>  1: enabled (legacy level 0)
>  2: enabled (legacy level 1)
>  3: enabled (legacy level 2)
>  4: enabled (legacy level 3)
>
> So now I'm not sure that I want your patch ;)
>
Makes sense. Will drop this patch.

Thanks
Sagar
>> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>>      "GuC firmware path to use instead of the default one");
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 59a9021..d0131bc 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -34,6 +34,7 @@
>>   * DOC: GuC firmware log
>>   *
>>   * Firmware log is enabled by setting i915.guc_log_level to 
>> non-negative level.
>> + * This takes effect only if GuC is to be loaded based on enable_guc.
>
> ... based on i915.enable_guc modparam.
>
>>   * Log data is printed out via reading debugfs i915_guc_log_dump. 
>> Reading from
>>   * i915_guc_load_status will print out firmware loading status and 
>> scratch
>>   * registers value.
sagar.a.kamble@intel.com Jan. 5, 2018, 8:53 a.m. UTC | #3
On 1/5/2018 10:24 AM, Sagar Arun Kamble wrote:
>
>
> On 1/4/2018 10:22 PM, Michal Wajdeczko wrote:
>> On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble 
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> guc_log_level parameter takes effect when GuC is loaded which is
>>> controlled through enable_guc parameter. Add this relation info.
>> ^^^
>> Extra "."
>>
>>> in parameter description and documentation.
>>> Earlier, this patch was added to sanitize guc_log_level like old
>>> GuC parameters enable_guc_loading/submission. With new parameter
>>> enable_guc, sanitization of guc_log_level is no more needed.
>>
>> Hmm, I think we still need to sanitize log_level if it was wrongly
>> enabled without enabling GuC first (in intel_uc_sanitize_options).
> I think it would not be harmful as all decisions based on it will be 
> gated by USES_GUC.
I was wrong. i915_guc_log_register/unregister were changed by my series 
to only rely on guc_log_level.
I have added HAS_GUC check in those function in v4 patch. Is that option 
okay or we should sanitize this parameter?
>>
>>>
>>> v2: Added documentation to intel_guc_log.c and param description
>>> about GuC loading dependency. (Michal Wajdeczko)
>>>
>>> v3: Removed sanitization of module parameter guc_log_level.
>>> Previous review comments not applicable now.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
>>> ---
>>>  drivers/gpu/drm/i915/i915_params.c   | 3 ++-
>>>  drivers/gpu/drm/i915/intel_guc_log.c | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>>> b/drivers/gpu/drm/i915/i915_params.c
>>> index b5f3eb4..a93a6ca 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>>>      "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>>> i915_param_named(guc_log_level, int, 0400,
>>> -    "GuC firmware logging level (-1:disabled (default), 
>>> 0-3:enabled)");
>>> +    "GuC firmware logging level. This takes effect only if GuC is 
>>> to be "
>>> +    "loaded (depends on enable_guc) (-1:disabled (default), 
>>> 0-3:enabled)");
>>
>> Btw, I was planing to change above values to follow schema used in 
>> other modparams:
>>
>> -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC)
>>  0: disabled
>>  1: enabled (legacy level 0)
>>  2: enabled (legacy level 1)
>>  3: enabled (legacy level 2)
>>  4: enabled (legacy level 3)
>>
>> So now I'm not sure that I want your patch ;)
>>
> Makes sense. Will drop this patch.
>
> Thanks
> Sagar
>>> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>>>      "GuC firmware path to use instead of the default one");
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>> index 59a9021..d0131bc 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>> @@ -34,6 +34,7 @@
>>>   * DOC: GuC firmware log
>>>   *
>>>   * Firmware log is enabled by setting i915.guc_log_level to 
>>> non-negative level.
>>> + * This takes effect only if GuC is to be loaded based on enable_guc.
>>
>> ... based on i915.enable_guc modparam.
>>
>>>   * Log data is printed out via reading debugfs i915_guc_log_dump. 
>>> Reading from
>>>   * i915_guc_load_status will print out firmware loading status and 
>>> scratch
>>>   * registers value.
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b5f3eb4..a93a6ca 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -155,7 +155,8 @@  struct i915_params i915_modparams __read_mostly = {
 	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
 
 i915_param_named(guc_log_level, int, 0400,
-	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
+	"GuC firmware logging level. This takes effect only if GuC is to be "
+	"loaded (depends on enable_guc) (-1:disabled (default), 0-3:enabled)");
 
 i915_param_named_unsafe(guc_firmware_path, charp, 0400,
 	"GuC firmware path to use instead of the default one");
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 59a9021..d0131bc 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -34,6 +34,7 @@ 
  * DOC: GuC firmware log
  *
  * Firmware log is enabled by setting i915.guc_log_level to non-negative level.
+ * This takes effect only if GuC is to be loaded based on enable_guc.
  * Log data is printed out via reading debugfs i915_guc_log_dump. Reading from
  * i915_guc_load_status will print out firmware loading status and scratch
  * registers value.