Re: [RFC][PATCH 12/12] drm/i915: Add encoder .sync_state() hook

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 28, 2016 at 05:50:48PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We cache a bunch of state under the encoders (eg. link freq and
> lane count for DP). We need to refresh that cached state on
> init/resume during hardware readout. Since we have already read out
> most interesting details into the pipe config, let's add a new
> optional encoder hook that can move the information from the pipe
> config into the encoder.
> 
> We can't really use the pipe config directly since this cached
> information is used during link retraining, which doesn't have
> access to a pipe config (mainly due to MST). Redoing MST in a way where
> the actual link would be part of the modeset with its own pipe config
> could be an alternative solution, but that would be a much more massive
> undertaking. For now, let's at least try to keep the two pieces of
> state in sync.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I really don't like that we have state stored in non-state structure
places. Especially with upfront link training at hpd time it probably
makes sense to figure this all out at compute_config time, and then store
it in the crtc state. Ok probably it needs to be somewhere else, not
attached to crtc, so maybe an array of port states. And cross-checking
would be real great for this too ...
-Daniel


> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++++
>  drivers/gpu/drm/i915/intel_display.c |  4 ++++
>  drivers/gpu/drm/i915/intel_dp.c      | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  7 +++++++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index bc188ee6e37f..2b679f72eba4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2314,6 +2314,9 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
>  	return connector;
>  }
>  
> +void intel_dp_sync_state(struct intel_encoder *encoder,
> +			 const struct intel_crtc_state *pipe_config);
> +
>  void intel_ddi_init(struct drm_device *dev, enum port port)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2377,6 +2380,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	intel_encoder->post_disable = intel_ddi_post_disable;
>  	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
>  	intel_encoder->get_config = intel_ddi_get_config;
> +	intel_encoder->sync_state = intel_dp_sync_state;
>  	intel_encoder->suspend = intel_dp_encoder_suspend;
>  
>  	intel_dig_port->port = port;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a6699c76bef3..dffcc814c178 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16031,8 +16031,12 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			encoder->base.crtc = &crtc->base;
>  			crtc->config->output_types |= 1 << encoder->type;
>  			encoder->get_config(encoder, crtc->config);
> +			if (encoder->sync_state)
> +				encoder->sync_state(encoder, crtc->config);
>  		} else {
>  			encoder->base.crtc = NULL;
> +			if (encoder->sync_state)
> +				encoder->sync_state(encoder, NULL);
>  		}
>  
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7551d92a3cfb..44567dd68257 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2482,6 +2482,23 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>  	}
>  }
>  
> +void intel_dp_sync_state(struct intel_encoder *encoder,
> +			 const struct intel_crtc_state *pipe_config)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +	if (pipe_config) {
> +		/* FIXME MST */
> +		intel_dp->active_streams = 1;
> +		intel_dp->link_rate = pipe_config->port_clock;
> +		intel_dp->lane_count = pipe_config->lane_count;
> +	} else {
> +		intel_dp->active_streams = 0;
> +		intel_dp->link_rate = 0;
> +		intel_dp->lane_count = 0;
> +	}
> +}
> +
>  static void intel_disable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -5675,6 +5692,7 @@ bool intel_dp_init(struct drm_device *dev,
>  	intel_encoder->disable = intel_disable_dp;
>  	intel_encoder->get_hw_state = intel_dp_get_hw_state;
>  	intel_encoder->get_config = intel_dp_get_config;
> +	intel_encoder->sync_state = intel_dp_sync_state;
>  	intel_encoder->suspend = intel_dp_encoder_suspend;
>  	if (IS_CHERRYVIEW(dev)) {
>  		intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 20cf7ad26357..49c75caa4150 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -213,6 +213,13 @@ struct intel_encoder {
>  	void (*get_config)(struct intel_encoder *,
>  			   struct intel_crtc_state *pipe_config);
>  	/*
> +	 * Used during init/resume to sync any cached state
> +	 * stored outside the pipe config (eg. active DP link
> +	 * parameters).
> +	 */
> +	void (*sync_state)(struct intel_encoder *,
> +			   const struct intel_crtc_state *);
> +	/*
>  	 * Called during system suspend after all pending requests for the
>  	 * encoder are flushed (for example for DP AUX transactions) and
>  	 * device interrupts are disabled.
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx