Re: [PATCH 09/19] drm/i915: check port power domain when reading the encoder hw state

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 18 Feb 2014 00:02:10 +0200
Imre Deak <imre.deak@intel.com> wrote:

> Since the encoder is tied to its port, we need to make sure the power
> domain for that port is on before reading out the encoder HW state.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2643d3b..f95bc3a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1110,7 +1110,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
>  	enum transcoder cpu_transcoder;
>  	uint32_t tmp;
>  
> -	if (!intel_encoder->get_hw_state(intel_encoder, &pipe))
> +	if (!intel_encoder_get_hw_state(intel_encoder, &pipe))
>  		return false;
>  
>  	if (port == PORT_A)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7ef06fa..ce1c00a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4519,7 +4519,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  		WARN(!encoder->connectors_active,
>  		     "encoder->connectors_active not set\n");
>  
> -		encoder_enabled = encoder->get_hw_state(encoder, &pipe);
> +		encoder_enabled = intel_encoder_get_hw_state(encoder, &pipe);
>  		WARN(!encoder_enabled, "encoder not enabled\n");
>  		if (WARN_ON(!encoder->base.crtc))
>  			return;
> @@ -4561,7 +4561,7 @@ bool intel_connector_get_hw_state(struct intel_connector *connector)
>  	enum pipe pipe = 0;
>  	struct intel_encoder *encoder = connector->encoder;
>  
> -	return encoder->get_hw_state(encoder, &pipe);
> +	return intel_encoder_get_hw_state(encoder, &pipe);
>  }
>  
>  static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
> @@ -9464,6 +9464,19 @@ check_connector_state(struct drm_device *dev)
>  	}
>  }
>  
> +bool intel_encoder_get_hw_state(struct intel_encoder *intel_encoder,
> +				enum pipe *pipe)
> +{
> +	enum intel_display_power_domain power_domain;
> +	struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private;
> +
> +	power_domain = intel_display_port_power_domain(intel_encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
> +	return intel_encoder->get_hw_state(intel_encoder, pipe);
> +}
> +
>  static void
>  check_encoder_state(struct drm_device *dev)
>  {
> @@ -9504,7 +9517,7 @@ check_encoder_state(struct drm_device *dev)
>  		     "encoder's computed active state doesn't match tracked active state "
>  		     "(expected %i, found %i)\n", active, encoder->connectors_active);
>  
> -		active = encoder->get_hw_state(encoder, &pipe);
> +		active = intel_encoder_get_hw_state(encoder, &pipe);
>  		WARN(active != encoder->connectors_active,
>  		     "encoder's hw state doesn't match sw tracking "
>  		     "(expected %i, found %i)\n",
> @@ -9571,7 +9584,7 @@ check_crtc_state(struct drm_device *dev)
>  			enum pipe pipe;
>  			if (encoder->base.crtc != &crtc->base)
>  				continue;
> -			if (encoder->get_hw_state(encoder, &pipe))
> +			if (intel_encoder_get_hw_state(encoder, &pipe))
>  				encoder->get_config(encoder, &pipe_config);
>  		}
>  
> @@ -11350,7 +11363,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			    base.head) {
>  		pipe = 0;
>  
> -		if (encoder->get_hw_state(encoder, &pipe)) {
> +		if (intel_encoder_get_hw_state(encoder, &pipe)) {
>  			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  			encoder->base.crtc = &crtc->base;
>  			encoder->get_config(encoder, &crtc->config);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e31eb1e..afc01a4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -738,6 +738,7 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_config *pipe_config);
> +bool intel_encoder_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);

I think there are a few more places to switch over, e.g.
connector_check_state and connector_get_hw_state?  Maybe we should
rename the fn pointer field to include a __ in the front so the
compiler will catch them all and we'll know they're not to be called
directly.

-- 
Jesse Barnes, Intel Open Source Technology Center