| Message ID | 1455183658-12516-1-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Patrik Jakobsson <patrik.jakobsson@linux.intel.com> writes: > The DMC can incorrectly run off and allow DC states on it's own. We > don't know the root-cause for this yet but this patch makes it more > visible. > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> Yes, we definitely need much more state checking and hardening in this area. Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_csr.c | 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++++++++ > 3 files changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e11eef1..7e33454 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -746,6 +746,7 @@ struct intel_csr { > uint32_t mmio_count; > i915_reg_t mmioaddr[8]; > uint32_t mmiodata[8]; > + uint32_t dc_state; > }; > > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 2a7ec31..b453fcc 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -243,6 +243,8 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > I915_WRITE(dev_priv->csr.mmioaddr[i], > dev_priv->csr.mmiodata[i]); > } > + > + dev_priv->csr.dc_state = 0; > } > > static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index bbca527..e79674b 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -494,10 +494,18 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state) > val = I915_READ(DC_STATE_EN); > DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", > val & mask, state); > + > + /* Check if DMC is ignoring our DC state requests */ > + if ((val & mask) != dev_priv->csr.dc_state) > + DRM_ERROR("DC state mismatch (0x%x -> 0x%x)\n", > + dev_priv->csr.dc_state, val & mask); > + > val &= ~mask; > val |= state; > I915_WRITE(DC_STATE_EN, val); > POSTING_READ(DC_STATE_EN); > + > + dev_priv->csr.dc_state = val & mask; > } > > void bxt_enable_dc9(struct drm_i915_private *dev_priv) > -- > 2.5.0
I was going to merge here but I saw on patchwork we got some warnings so I'm not sure they are only false positives or this is exactly what this patches wants... On Thu, 2016-02-11 at 12:43 +0200, Mika Kuoppala wrote: > Patrik Jakobsson <patrik.jakobsson@linux.intel.com> writes: > > > The DMC can incorrectly run off and allow DC states on it's own. We > > don't know the root-cause for this yet but this patch makes it more > > visible. > > > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > Yes, we definitely need much more state checking and hardening > in this area. > > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_csr.c | 2 ++ > > drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++++++++ > > 3 files changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index e11eef1..7e33454 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -746,6 +746,7 @@ struct intel_csr { > > uint32_t mmio_count; > > i915_reg_t mmioaddr[8]; > > uint32_t mmiodata[8]; > > + uint32_t dc_state; > > }; > > > > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > b/drivers/gpu/drm/i915/intel_csr.c > > index 2a7ec31..b453fcc 100644 > > --- a/drivers/gpu/drm/i915/intel_csr.c > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > @@ -243,6 +243,8 @@ void intel_csr_load_program(struct > > drm_i915_private *dev_priv) > > I915_WRITE(dev_priv->csr.mmioaddr[i], > > dev_priv->csr.mmiodata[i]); > > } > > + > > + dev_priv->csr.dc_state = 0; > > } > > > > static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index bbca527..e79674b 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -494,10 +494,18 @@ static void gen9_set_dc_state(struct > > drm_i915_private *dev_priv, uint32_t state) > > val = I915_READ(DC_STATE_EN); > > DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", > > val & mask, state); > > + > > + /* Check if DMC is ignoring our DC state requests */ > > + if ((val & mask) != dev_priv->csr.dc_state) > > + DRM_ERROR("DC state mismatch (0x%x -> 0x%x)\n", > > + dev_priv->csr.dc_state, val & mask); > > + > > val &= ~mask; > > val |= state; > > I915_WRITE(DC_STATE_EN, val); > > POSTING_READ(DC_STATE_EN); > > + > > + dev_priv->csr.dc_state = val & mask; > > } > > > > void bxt_enable_dc9(struct drm_i915_private *dev_priv)
On Thu, Feb 18, 2016 at 12:16:40AM +0000, Vivi, Rodrigo wrote: > I was going to merge here but I saw on patchwork we got some warnings > so I'm not sure they are only false positives or this is exactly what > this patches wants... They are unfortunately not false positives. The question is if it's ok to clutter CI results with known errors or not? Together with Mika's and Imre's patches we shouldn't be seeing this but if we do we know we're not out of the deep water yet. Perhaps we can wait with this patch until the others are merged? -Patrik > > On Thu, 2016-02-11 at 12:43 +0200, Mika Kuoppala wrote: > > Patrik Jakobsson <patrik.jakobsson@linux.intel.com> writes: > > > > > The DMC can incorrectly run off and allow DC states on it's own. We > > > don't know the root-cause for this yet but this patch makes it more > > > visible. > > > > > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > > Yes, we definitely need much more state checking and hardening > > in this area. > > > > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > > > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_csr.c | 2 ++ > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++++++++ > > > 3 files changed, 11 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index e11eef1..7e33454 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -746,6 +746,7 @@ struct intel_csr { > > > uint32_t mmio_count; > > > i915_reg_t mmioaddr[8]; > > > uint32_t mmiodata[8]; > > > + uint32_t dc_state; > > > }; > > > > > > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > > b/drivers/gpu/drm/i915/intel_csr.c > > > index 2a7ec31..b453fcc 100644 > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > @@ -243,6 +243,8 @@ void intel_csr_load_program(struct > > > drm_i915_private *dev_priv) > > > I915_WRITE(dev_priv->csr.mmioaddr[i], > > > dev_priv->csr.mmiodata[i]); > > > } > > > + > > > + dev_priv->csr.dc_state = 0; > > > } > > > > > > static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index bbca527..e79674b 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -494,10 +494,18 @@ static void gen9_set_dc_state(struct > > > drm_i915_private *dev_priv, uint32_t state) > > > val = I915_READ(DC_STATE_EN); > > > DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", > > > val & mask, state); > > > + > > > + /* Check if DMC is ignoring our DC state requests */ > > > + if ((val & mask) != dev_priv->csr.dc_state) > > > + DRM_ERROR("DC state mismatch (0x%x -> 0x%x)\n", > > > + dev_priv->csr.dc_state, val & mask); > > > + > > > val &= ~mask; > > > val |= state; > > > I915_WRITE(DC_STATE_EN, val); > > > POSTING_READ(DC_STATE_EN); > > > + > > > + dev_priv->csr.dc_state = val & mask; > > > } > > > > > > void bxt_enable_dc9(struct drm_i915_private *dev_priv)
On Thu, Feb 18, 2016 at 11:22 AM, Patrik Jakobsson <patrik.jakobsson@linux.intel.com> wrote: > On Thu, Feb 18, 2016 at 12:16:40AM +0000, Vivi, Rodrigo wrote: >> I was going to merge here but I saw on patchwork we got some warnings >> so I'm not sure they are only false positives or this is exactly what >> this patches wants... > > They are unfortunately not false positives. The question is if it's ok > to clutter CI results with known errors or not? Together with Mika's and > Imre's patches we shouldn't be seeing this but if we do we know we're > not out of the deep water yet. Perhaps we can wait with this patch until > the others are merged? > > -Patrik I just tested Mika's tree (with all the required patches included) and I see no "DC state mismatch" errors. If we wait with this one until they are merged we wont unnecessarily clutter CI. > >> >> On Thu, 2016-02-11 at 12:43 +0200, Mika Kuoppala wrote: >> > Patrik Jakobsson <patrik.jakobsson@linux.intel.com> writes: >> > >> > > The DMC can incorrectly run off and allow DC states on it's own. We >> > > don't know the root-cause for this yet but this patch makes it more >> > > visible. >> > > >> > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> >> > >> > Yes, we definitely need much more state checking and hardening >> > in this area. >> > >> > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> >> > >> > >> > > --- >> > > drivers/gpu/drm/i915/i915_drv.h | 1 + >> > > drivers/gpu/drm/i915/intel_csr.c | 2 ++ >> > > drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++++++++ >> > > 3 files changed, 11 insertions(+) >> > > >> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> > > b/drivers/gpu/drm/i915/i915_drv.h >> > > index e11eef1..7e33454 100644 >> > > --- a/drivers/gpu/drm/i915/i915_drv.h >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > > @@ -746,6 +746,7 @@ struct intel_csr { >> > > uint32_t mmio_count; >> > > i915_reg_t mmioaddr[8]; >> > > uint32_t mmiodata[8]; >> > > + uint32_t dc_state; >> > > }; >> > > >> > > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ >> > > diff --git a/drivers/gpu/drm/i915/intel_csr.c >> > > b/drivers/gpu/drm/i915/intel_csr.c >> > > index 2a7ec31..b453fcc 100644 >> > > --- a/drivers/gpu/drm/i915/intel_csr.c >> > > +++ b/drivers/gpu/drm/i915/intel_csr.c >> > > @@ -243,6 +243,8 @@ void intel_csr_load_program(struct >> > > drm_i915_private *dev_priv) >> > > I915_WRITE(dev_priv->csr.mmioaddr[i], >> > > dev_priv->csr.mmiodata[i]); >> > > } >> > > + >> > > + dev_priv->csr.dc_state = 0; >> > > } >> > > >> > > static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, >> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c >> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c >> > > index bbca527..e79674b 100644 >> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> > > @@ -494,10 +494,18 @@ static void gen9_set_dc_state(struct >> > > drm_i915_private *dev_priv, uint32_t state) >> > > val = I915_READ(DC_STATE_EN); >> > > DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", >> > > val & mask, state); >> > > + >> > > + /* Check if DMC is ignoring our DC state requests */ >> > > + if ((val & mask) != dev_priv->csr.dc_state) >> > > + DRM_ERROR("DC state mismatch (0x%x -> 0x%x)\n", >> > > + dev_priv->csr.dc_state, val & mask); >> > > + >> > > val &= ~mask; >> > > val |= state; >> > > I915_WRITE(DC_STATE_EN, val); >> > > POSTING_READ(DC_STATE_EN); >> > > + >> > > + dev_priv->csr.dc_state = val & mask; >> > > } >> > > >> > > void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > -- > --- > Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e11eef1..7e33454 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -746,6 +746,7 @@ struct intel_csr { uint32_t mmio_count; i915_reg_t mmioaddr[8]; uint32_t mmiodata[8]; + uint32_t dc_state; }; #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 2a7ec31..b453fcc 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -243,6 +243,8 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) I915_WRITE(dev_priv->csr.mmioaddr[i], dev_priv->csr.mmiodata[i]); } + + dev_priv->csr.dc_state = 0; } static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index bbca527..e79674b 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -494,10 +494,18 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state) val = I915_READ(DC_STATE_EN); DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", val & mask, state); + + /* Check if DMC is ignoring our DC state requests */ + if ((val & mask) != dev_priv->csr.dc_state) + DRM_ERROR("DC state mismatch (0x%x -> 0x%x)\n", + dev_priv->csr.dc_state, val & mask); + val &= ~mask; val |= state; I915_WRITE(DC_STATE_EN, val); POSTING_READ(DC_STATE_EN); + + dev_priv->csr.dc_state = val & mask; } void bxt_enable_dc9(struct drm_i915_private *dev_priv)
The DMC can incorrectly run off and allow DC states on it's own. We don't know the root-cause for this yet but this patch makes it more visible. Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_csr.c | 2 ++ drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++++++++ 3 files changed, 11 insertions(+)