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

[Goel, Akash]

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
>> +++ 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.

>
>> +
>> +    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.

Best regards
Akash

>>
>>       if (guc->ads_obj) {
>>           u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
>>
>
> Regards,
>
> Tvrtko
>