Re: [PATCH v2 09/21] drm/i915/dp: Pass atomic state to link training function

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

On Mon, May 20, 2024 at 09:58:07PM +0300, Imre Deak wrote:
> From: Imre Deak <imre.deak@gmail.com>
> 
> The next patch adds sending a modeset-retry uevent after a link training
> failure to all MST connectors on link. This requires the atomic state,
> so pass it to intel_dp_start_link_train(). In case of SST where
> retraining still happens by calling this function directly instead of a
> modeset commit the atomic state is not available and NULL is passed
> instead. This is ok, since in this case the encoder's only DP connector
> is available from intel_dp->attached_connector not requiring the atomic
> state.
> 
> Signed-off-by: Imre Deak <imre.deak@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/g4x_dp.c                 | 2 +-
>  drivers/gpu/drm/i915/display/intel_ddi.c              | 6 +++---
>  drivers/gpu/drm/i915/display/intel_dp.c               | 2 +-
>  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 4 +++-
>  drivers/gpu/drm/i915/display/intel_dp_link_training.h | 4 +++-
>  5 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> index 4363e32a834df..0d7424a7581e6 100644
> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> @@ -707,7 +707,7 @@ static void intel_enable_dp(struct intel_atomic_state *state,
>  	intel_dp_configure_protocol_converter(intel_dp, pipe_config);
>  	intel_dp_check_frl_training(intel_dp);
>  	intel_dp_pcon_dsc_configure(intel_dp, pipe_config);
> -	intel_dp_start_link_train(intel_dp, pipe_config);
> +	intel_dp_start_link_train(state, intel_dp, pipe_config);
>  	intel_dp_stop_link_train(intel_dp, pipe_config);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 86358ec27e685..58e57a7704811 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2586,7 +2586,7 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	 *     Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent)
>  	 *     (timeout after 800 us)
>  	 */
> -	intel_dp_start_link_train(intel_dp, crtc_state);
> +	intel_dp_start_link_train(state, intel_dp, crtc_state);
>  
>  	/* 6.n Set DP_TP_CTL link training to Normal */
>  	if (!is_trans_port_sync_mode(crtc_state))
> @@ -2728,7 +2728,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	 *     Pattern, wait for 5 idle patterns (DP_TP_STATUS Min_Idles_Sent)
>  	 *     (timeout after 800 us)
>  	 */
> -	intel_dp_start_link_train(intel_dp, crtc_state);
> +	intel_dp_start_link_train(state, intel_dp, crtc_state);
>  
>  	/* 7.k Set DP_TP_CTL link training to Normal */
>  	if (!is_trans_port_sync_mode(crtc_state))
> @@ -2795,7 +2795,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  						   to_intel_connector(conn_state->connector),
>  						   crtc_state);
>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, true);
> -	intel_dp_start_link_train(intel_dp, crtc_state);
> +	intel_dp_start_link_train(state, intel_dp, crtc_state);
>  	if ((port != PORT_A || DISPLAY_VER(dev_priv) >= 9) &&
>  	    !is_trans_port_sync_mode(crtc_state))
>  		intel_dp_stop_link_train(intel_dp, crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7c824c5a13346..1f0b7cceea2dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5214,7 +5214,7 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
>  
>  		intel_dp_check_frl_training(intel_dp);
>  		intel_dp_pcon_dsc_configure(intel_dp, crtc_state);
> -		intel_dp_start_link_train(intel_dp, crtc_state);
> +		intel_dp_start_link_train(NULL, intel_dp, crtc_state);

I was going to suggest s/crtc_state/crtc/ for this, but the state==NULL
definitiely makes that impossible. I think we need to document each and
every function where the atomic state may be NULL and thus needs to be
passed the crtc_state as well. Otherwise someone (probably me) is likely
going to attempt some cleanups which will then explode, or someone will
just attempt to dig out something from the full atomic state (eg.
state->base.dev).

>  		intel_dp_stop_link_train(intel_dp, crtc_state);
>  		break;
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index e804f0b801c02..4f60daa97407d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1453,6 +1453,7 @@ intel_dp_128b132b_link_train(struct intel_dp *intel_dp,
>  
>  /**
>   * intel_dp_start_link_train - start link training
> + * @state: Atomic state
>   * @intel_dp: DP struct
>   * @crtc_state: state for CRTC attached to the encoder
>   *
> @@ -1461,7 +1462,8 @@ intel_dp_128b132b_link_train(struct intel_dp *intel_dp,
>   * fails.
>   * After calling this function intel_dp_stop_link_train() must be called.
>   */
> -void intel_dp_start_link_train(struct intel_dp *intel_dp,
> +void intel_dp_start_link_train(struct intel_atomic_state *state,
> +			       struct intel_dp *intel_dp,
>  			       const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.h b/drivers/gpu/drm/i915/display/intel_dp_link_training.h
> index 19836a8a4f904..f658230960333 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.h
> @@ -8,6 +8,7 @@
>  
>  #include <drm/display/drm_dp_helper.h>
>  
> +struct intel_atomic_state;
>  struct intel_crtc_state;
>  struct intel_dp;
>  
> @@ -25,7 +26,8 @@ void intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
>  void intel_dp_set_signal_levels(struct intel_dp *intel_dp,
>  				const struct intel_crtc_state *crtc_state,
>  				enum drm_dp_phy dp_phy);
> -void intel_dp_start_link_train(struct intel_dp *intel_dp,
> +void intel_dp_start_link_train(struct intel_atomic_state *state,
> +			       struct intel_dp *intel_dp,
>  			       const struct intel_crtc_state *crtc_state);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp,
>  			      const struct intel_crtc_state *crtc_state);
> -- 
> 2.43.3

-- 
Ville Syrjälä
Intel