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ä <[email protected]> > > > > 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 <[email protected]> > > Cc: Rodrigo Vivi <[email protected]> > > Signed-off-by: Ville Syrjälä <[email protected]> > > 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. > > > + 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
