Re: [PATCH 1/3] drm/i915: get basic encoder state before reading CRTC state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 17, 2014 at 01:16:56PM -0800, Jesse Barnes wrote:
> In DDI configs, we need to get the encoder to CRTC mapping early on so
> we can read out and calculate the clock state correctly, as it depends
> on the port.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 59 +++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74137d5..92f46ad 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9476,6 +9476,22 @@ check_crtc_state(struct drm_device *dev)
>  		     "crtc's computed enabled state doesn't match tracked enabled state "
>  		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
>  
> +
> +		list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +				    base.head) {
> +			/* Get encoder->crtc mapping */
> +			struct intel_crtc *tmp_crtc;
> +			enum pipe pipe;
> +			if (encoder->get_hw_state(encoder, &pipe)) {
> +				tmp_crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +				if (tmp_crtc == crtc) {
> +					encoder->base.crtc = &tmp_crtc->base;
> +					encoder->get_config(encoder,
> +							    &pipe_config);
> +				}
> +			}
> +		}
> +
>  		active = dev_priv->display.get_pipe_config(crtc,
>  							   &pipe_config);
>  

NAK. This will break clock readout for every other platform. For most 
things we read out the .port_clock from the DPLL in .get_pipe_config()
(eDP port A being the excption where intel_dp_get_config() also fills
out .port_clock), and then the encoder .get_config() massages .port_clock
appropriately to figure out what .crtc_clock was supposed to be.

Maybe I'm missing something, but can't you just follow the eDP port A
route and simply stick your intel_ddi_clock_get() into
intel_ddi_get_config(), so that intel_ddi_get_config() is always
responsible for filling out both .port_clock and .crtc_clock?

> @@ -11109,9 +11125,26 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	int i;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> -			    base.head) {
> +			    base.head)
>  		memset(&crtc->config, 0, sizeof(crtc->config));
>  
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +			    base.head) {
> +		pipe = 0;
> +
> +		if (encoder->get_hw_state(encoder, &pipe)) {
> +			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +			encoder->base.crtc = &crtc->base;
> +			drm_mode_debug_printmodeline(&crtc->config.adjusted_mode);
> +		} else {
> +			encoder->base.crtc = NULL;
> +		}
> +
> +		encoder->connectors_active = false;
> +	}
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> +			    base.head) {
>  		crtc->active = dev_priv->display.get_pipe_config(crtc,
>  								 &crtc->config);
>  
> @@ -11145,22 +11178,18 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
>  			    base.head) {
> -		pipe = 0;
> -
> -		if (encoder->get_hw_state(encoder, &pipe)) {
> -			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -			encoder->base.crtc = &crtc->base;
> +		if (encoder->base.crtc) {
> +			crtc = to_intel_crtc(encoder->base.crtc);
>  			encoder->get_config(encoder, &crtc->config);
> -		} else {
> -			encoder->base.crtc = NULL;
> -		}
>  
> -		encoder->connectors_active = false;
> -		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
> -			      encoder->base.base.id,
> -			      drm_get_encoder_name(&encoder->base),
> -			      encoder->base.crtc ? "enabled" : "disabled",
> -			      pipe_name(pipe));
> +			DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c, dp: %s\n",
> +				      encoder->base.base.id,
> +				      drm_get_encoder_name(&encoder->base),
> +				      encoder->base.crtc ? "enabled" : "disabled",
> +				      pipe_name(crtc->pipe),
> +				      crtc->config.has_dp_encoder ? "yes" : "no");
> +			drm_mode_debug_printmodeline(&crtc->config.adjusted_mode);
> +		}
>  	}
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list,
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC