[Intel-gfx] [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Sagar Arun Kamble]

On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> Parameter guc_log_level needs to be sanitized based on GuC support and
>> enable_guc_loading parameter since it depends on them like
>> enable_guc_submission. This will make GuC logging paths independent of
>> enable_guc_submission parameter in further patches.
>>
>> v2: Added documentation to intel_guc_log.c and param description about
>> GuC loading dependency. (Michal Wajdeczko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_params.c   |  4 +++-
>>   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
>>   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
>>   3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index b4faeb6..774c56e 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
>>       "(-1=auto, 0=never [default], 1=if available, 2=required)");
>>     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_loading) (-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 f53c663..200f0a1 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_loading.
>>    * 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.
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 62738ad..8feefcd 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>>   {
>>       if (!HAS_GUC(dev_priv)) {
>>           if (i915_modparams.enable_guc_loading > 0 ||
>> -            i915_modparams.enable_guc_submission > 0)
>> +            i915_modparams.enable_guc_submission > 0 ||
>> +            i915_modparams.guc_log_level > 0)
>>               DRM_INFO("Ignoring GuC options, no hardware\n");
>
> Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
> default to one? Or we can worry about that when we get there...
This will fire for <=gen8 for +ve values which is expected.
>
>> i915_modparams.enable_guc_loading = 0;
>>           i915_modparams.enable_guc_submission = 0;
>> +        i915_modparams.guc_log_level = -1;
>>           return;
>>       }
>>   @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>>               i915_modparams.enable_guc_loading = 0;
>>       }
>>   -    /* Can't enable guc submission without guc loaded */
>> -    if (!i915_modparams.enable_guc_loading)
>> +    /* Can't enable guc submission and logging without guc loaded */
>> +    if (!i915_modparams.enable_guc_loading) {
>>           i915_modparams.enable_guc_submission = 0;
>> +        i915_modparams.guc_log_level = -1;
>
> Hm2, how does this interact with the changes which are removing 
> enable_guc_loading? Or it is a race?
It interacts. Will race. I think I should pause this series till the 
other one gets merged.
>
> I was just thinking that we should probably let the user know if they 
> asked for logging, but it cannot happen due guc loading turned off, to 
> have analogy with the same when GuC is not available in the first place.
will change with enable_guc_loading change.
>
> Does that make sense?
Yes :)
>
>> +    }
>>         /* A negative value means "use platform default" */
>>       if (i915_modparams.enable_guc_submission < 0)
>>
>
> But since it looks like a cleanup even without the above:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> Regards,
>
> Tvrtko