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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 18, 2014 at 12:02:10AM +0200, Imre Deak 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>
> +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);
> +}

Nope, these kinds of checks should be pushed down into the
encoder/platform specific callbacks imo. Like I've said in my previous
reply I'm not a fan of the port_power_domain function, knowledge about
this should be pushed out from generic code into encoder callbacks. hw
engineers are create enough imo that this will hurt us in the end if we
don't have full flexibility in the encoder specific code.

Same goes with any other relevant power well checks, imo they should be as
close to the actual hw readout code as possible (e.g. pfit, pll, ...). For
example see the platform specific power well check in
haswell_get_pipe_config.

So please replaced this patch with one which sprinkles the relevant checks
all over the place.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch