On Thu, Feb 16, 2017 at 02:18:53PM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> > Atm, power wells that BIOS has enabled, but which we don't explicitly
> > enable during power domain initialization would get disabled as we clear
> > the BIOS request bit in the given power well sync_hw hook. To prevent
> > this copy over any set request bits in the BIOS request register to the
> > driver request register and clear the BIOS request bit only afterwards.
> >
> > This doesn't make a difference now, since we enable all power wells
> > during power domain initialization. A follow-up patchset will add power
> > wells for which this isn't true, so fix up the inconsistency.
> >
> > Cc: Ander Conselvan de Oliveira <conselvan2@xxxxxxxxx>
> > Cc: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b7b0e0f..e747215 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -855,12 +855,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
> > struct i915_power_well *power_well)
> > {
> > - /*
> > - * We're taking over the BIOS, so clear any requests made by it since
> > - * the driver is in charge now.
> > - */
> > - if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
> > + /* Take over the request bit if set by BIOS. */
> > + if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST) {
> > + if (!(I915_READ(HSW_PWR_WELL_DRIVER) &
> > + HSW_PWR_WELL_ENABLE_REQUEST))
> > + I915_WRITE(HSW_PWR_WELL_DRIVER,
> > + HSW_PWR_WELL_ENABLE_REQUEST);
> > I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > + }
> > }
> >
> > static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> > @@ -890,8 +892,12 @@ static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
> > uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
> > uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
> >
> > - /* Clear any request made by BIOS as driver is taking over */
> > + /* Take over the request bit if set by BIOS. */
> > if (bios_req & mask) {
> > + uint32_t drv_req = I915_READ(HSW_PWR_WELL_DRIVER);
> > +
> > + if (!(drv_req & mask))
> > + I915_WRITE(HSW_PWR_WELL_DRIVER, drv_req | mask);
> > I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
> > }
> > }
>
> With this change it will be possible for the hardware state readout code to
> inherit the state of the DDI IO power wells in Geminilake, which is what I
> wanted. I'm just thinking if there is a risk for inconsistency if the hardware
> state readout doesn't fix the reference count to a domain that is left enabled.
> Would it make sense to add a WARN or at least a debug message somewhere if this
> happens, so it doesn't go unnoticed?
Yes, good idea. This requires then that power_well->count == power_well->hw_enabled
for all power wells at the end of modeset readout. I'll add that.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
