diff mbox

drm/i915: Acquire RPM wakeref for KMS atomic commit

Message ID 1452790243.27879.37.camel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Imre Deak Jan. 14, 2016, 4:50 p.m. UTC 
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.


--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;

Comments

Joonas Lahtinen Jan. 21, 2016, 11:52 a.m. UTC | #1 
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;
Imre Deak Jan. 22, 2016, 9:21 a.m. UTC | #2 
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 mbox

Patch

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