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

Jesse Barnes
2014-02-20T19:36:09Z

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