[Intel-gfx] [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter

[Goel, Akash]

On 7/11/2016 5:20 PM, Tvrtko Ursulin wrote:
>
> On 11/07/16 12:41, Goel, Akash wrote:
>> On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote:
>>>
>>> On 10/07/16 14:41, akash.goel at intel.com wrote:
>>>> From: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>>
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 2112e02..8a9a0cb 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> index 605c696..b211bd0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> @@ -175,11 +175,13 @@ static void set_guc_init_params(struct
>>>> drm_i915_private *dev_priv)
>>>>       params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>>>>               GUC_CTL_VCS2_ENABLED;
>>>>
>>>> -    if (i915.guc_log_level >= 0) {
>>>> -        params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>>>> +    params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>>>
>>> guc->log_flags will be zero when logging is not configured because guc
>>> is a part of dev_priv. So it looks safe - although I reckon it would be
>>> clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
>>> below?
>>
>> If logging is not enabled at (due to guc_log_level < 0), then also
>> log_flags needs to be setup & passed to GuC firmware.
>> log_flags shall not be zero even when logging is not be enabled (at boot
>> time).
>> Actually log_flags will also contain the address of the log buffer.
>
> Ah yes, I got confused by jumping between one file with your patch
> applied and one without it.
>
>>>> +
>>>> +    if (i915.guc_log_level >= 0)
>>>>           params[GUC_CTL_DEBUG] =
>>>>               i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
>>>> -    }
>>>> +    else
>>>> +        params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>>>
>>> I also wonder how come GUC_LOG_DISABLED isn't set today when
>>> i915.guc_log_level == -1, given that:
>>>
>>> #define   GUC_LOG_DISABLED             (1 << 6)
>>>
>>> Is that bit set by default somehow if i915 does not program it?
>>>
>>
>> Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1.
>> But then log buffer address will go as NULL and GUC_LOG_VALID flag will
>> go as 0, for guc_log_level = -1. So this way logging on GuC side will
>> not get enabled.
>> I hope I understood your concern correctly.
>
> Yes, this clarifies it. Although I do have one more question then - what
> happens if at boot i915.guc_log_level == -1 and then with later patches
> logging gets enabled via debugfs - who and how sets
> params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter?
>

Yes through Host2GuC action type, UK_LOG_ENABLE_LOGGING, Host will 
request GuC firmware to enable/disable logging and alter the verbosity
level.

The params[GUC_CTL_DEBUG] is just part of the firmware initialization
parameters and is not used after that.

Best regards
Akash

> Regards,
>
> Tvrtko
>
>