Re: [PATCH] firmware/guc: Remove USES_GUC_SUBMISSION for suspend/resume

[John Spotswood <john.a.spotswood@intel.com>]

On Fri, 2018-06-22 at 10:25 -0700, Daniele Ceraolo Spurio wrote:
> 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@intel.com
> > >
> > Cc: John Spotswood <john.a.spotswood@intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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@intel.com>
> 

With Daniele's recommended changes:

Reviewed-by:  John Spotswood <john.a.spotswood@intel.com>

> > 
> > +			PAGE_SHIFT;
> >   
> > -		flags |= ads << GUC_ADS_ADDR_SHIFT |
> > GUC_ADS_ENABLED;
> > -	}
> > +	flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED;
> >   
> >   	return flags;
> >   }
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx