| Message ID | 1441234332-7446-1-git-send-email-yu.dai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/09/15 23:52, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > Bit 16 of GuC status indicates resuming from RC6. The LAPIC_DONE > status is a reliable readiness flag only when resuming from RC6. > This fix a racing issue that allocation of doorbell fails whilst > GuC init is not finished. > > Signed-off-by: Alex Dai <yu.dai@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_reg.h | 1 + > drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h > index 8c8e574..dd0e1e8 100644 > --- a/drivers/gpu/drm/i915/i915_guc_reg.h > +++ b/drivers/gpu/drm/i915/i915_guc_reg.h > @@ -37,6 +37,7 @@ > #define GS_UKERNEL_READY (0xF0 << GS_UKERNEL_SHIFT) > #define GS_MIA_SHIFT 16 > #define GS_MIA_MASK (0x07 << GS_MIA_SHIFT) > +#define GS_MIA_CORE_STATE (1 << GS_MIA_SHIFT) > > #define SOFT_SCRATCH(n) (0xc180 + ((n) * 4)) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 5eafd31..5823615 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -209,9 +209,10 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > u32 *status) > { > u32 val = I915_READ(GUC_STATUS); > + u32 uk_val = val & GS_UKERNEL_MASK; > *status = val; > - return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || > - (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); > + return (uk_val == GS_UKERNEL_READY || > + ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); > } > > /* This does indeed fix the problem of not being able to *re*initialise the GuC correctly after a hang/reset/reload cycle :) However I think the last line looks quite confusing, especially because of the mix of bitwise and logical operators (we have &, &&, and || all in one expression). So I think it might be clearer if spelled out in two stages: { u32 val = I915_READ(GUC_STATUS); + u32 uk_val = val & GS_UKERNEL_MASK; *status = val; - return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || - (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); + + /* Iff resuming from RC6, look for LAPIC_DONE */ + if (val & GS_MIA_CORE_STATE) + if (uk_val == GS_UKERNEL_LAPIC_DONE) + return true; + + /* Otherwise just check whether the uKernel is READY */ + return uk_val == GS_UKERNEL_READY; } Other than that I'm quite happy with it, so either version gets a Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h index 8c8e574..dd0e1e8 100644 --- a/drivers/gpu/drm/i915/i915_guc_reg.h +++ b/drivers/gpu/drm/i915/i915_guc_reg.h @@ -37,6 +37,7 @@ #define GS_UKERNEL_READY (0xF0 << GS_UKERNEL_SHIFT) #define GS_MIA_SHIFT 16 #define GS_MIA_MASK (0x07 << GS_MIA_SHIFT) +#define GS_MIA_CORE_STATE (1 << GS_MIA_SHIFT) #define SOFT_SCRATCH(n) (0xc180 + ((n) * 4)) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 5eafd31..5823615 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -209,9 +209,10 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, u32 *status) { u32 val = I915_READ(GUC_STATUS); + u32 uk_val = val & GS_UKERNEL_MASK; *status = val; - return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || - (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); + return (uk_val == GS_UKERNEL_READY || + ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); } /*