On Fri, Dec 07, 2018 at 07:13:05PM +0200, Ville Syrjälä wrote:
> 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.
> >
> > 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
> <snip>
> > > @@ -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?
>
> Just copy-pasted it so got it in both. I can drop the misc-io here if
> you prefer that.
Yes imo, it's clearer that way. With that it's
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
provided the change wouldn't be NAKed by the pending BSpec update, but I
doubt it would. Let's still wait for an update.
>
> >
> > > + 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
> > >
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
