On Mon, Sep 16, 2013 at 02:33:06PM -0300, Paulo Zanoni wrote:
> 2013/9/16 <ville.syrjala@xxxxxxxxxxxxxxx>:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Reorganize the internal i915_request power well handling to use the
> > reference count just like everyone else. This way all we need to do is
> > check the reference count and we know whether the power well needs to be
> > enabled of disabled.
> >
> > v2: Split he intel_display_power_{get,put} change to another patch.
> > Add intel_resume_power_well() to make sure we enable the power
> > well on resume
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_pm.c | 43 +++++++++++++++++++++++++++++++---------
> > 2 files changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 774ebb6..8853f53 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -765,6 +765,7 @@ extern bool intel_display_power_enabled(struct drm_device *dev,
> > enum intel_display_power_domain domain);
> > extern void intel_init_power_well(struct drm_device *dev);
> > extern void intel_set_power_well(struct drm_device *dev, bool enable);
> > +extern void intel_resume_power_well(struct drm_device *dev);
> > extern void intel_enable_gt_powersave(struct drm_device *dev);
> > extern void intel_disable_gt_powersave(struct drm_device *dev);
> > extern void ironlake_teardown_rc6(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8cffef4..310d2ed 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5342,8 +5342,7 @@ void i915_request_power_well(void)
> > return;
> >
> > spin_lock_irq(&hsw_pwr->lock);
> > - if (!hsw_pwr->count++ &&
> > - !hsw_pwr->i915_request)
> > + if (!hsw_pwr->count++)
> > __intel_set_power_well(hsw_pwr->device, true);
> > spin_unlock_irq(&hsw_pwr->lock);
> > }
> > @@ -5357,8 +5356,7 @@ void i915_release_power_well(void)
> >
> > spin_lock_irq(&hsw_pwr->lock);
> > WARN_ON(!hsw_pwr->count);
> > - if (!--hsw_pwr->count &&
> > - !hsw_pwr->i915_request)
> > + if (!--hsw_pwr->count)
> > __intel_set_power_well(hsw_pwr->device, false);
> > spin_unlock_irq(&hsw_pwr->lock);
> > }
> > @@ -5394,15 +5392,41 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> > return;
> >
> > spin_lock_irq(&power_well->lock);
> > +
> > + /*
> > + * This function will only ever contribute one
> > + * to the power well reference count. i915_request
> > + * is what tracks whether we have or have not
> > + * added the one to the reference count.
> > + */
> > + if (power_well->i915_request == enable)
> > + goto out;
> > +
> > power_well->i915_request = enable;
> >
> > - /* only reject "disable" power well request */
> > - if (power_well->count && !enable) {
> > - spin_unlock_irq(&power_well->lock);
> > - return;
> > + if (enable) {
> > + if (!power_well->count++)
> > + __intel_set_power_well(dev, true);
> > + } else {
> > + WARN_ON(!power_well->count);
> > + if (!--power_well->count)
> > + __intel_set_power_well(dev, false);
> > }
> >
> > - __intel_set_power_well(dev, enable);
> > + out:
> > + spin_unlock_irq(&power_well->lock);
> > +}
> > +
> > +void intel_resume_power_well(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > + if (!HAS_POWER_WELL(dev))
> > + return;
> > +
> > + spin_lock_irq(&power_well->lock);
> > + __intel_set_power_well(dev, power_well->count > 0);
> > spin_unlock_irq(&power_well->lock);
> > }
> >
> > @@ -5421,6 +5445,7 @@ void intel_init_power_well(struct drm_device *dev)
> >
> > /* For now, we need the power well to be always enabled. */
> > intel_set_power_well(dev, true);
> > + intel_resume_power_well(dev);
>
> I find this a little bit confusing because we basically have 2
> functions that maybe call __intel_set_power_well, and in the init code
> we end calling it twice. It would be nicer if we had only 1 codepath
> leading to __intel_set_power_well.
I think the split is now very clear. intel_resume_power_well() makes
sure the hardware state matches the software state, and
intel_set_power_well() requests the power well to be activated just
like before.
>
>
> >
> > /* We're taking over the BIOS, so clear any requests made by it since
> > * the driver is in charge now. */
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
