| Message ID | 20240520185822.3725844-10-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp_mst: Enable link training fallback | expand |
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
On Thu, May 23, 2024 at 05:41:17PM +0300, Ville Syrjälä wrote: > 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). This is meant to be a temporary solution, for maybe a few weeks after this gets merged after which retraining should be switched over to be a modeset in all cases; but agreed it's better to document that in intel_dp_start_link_train(), will add that. > > > 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
On Thu, May 23, 2024 at 05:47:36PM +0300, Imre Deak wrote: > On Thu, May 23, 2024 at 05:41:17PM +0300, Ville Syrjälä wrote: > > 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). > > This is meant to be a temporary solution, for maybe a few weeks after > this gets merged after which retraining should be switched over to be a > modeset in all cases; but agreed it's better to document that in > intel_dp_start_link_train(), will add that. The documentation should go all the way to the bottom IMO. Anything that gets passed the state by this guy can now also end up with state==NULL. > > > > > > 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
On Thu, May 23, 2024 at 05:54:10PM +0300, Ville Syrjälä wrote: > On Thu, May 23, 2024 at 05:47:36PM +0300, Imre Deak wrote: > > On Thu, May 23, 2024 at 05:41:17PM +0300, Ville Syrjälä wrote: > > > 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). > > > > This is meant to be a temporary solution, for maybe a few weeks after > > this gets merged after which retraining should be switched over to be a > > modeset in all cases; but agreed it's better to document that in > > intel_dp_start_link_train(), will add that. > > The documentation should go all the way to the bottom IMO. > Anything that gets passed the state by this guy can now also > end up with state==NULL. Ok, makes sense. > > > > > > > > > > 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 > > -- > Ville Syrjälä > Intel
On Thu, May 23, 2024 at 05:54:10PM +0300, Ville Syrjälä wrote: > On Thu, May 23, 2024 at 05:47:36PM +0300, Imre Deak wrote: > > On Thu, May 23, 2024 at 05:41:17PM +0300, Ville Syrjälä wrote: > > > 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). > > > > This is meant to be a temporary solution, for maybe a few weeks after > > this gets merged after which retraining should be switched over to be a > > modeset in all cases; but agreed it's better to document that in > > intel_dp_start_link_train(), will add that. > > The documentation should go all the way to the bottom IMO. > Anything that gets passed the state by this guy can now also > end up with state==NULL. Hmm. Is there some could_be_null(state) trick we could use to tell the compiler/static analyzers that this indeed may be NULL? Would essentially need some kind of nop NULL check I suppose.
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); 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);