Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Imre Deak <imre.deak@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx