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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/07/16 13:11, Goel, Akash wrote:
>
>
> 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@intel.com wrote:
>>>>> From: Sagar Arun Kamble <sagar.a.kamble@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.

Got it now, in that case:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


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