| Message ID | 1393540010-1582-17-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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. --Imre
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...
On Fri, 2014-02-28 at 09:55 -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? Yea, actually link (re)training for example, but for that we already now take the needed power domains (and hence adjust RPM) during modeset. > 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... I assume all these need a non-NULL modeset, so we are covered based on the above. Also as a side-note, we have to get/put the power domain on these paths not RPM, because some platforms may need some power well to be on in addition to the D0 state RPM guarantees. --Imre
2014-02-28 15:20 GMT-03:00 Imre Deak <imre.deak@intel.com>: > On Fri, 2014-02-28 at 09:55 -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? > > Yea, actually link (re)training for example, but for that we already now > take the needed power domains (and hence adjust RPM) during modeset. > >> 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... > > I assume all these need a non-NULL modeset, so we are covered based on > the above. Also as a side-note, we have to get/put the power domain on > these paths not RPM, because some platforms may need some power well to > be on in addition to the D0 state RPM guarantees. I agree with Imre. For now, while the current code just needs runtime PM, we just get runtime PM. But as soon as we add support to some power well that also needs to be enabled, then we just get the appropriate power domain instead of runtime PM (remember: on this series, whenever we get a power domain reference, we also get a runtime PM reference). > > --Imre >
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
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;