Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 28 Feb 2014 19:38:17 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Fri, 2014-02-28 at 09:13 -0800, Jesse Barnes wrote:
> > On Thu, 27 Feb 2014 19:26:43 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> > 
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > We had these intel_aux_display_runtime_{get,put} abstractions that
> > > would just get/put PC8 references, but now that runtime PM and PC8
> > > are the same stuff, we just need the runtime PM references, so just
> > > get/put runtime PM directly, because that's what the rest of our code
> > > does.
> > > 
> > > Another way to solve this problem would be to add DP_AUX and GMBUS
> > > power domains, and then use intel_display_power_{get,put}, but every
> > > power domain already gets/puts runtime PM references, so this would
> > > just make things more complex.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 --
> > >  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
> > >  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
> > >  4 files changed, 4 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index c512d78..79d4844 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > -	intel_aux_display_runtime_get(dev_priv);
> > > +	intel_runtime_pm_get(dev_priv);
> > >  
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > > @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  	ret = recv_bytes;
> > >  out:
> > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > -	intel_aux_display_runtime_put(dev_priv);
> > > +	intel_runtime_pm_put(dev_priv);
> > >  
> > >  	return ret;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index ea24eae..a2e0cd7 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
> > >  void gen6_update_ring_freq(struct drm_device *dev);
> > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> > > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> > >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> > >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > index d33b61d..3d403ce 100644
> > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
> > >  	int i, reg_offset;
> > >  	int ret = 0;
> > >  
> > > -	intel_aux_display_runtime_get(dev_priv);
> > > +	intel_runtime_pm_get(dev_priv);
> > >  	mutex_lock(&dev_priv->gmbus_mutex);
> > >  
> > >  	if (bus->force_bit) {
> > > @@ -546,7 +546,7 @@ timeout:
> > >  
> > >  out:
> > >  	mutex_unlock(&dev_priv->gmbus_mutex);
> > > -	intel_aux_display_runtime_put(dev_priv);
> > > +	intel_runtime_pm_put(dev_priv);
> > >  	return ret;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 86e6835..1e3580f 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
> > >  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > >  }
> > >  
> > > -/* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
> > > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
> > > -{
> > > -	hsw_disable_package_c8(dev_priv);
> > > -}
> > > -
> > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> > > -{
> > > -	hsw_enable_package_c8(dev_priv);
> > > -}
> > > -
> > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
> > >  {
> > >  	struct drm_device *dev = dev_priv->dev;
> > 
> > But OTOH, in cases where we have a separate, explicit power well for
> > display, doesn't the aux_display_runtime_get|put make sense?  We don't
> > want just global runtime get/put everywhere since we can be finer
> > grained in may cases, right?
> 
> I think here we should just depend on connector->detect and ->get_modes
> getting the needed power domains, which will also adjust the RPM
> refcount accordingly.

That should work too I think, do we have any paths outside those where
we would do aux poking?  Maybe audio or DDC brightness controls in the
future?  I think audio is ok today, and we can worry about new stuff as
it comes along...

-- 
Jesse Barnes, Intel Open Source Technology Center