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

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

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
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc)
>>>       unsigned long offset;
>>>       uint32_t size, flags;
>>>
>>> -    if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN)
>>> -        return;
>>> -
>>>       if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
>>>           i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
>>>
>>> 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?

Regards,

Tvrtko


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