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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 28, 2014 at 09:55:12AM -0800, Jesse Barnes wrote:
> 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...

Yes, userspace can directly do i2c transactions on gmbus and through i2c
over dp aux also there. And I guess eventually our dp aux helpers in drm
core will grow their own native userspace interface.

I'm not on top of all the changes in our future platforms, iirc there's
been some more fine-grained changes just for the dp aux/gmbus stuff.
Damien/Ville/Rodrigo?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch