Message ID | 1452790243.27879.37.camel@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On to, 2016-01-14 at 18:50 +0200, Imre Deak wrote: > On la, 2015-12-19 at 09:58 +0000, Chris Wilson wrote: > > Once all the preparations are complete, we are ready to write the > > modesetting to the hardware. During this phase, we will be making > > lots > > of HW register access, so take a top level wakeref to prevent an > > unwarranted rpm suspend cycle mid-commit. Lower level functions > > should > > be waking the individual power wells as required. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93439 > > I would separate here two things: > > The device level power flip-flopping you mention and the fix for the > above bug. For the flip-flopping we could use what you suggest, > perhaps > by also avoiding waking up the device if nothing will change and > everything will stay disabled. > > As for the fix I would go with what Ville suggested. By ensuring we > keep an RPM reference we still allow for a display power domain > reference to come and go in the middle of the HW readout. I went > ahead > and tried the following which got rid of the problem too, if people > are > ok with it I could convert the rest of the HW readout places > accordingly and send out the patch. We can also > get pm_runtime_get_if_in_use() into use once it's added, but it's > not crucial for the fix. > Below patch looks fine. Just that I'd use s/if_enabled/if_in_use/ to match the PM API better. We're already doing too much of a good job to having many names for same thing. Regards, Joonas > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 1f9a368..907377dc 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1910,13 +1910,16 @@ bool intel_ddi_connector_get_hw_state(struct > intel_connector *intel_connector) > enum transcoder cpu_transcoder; > enum intel_display_power_domain power_domain; > uint32_t tmp; > + bool ret; > > power_domain = > intel_display_port_power_domain(intel_encoder); > - if (!intel_display_power_is_enabled(dev_priv, power_domain)) > + if (!intel_display_power_get_if_enabled(dev_priv, > power_domain)) > return false; > > - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) > - return false; > + if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) { > + ret = false; > + goto out; > + } > > if (port == PORT_A) > cpu_transcoder = TRANSCODER_EDP; > @@ -1928,23 +1931,30 @@ bool intel_ddi_connector_get_hw_state(struct > intel_connector *intel_connector) > switch (tmp & TRANS_DDI_MODE_SELECT_MASK) { > case TRANS_DDI_MODE_SELECT_HDMI: > case TRANS_DDI_MODE_SELECT_DVI: > - return (type == DRM_MODE_CONNECTOR_HDMIA); > + ret = type == DRM_MODE_CONNECTOR_HDMIA; > + goto out; > > case TRANS_DDI_MODE_SELECT_DP_SST: > - if (type == DRM_MODE_CONNECTOR_eDP) > - return true; > - return (type == DRM_MODE_CONNECTOR_DisplayPort); > + ret = type == DRM_MODE_CONNECTOR_eDP || > + type == DRM_MODE_CONNECTOR_DisplayPort; > + goto out; > case TRANS_DDI_MODE_SELECT_DP_MST: > /* if the transcoder is in MST state then > * connector isn't connected */ > - return false; > + ret = false; > + goto out; > > case TRANS_DDI_MODE_SELECT_FDI: > - return (type == DRM_MODE_CONNECTOR_VGA); > + ret = type == DRM_MODE_CONNECTOR_VGA; > + goto out; > > default: > - return false; > + ret = false; > } > +out: > + intel_display_power_put(dev_priv, power_domain); > + > + return ret; > } > > bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 059b46e..3c84159 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1456,6 +1456,8 @@ bool __intel_display_power_is_enabled(struct > drm_i915_private *dev_priv, > enum > intel_display_power_domain domain); > void intel_display_power_get(struct drm_i915_private *dev_priv, > enum intel_display_power_domain > domain); > +bool intel_display_power_get_if_enabled(struct drm_i915_private > *dev_priv, > + enum > intel_display_power_domain domain); > void intel_display_power_put(struct drm_i915_private *dev_priv, > enum intel_display_power_domain > domain); > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index bbca527..6c4f170 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1470,6 +1470,17 @@ void intel_display_power_get(struct > drm_i915_private *dev_priv, > mutex_unlock(&power_domains->lock); > } > > +bool intel_display_power_get_if_enabled(struct drm_i915_private > *dev_priv, > + enum > intel_display_power_domain domain) > +{ > + if (!intel_display_power_is_enabled(dev_priv, domain)) > + return false; > + > + intel_display_power_get(dev_priv, domain); > + > + return true; > +} > + > /** > * intel_display_power_put - release a power domain reference > * @dev_priv: i915 device instance > > --Imre > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index abd2d2944022..60451c3932db 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13470,6 +13470,13 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > drm_atomic_helper_swap_state(dev, state); > > dev_priv->wm.config = to_intel_atomic_state(state)- > > > wm_config; > > > > + /* Take a rpm wakeref for the duration of the commit. > > Lower > > level > > + * functions should be acquiring the power wells for their > > own use, > > + * we take this toplevel reference to prevent rpm suspend > > cycles > > + * mid-commit. > > + */ > > + intel_runtime_pm_get(dev_priv); > > + > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > > struct intel_crtc *intel_crtc = > > to_intel_crtc(crtc); > > > > @@ -13558,6 +13565,8 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > if (any_ms) > > intel_modeset_check_state(dev, state); > > > > + intel_runtime_pm_put(dev_priv); > > + > > drm_atomic_state_free(state); > > > > return 0;
On Thu, 2016-01-21 at 13:52 +0200, Joonas Lahtinen wrote: > On to, 2016-01-14 at 18:50 +0200, Imre Deak wrote: > [...] > Below patch looks fine. Just that I'd use s/if_enabled/if_in_use/ to > match the PM API better. We're already doing too much of a good job > to > having many names for same thing. We do have two separate states that we handle separately in both frameworks: - enabled/active: the HW is powered-on at the moment - in_use: the HW is powered-on _and_ we hold a reference So imo it makes sense to make this distinction in the function names too. --Imre > Regards, Joonas > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 1f9a368..907377dc 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1910,13 +1910,16 @@ bool > > intel_ddi_connector_get_hw_state(struct > > intel_connector *intel_connector) > > enum transcoder cpu_transcoder; > > enum intel_display_power_domain power_domain; > > uint32_t tmp; > > + bool ret; > > > > power_domain = > > intel_display_port_power_domain(intel_encoder); > > - if (!intel_display_power_is_enabled(dev_priv, > > power_domain)) > > + if (!intel_display_power_get_if_enabled(dev_priv, > > power_domain)) > > return false; > > > > - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) > > - return false; > > + if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) { > > + ret = false; > > + goto out; > > + } > > > > if (port == PORT_A) > > cpu_transcoder = TRANSCODER_EDP; > > @@ -1928,23 +1931,30 @@ bool > > intel_ddi_connector_get_hw_state(struct > > intel_connector *intel_connector) > > switch (tmp & TRANS_DDI_MODE_SELECT_MASK) { > > case TRANS_DDI_MODE_SELECT_HDMI: > > case TRANS_DDI_MODE_SELECT_DVI: > > - return (type == DRM_MODE_CONNECTOR_HDMIA); > > + ret = type == DRM_MODE_CONNECTOR_HDMIA; > > + goto out; > > > > case TRANS_DDI_MODE_SELECT_DP_SST: > > - if (type == DRM_MODE_CONNECTOR_eDP) > > - return true; > > - return (type == DRM_MODE_CONNECTOR_DisplayPort); > > + ret = type == DRM_MODE_CONNECTOR_eDP || > > + type == DRM_MODE_CONNECTOR_DisplayPort; > > + goto out; > > case TRANS_DDI_MODE_SELECT_DP_MST: > > /* if the transcoder is in MST state then > > * connector isn't connected */ > > - return false; > > + ret = false; > > + goto out; > > > > case TRANS_DDI_MODE_SELECT_FDI: > > - return (type == DRM_MODE_CONNECTOR_VGA); > > + ret = type == DRM_MODE_CONNECTOR_VGA; > > + goto out; > > > > default: > > - return false; > > + ret = false; > > } > > +out: > > + intel_display_power_put(dev_priv, power_domain); > > + > > + return ret; > > } > > > > bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 059b46e..3c84159 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1456,6 +1456,8 @@ bool __intel_display_power_is_enabled(struct > > drm_i915_private *dev_priv, > > enum > > intel_display_power_domain domain); > > void intel_display_power_get(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain > > domain); > > +bool intel_display_power_get_if_enabled(struct drm_i915_private > > *dev_priv, > > + enum > > intel_display_power_domain domain); > > void intel_display_power_put(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain > > domain); > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index bbca527..6c4f170 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -1470,6 +1470,17 @@ void intel_display_power_get(struct > > drm_i915_private *dev_priv, > > mutex_unlock(&power_domains->lock); > > } > > > > +bool intel_display_power_get_if_enabled(struct drm_i915_private > > *dev_priv, > > + enum > > intel_display_power_domain domain) > > +{ > > + if (!intel_display_power_is_enabled(dev_priv, domain)) > > + return false; > > + > > + intel_display_power_get(dev_priv, domain); > > + > > + return true; > > +} > > + > > /** > > * intel_display_power_put - release a power domain reference > > * @dev_priv: i915 device instance > > > > --Imre
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 1f9a368..907377dc 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1910,13 +1910,16 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) enum transcoder cpu_transcoder; enum intel_display_power_domain power_domain; uint32_t tmp; + bool ret; power_domain = intel_display_port_power_domain(intel_encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) - return false; + if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) { + ret = false; + goto out; + } if (port == PORT_A) cpu_transcoder = TRANSCODER_EDP; @@ -1928,23 +1931,30 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) switch (tmp & TRANS_DDI_MODE_SELECT_MASK) { case TRANS_DDI_MODE_SELECT_HDMI: case TRANS_DDI_MODE_SELECT_DVI: - return (type == DRM_MODE_CONNECTOR_HDMIA); + ret = type == DRM_MODE_CONNECTOR_HDMIA; + goto out; case TRANS_DDI_MODE_SELECT_DP_SST: - if (type == DRM_MODE_CONNECTOR_eDP) - return true; - return (type == DRM_MODE_CONNECTOR_DisplayPort); + ret = type == DRM_MODE_CONNECTOR_eDP || + type == DRM_MODE_CONNECTOR_DisplayPort; + goto out; case TRANS_DDI_MODE_SELECT_DP_MST: /* if the transcoder is in MST state then * connector isn't connected */ - return false; + ret = false; + goto out; case TRANS_DDI_MODE_SELECT_FDI: - return (type == DRM_MODE_CONNECTOR_VGA); + ret = type == DRM_MODE_CONNECTOR_VGA; + goto out; default: - return false; + ret = false; } +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } bool intel_ddi_get_hw_state(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 059b46e..3c84159 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1456,6 +1456,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv, + enum intel_display_power_domain domain); void intel_display_power_put(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index bbca527..6c4f170 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1470,6 +1470,17 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, mutex_unlock(&power_domains->lock); } +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv, + enum intel_display_power_domain domain) +{ + if (!intel_display_power_is_enabled(dev_priv, domain)) + return false; + + intel_display_power_get(dev_priv, domain); + + return true; +} + /** * intel_display_power_put - release a power domain reference * @dev_priv: i915 device instance