Re: [PATCH v3 10/21] drm/i915: check port power domain when reading the encoder hw state

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed,  5 Mar 2014 16:20:54 +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.
> 
> Note that this also covers also all connector get_hw_state handlers,
> since all those just call the corresponding encoder get_hw_state
> handler, which checks - after this change - for all power domains
> the connector needs.
> 
> v2:
> - no change
> v3:
> - push down the power domain checks into the specific encoder
>   get_hw_state handlers (Daniel)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c  | 5 +++++
>  drivers/gpu/drm/i915/intel_ddi.c  | 5 +++++
>  drivers/gpu/drm/i915/intel_dp.c   | 9 ++++++++-
>  drivers/gpu/drm/i915/intel_dsi.c  | 5 +++++
>  drivers/gpu/drm/i915/intel_hdmi.c | 5 +++++
>  5 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 4a9d097..6ecea69 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -68,8 +68,13 @@ static bool intel_crt_get_hw_state(struct intel_encoder *encoder,
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crt *crt = intel_encoder_to_crt(encoder);
> +	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  
> +	power_domain = intel_display_port_power_domain(encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
>  	tmp = I915_READ(crt->adpa_reg);
>  
>  	if (!(tmp & ADPA_DAC_ENABLE))
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2643d3b..e2665e0 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1145,9 +1145,14 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  	int i;
>  
> +	power_domain = intel_display_port_power_domain(encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
>  	tmp = I915_READ(DDI_BUF_CTL(port));
>  
>  	if (!(tmp & DDI_BUF_CTL_ENABLE))
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3bc2fbc..2c5fae4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1485,7 +1485,14 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 tmp = I915_READ(intel_dp->output_reg);
> +	enum intel_display_power_domain power_domain;
> +	u32 tmp;
> +
> +	power_domain = intel_display_port_power_domain(encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
> +	tmp = I915_READ(intel_dp->output_reg);
>  
>  	if (!(tmp & DP_PORT_EN))
>  		return false;
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 63b95bbd..cf7322e 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -243,11 +243,16 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
>  				   enum pipe *pipe)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	enum intel_display_power_domain power_domain;
>  	u32 port, func;
>  	enum pipe p;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	power_domain = intel_display_port_power_domain(encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
>  	/* XXX: this only works for one DSI output */
>  	for (p = PIPE_A; p <= PIPE_B; p++) {
>  		port = I915_READ(MIPI_PORT_CTRL(p));
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index d0e2026..ceb4797 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -667,8 +667,13 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  
> +	power_domain = intel_display_port_power_domain(encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
>  	tmp = I915_READ(intel_hdmi->hdmi_reg);
>  
>  	if (!(tmp & SDVO_ENABLE))

Given the way the functions work, returning the power domain required
and making things fairly opaque, I'm not sure Daniel's suggestion buys
us anything.  So to me the wrapper seemed nicer... but either way works
I guess.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center