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

[Daniele Ceraolo Spurio]

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

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.

> 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:

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

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