Re: [PATCH 1/5] drm/i915/dp: Add helpers to reset link params

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

On Fri, May 22, 2026 at 01:29:39PM +0300, Jani Nikula wrote:
> On Fri, 22 May 2026, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, May 22, 2026 at 12:36:27AM +0300, Ville Syrjälä wrote:
> >> On Mon, May 18, 2026 at 02:24:22PM +0300, Imre Deak wrote:
> >> > Add helpers to defer and handle link params resets instead of
> >> > open-coding the same. Rename intel_dp_reset_link_params() to
> >> > intel_dp_reset_link_params_force() to align its name with the new
> >> > deferred reset helpers.
> >> > 
> >> > When deferring a reset, return whether a new reset was queued, used by a
> >> > follow-up change.
> >> > 
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
> >> >  drivers/gpu/drm/i915/display/intel_ddi.c      |  2 +-
> >> >  drivers/gpu/drm/i915/display/intel_dp.c       | 41 +++++++++++++++----
> >> >  drivers/gpu/drm/i915/display/intel_dp.h       |  3 +-
> >> >  .../drm/i915/display/intel_dp_link_training.c |  4 +-
> >> >  5 files changed, 38 insertions(+), 14 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> >> > index 5ff1cdf4581a5..c20a97e21419b 100644
> >> > --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> >> > +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> >> > @@ -1265,7 +1265,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >> >  
> >> >  	intel_dp->DP = intel_de_read(display, intel_dp->output_reg);
> >> >  
> >> > -	intel_dp->reset_link_params = true;
> >> > +	intel_dp_reset_link_params_defer(intel_dp);
> >> >  	intel_dp_invalidate_source_oui(intel_dp);
> >> >  
> >> >  	if (display->platform.valleyview || display->platform.cherryview)
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > index 86520848892e0..77819aaeccb76 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > @@ -4664,7 +4664,7 @@ static void intel_ddi_encoder_reset(struct drm_encoder *encoder)
> >> >  	struct intel_dp *intel_dp = enc_to_intel_dp(to_intel_encoder(encoder));
> >> >  	struct intel_digital_port *dig_port = enc_to_dig_port(to_intel_encoder(encoder));
> >> >  
> >> > -	intel_dp->reset_link_params = true;
> >> > +	intel_dp_reset_link_params_defer(intel_dp);
> >> >  	intel_dp_invalidate_source_oui(intel_dp);
> >> >  
> >> >  	intel_pps_encoder_reset(intel_dp);
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > index 1920d2f026665..13163dd085e91 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > @@ -3710,7 +3710,11 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >> >  	intel_dp->lane_count = lane_count;
> >> >  }
> >> >  
> >> > -void intel_dp_reset_link_params(struct intel_dp *intel_dp)
> >> > +/*
> >> > + * Reset link params now, preserving any deferred connector
> >> > + * detect-time reset request.
> >> > + */
> >> > +void intel_dp_reset_link_params_force(struct intel_dp *intel_dp)
> >> >  {
> >> >  	intel_dp->link.max_lane_count = intel_dp_max_common_lane_count(intel_dp);
> >> >  	intel_dp->link.max_rate = intel_dp_max_common_rate(intel_dp);
> >> > @@ -3720,6 +3724,28 @@ void intel_dp_reset_link_params(struct intel_dp *intel_dp)
> >> >  	intel_dp->link.seq_train_failures = 0;
> >> >  }
> >> >  
> >> > +/*
> >> > + * Reset link params during the next connector detect.
> >> > + * Return %true if a new reset was queued.
> >> > + */
> >> > +bool intel_dp_reset_link_params_defer(struct intel_dp *intel_dp)
> >> 
> >> I find the intel_dp_reset_link_params_defer() vs.
> >> intel_dp_reset_link_params_force() naming rather confusing.
> >> 
> >> Can't immediately think of a really good name for
> >> intel_dp_reset_link_params_defer() so maybe it's better to not
> >> have a function for it at all (ie. just drop this patch)?
> >
> > The idea was to have an interface to reset the link params directly or
> > in a deferred way, instead of a direct access of the flag.
> >
> > The names are not great yes. I could use what Jouni suggested instead,
> > or if the above argument is not good enough I can also drop this patch.
> 
> I suggest intel_dp_reset_link_params() keeps its name, or gets renamed
> to intel_dp_link_params_reset(). It just does the thing, no "force".
> 
> Then the other two use something like:
> 
> - submit/process
> - queue/process
> - stage/handle
> 
> or some combination i.e. something like:
> 
> intel_dp_link_params_reset()
> intel_dp_link_params_reset_submit()
> intel_dp_link_params_reset_process()

All of it kinda leaves it a bit unclear how these things
are related, and then one probably has to read through
them to figure out what they're actually doing.

Hence why I think just setting/checking the flag on the
spot might be the clearest approach. You can immediately
find where it's set vs. checked.

-- 
Ville Syrjälä
Intel