| Message ID | 1529687156-27632-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > + PAGE_SHIFT; > > - flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED; > - } > + flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED; > > return flags; > } >
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; > > } > >
>-----Original Message----- >From: Ceraolo Spurio, Daniele >Sent: Friday, June 22, 2018 10:26 AM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- >gfx@lists.freedesktop.org >Cc: Spotswood, John A <john.a.spotswood@intel.com>; Mateo Lozano, Oscar ><oscar.mateo@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@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: Oops...thanks for pointing it out. >Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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; >> } >>
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@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Spotswood, John A <john.a.spotswood@intel.com>; Mateo Lozano, Oscar >> <oscar.mateo@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@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: > Oops...thanks for pointing it out. > >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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; >>> } >>>
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) << + PAGE_SHIFT; - flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED; - } + flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED; return flags; }
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) 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(-)