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

[Daniele Ceraolo Spurio]

On 22/06/18 10:38, Srivatsa, Anusha wrote:
> 
> 
>> -----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?
> 

This fails on SKL as well and we had FW there for a while. I still think 
we can skip the tag because guc is off by default, but don't take my 
word as assurance :)

Daniele

> 
>>> 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;
>>>    }
>>>