[Intel-gfx] [PATCH] firmware/guc: Remove USES_GUC_SUBMISSION for suspend/resume

[Srivatsa, Anusha]

>-----Original Message-----
>From: Ceraolo Spurio, Daniele
>Sent: Friday, June 22, 2018 10:26 AM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
>gfx at lists.freedesktop.org
>Cc: Spotswood, John A <john.a.spotswood at intel.com>; Mateo Lozano, Oscar
><oscar.mateo at intel.com>
>Subject: Re: [PATCH] firmware/guc: Remove USES_GUC_SUBMISSION for
>suspend/resume
>
>Commit title is slightly misleading, as the USES_GUC_SUBMISSION is not removed
>from a suspend/resume path. the firmware tag is also confusing since this fixes
>an i915 bug. Maybe something like "drm/i915/guc: Remove
>USES_GUC_SUBMISSION for ads programming" would be clearer
Sure.
Makes sense.

>On 22/06/18 10:05, Anusha Srivatsa wrote:
>> In the guc_ctl_debug_flags, the ads struct is programmed only when
>> USES_GUC_SUBMISSION is satisfied. But, this has to be programmed for
>> all suspend/resume cases.
>> Remove the condition and program the ads struct for both huc loading
>> and guc submission.
>>
>> This issue was noticed when CI threw errors for enable_guc=2 (load
>> huc; disable submission)
>>
>
>Do we need a fixes: tag? Not sure we want this backported since GuC is off by
>default.

Hmm...so the patch - Load Guc,HuC on GLK is still not merged. 
Maybe skip the fixes tag? 


>> Credits to: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: John Spotswood <john.a.spotswood at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 1aff30b..b1d1a10 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -207,6 +207,7 @@ static u32 guc_ctl_debug_flags(struct intel_guc *guc)
>>   {
>>   	u32 level = intel_guc_log_get_level(&guc->log);
>>   	u32 flags = 0;
>> +	u32 ads = 0;
>>
>>   	if (!GUC_LOG_LEVEL_IS_ENABLED(level))
>>   		flags |= GUC_LOG_DEFAULT_DISABLED; @@ -217,12 +218,10
>@@ static
>> u32 guc_ctl_debug_flags(struct intel_guc *guc)
>>   		flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) <<
>>   			 GUC_LOG_VERBOSITY_SHIFT;
>>
>> -	if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
>> -		u32 ads = intel_guc_ggtt_offset(guc, guc->ads_vma)
>> -			>> PAGE_SHIFT;
>> +	ads = intel_guc_ggtt_offset(guc, guc->ads_vma) <<
>
>You've flipped the shift here. With that fixed:
Oops...thanks for pointing it out.

>Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>

And thanks for the review. Will re-spin the patch asap.

Anusha 
>> +			PAGE_SHIFT;
>>
>> -		flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED;
>> -	}
>> +	flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED;
>>
>>   	return flags;
>>   }
>>