On Thu, Dec 06, 2018 at 02:23:00PM +0200, Imre Deak wrote:
> On Wed, Dec 05, 2018 at 10:20:23PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > The DMC firmware is confused and forces on the BIOS and debug
> > power well requests for PW1 and MISC IO on some platforms. On
> > BXT I measured this to waste about 10mW in the freeze system
> > suspend state with the SoC in s0. I didn't get conclusive
> > numbers for s0ix on account of the power consumption being
> > much more noisy than in s0.
> >
> > This is pretty much undoing part of commit 42d9366d41a9
> > ("drm/i915/gen9+: Don't remove secondary power well requests")
> > where we stopped sanitizing the DMCs bogus request bits.
> >
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Thanks for the effort for clarifying this.
>
> The justification in 42d9366d41a9 for not removing the PW#1 and MISC_IO driver
> request bits is not too solid, I admit. Bspec 4230 has this to say about them
> (for SKL, but same in practice for the rest):
>
> """
> 4. Disable Power Well 1 (PG1) and Misc IO Power
>
> Clear PWR_WELL_CTL Power Well 1 Request to 0b. Do not clear Misc IO Power Request.
> Wait for 10us. Do not poll for the power well to disable. Other clients may be keeping it enabled.
> """
>
> With MISC_IO we would clearly do the opposite in this patch wrt. the spec.
Actually we don't disable MISC_IO during SKL display core uninit, so
we'd still follow the spec regarding this.
>
> With PW#1 it's unclear if the spec means only the driver's request bit or all
> of them, but "other clients may be keeping it enabled" suggests to me it means
> only the driver's request bit. OTOH removing only the driver's request bit
> alone doesn't make sense due to DMC forcing on the BIOS (and debug) request
> bits anyway.
>
> I opened a BSpec update request to clarify this, I suggest waiting for an
> update there before going ahead with this change.
>
> > ---
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 35 +++++++++++++++++++++++--
> > 1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 4350a5270423..6e349181dd1c 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -336,10 +336,17 @@ static void hsw_wait_for_power_well_disable(struct drm_i915_private *dev_priv,
> > * Bspec doesn't require waiting for PWs to get disabled, but still do
> > * this for paranoia. The known cases where a PW will be forced on:
> > * - a KVMR request on any power well via the KVMR request register
> > - * - a DMC request on PW1 and MISC_IO power wells via the BIOS and
> > - * DEBUG request registers
> > + * - a debug request on any power well via the DEBUG request register
> > * Skip the wait in case any of the request bits are set and print a
> > * diagnostic message.
> > + *
> > + * Note that DMC firmware will also force on the PW1 BIOS request
> > + * on SKL-CNL, MISC_IO BIOS request on SKL-GLK (although MISC_IO
> > + * does not even exits on BXT/GLK so the bit doesn't stick),
> > + * and the PW1/MISC_IO debug request on BXT. We simply clear
> > + * those spurious requests in hsw_power_well_disable() to make
> > + * sure they don't waste power. Starting from ICL the DMC firmware
> > + * has been fixed to only force on the PW1 driver request bit.
> > */
> > wait_for((disabled = !(I915_READ(regs->driver) &
> > HSW_PWR_WELL_CTL_STATE(pw_idx))) ||
> > @@ -347,6 +354,11 @@ static void hsw_wait_for_power_well_disable(struct drm_i915_private *dev_priv,
> > if (disabled)
> > return;
> >
> > + WARN(reqs & 3,
> > + "%s left on (bios:%d driver:%d kvmr:%d debug:%d)\n",
> > + power_well->desc->name,
> > + !!(reqs & 1), !!(reqs & 2), !!(reqs & 4), !!(reqs & 8));
> > +
> > DRM_DEBUG_KMS("%s forced on (bios:%d driver:%d kvmr:%d debug:%d)\n",
> > power_well->desc->name,
> > !!(reqs & 1), !!(reqs & 2), !!(reqs & 4), !!(reqs & 8));
> > @@ -409,6 +421,7 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
> > struct i915_power_well *power_well)
> > {
> > const struct i915_power_well_regs *regs = power_well->desc->hsw.regs;
> > + enum i915_power_well_id id = power_well->desc->id;
> > int pw_idx = power_well->desc->hsw.idx;
> > u32 val;
> >
> > @@ -417,6 +430,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
> >
> > val = I915_READ(regs->driver);
> > I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> > + /*
> > + * On SKL-CNL DMC firmware forces on the BIOS request.
> > + * This wastes a bit of power so clear it.
> > + */
> > + if (INTEL_GEN(dev_priv) < 11 &&
> > + (id == SKL_DISP_PW_1 || id == SKL_DISP_PW_MISC_IO)) {
> > + val = I915_READ(regs->bios);
> > + I915_WRITE(regs->bios, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> > + }
> > + /*
> > + * On BXT DMC firmware forces on the debug request.
> > + * This wastes a bit of power so clear it.
> > + */
> > + if (IS_BROXTON(dev_priv) &&
> > + (id == SKL_DISP_PW_1 || id == SKL_DISP_PW_MISC_IO)) {
>
> No MISC_IO on BXT, so is this just for symmetry with the above if-block?
>
> > + val = I915_READ(regs->debug);
> > + I915_WRITE(regs->debug, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> > + }
> > hsw_wait_for_power_well_disable(dev_priv, power_well);
> > }
> >
> > --
> > 2.18.1
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
