Re: [PATCH 043/108] drm/i915/dp_link_caps: Add helper to get common rate index

[Imre Deak <imre.deak@intel.com>]

On Thu, May 07, 2026 at 11:26:25AM +0300, Jani Nikula wrote:
> On Tue, 28 Apr 2026, Imre Deak <imre.deak@intel.com> wrote:
> > Add intel_dp_link_caps_common_rate_idx() to look up supported link rates
> > tracked by the link_caps module by rate. This prepares for tracking these
> > capabilities internally within the link caps module.
> 
> It's a big series, and it's not obvious at which patch to reply for
> something generic. Might as well be here, as this function is preserved
> unchanged in the end.
> 
> When the rates and their handling were not abstracted, working with
> indexes to rates array made sense. It all started simple.
> 
> I think with the intel_dp_link_caps.c abstraction, having the rate index
> or link config index in the interface is a leaky abstraction. The caller
> has to deal with an index, or pos, or iter_pos, that the caller has no
> idea what it's an index to.
> 
> I think storing the various link configs in an array, and indexing it
> with some value, is an implementation detail that the interface is
> supposed to abstract. Imagine changing that implementation to use a
> linked list instead of an array. How much of the interface would you
> have to change? How much the indexes in the interface would get in the
> way of changing what's supposed to be an implementation detail? You'd
> end up having to deal with an "index" that the caller has no idea what
> it's an index to, and the callee having to translate that to a linked
> list where indexes make no sense.
> 
> I'm thinking maybe the interface should only deal with link
> configs. "This is the link config I have, give me the next one."

Thanks for taking the time to read through the patchset.

The following is also a result of an off-line discussion with you and
Ville.

Yes, the rate index and iteration position shouldn't be part of the
interface. The reason they still are is that I intended to keep the
iteration of the link fallback code in place and as-is, with the idea to
align that part with the new interface only as a follow-up.

However, re-checking this, the fallback iteration is really just an
open-coded form now of what is already provided via the interface's
for_each_dp_link_config() iterator. Using that iterator instead of
exposing new rate index and iteration position APIs makes more sense,
especially that this simplifies the fallback code a lot. I'll update the
patchset doing this, which also removes the rate index and iteration
position from the API.

The configuration index/mask is part of the interface to provide a way
for the link configuration computation code to support filtering the
set of valid configurations for a modeset (required for instance by the
intel_dp::use_max_params logic and the TEST_LANE_COUNT/LINK_RATE
autotest modesets). I agree that this filtering parameter should be more
hidden and not exposed via the actual configuration index the interace
uses internally for the filtering. I'll update the patchset to make the
filtering mechanism opaque to the interface user.

During the discussion you also had the idea to setup an iteration
object and pass all the iteration parameters through that object instead
of open-coding this at the call-sites of iterators. This simplifies the
iterator macro implementation and also allows for future extentions to
the iterator functionality. Will take this into use.

Ville mentioned that simplifying the link configuration table using
virtual table indices could be considered only as a follow-up to this
patchset, will drop that using the current rate/lane count internal
encoding for configuration entries.

The patchset should be reviewed/merged in smaller chunks. The cover
letter mentioned this too, a clarification to that is that the second
part (patches 27-89) should be further divided to smaller patchsets.
I'll follow up with an updated version of the patchet divided along
these lines, addressing the above feedback and other smaller issues
found by sashiko-bot fixed.

Thanks,
Imre

> BR,
> Jani.
> 
> 
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_dp_link_caps.c | 25 +++++++++++++++++++
> >  .../gpu/drm/i915/display/intel_dp_link_caps.h |  1 +
> >  .../drm/i915/display/intel_dp_link_training.c |  5 ++--
> >  drivers/gpu/drm/i915/display/intel_dp_test.c  |  7 +++---
> >  4 files changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_caps.c b/drivers/gpu/drm/i915/display/intel_dp_link_caps.c
> > index b1b78e7cda897..c99fc7704b3e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_caps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_caps.c
> > @@ -60,6 +60,31 @@ int intel_dp_common_rate(struct intel_dp *intel_dp, int index)
> >  	return intel_dp->common_rates[index];
> >  }
> >  
> > +/**
> > + * intel_dp_link_caps_common_rate_idx - get index of a common link rate
> > + * @link_caps: link capabilities state
> > + * @rate: common link rate to look up
> > + *
> > + * Look up @rate in the rate list currently supported by @link_caps, common to
> > + * both the source and the sink.
> > + *
> > + * The returned value is an index into the common rate list returned by
> > + * intel_dp_link_caps_all_common_rates() and accepted by
> > + * intel_dp_link_caps_common_rate().
> > + *
> > + * Return:
> > + * - Index of @rate in the current common rate list.
> > + * - %-1 if @rate is not present.
> > + */
> > +int intel_dp_link_caps_common_rate_idx(struct intel_dp_link_caps *link_caps, int rate)
> > +{
> > +	struct intel_dp *intel_dp = link_caps->dp;
> > +
> > +	return intel_dp_rate_index(intel_dp->common_rates,
> > +				   intel_dp->num_common_rates,
> > +				   rate);
> > +}
> > +
> >  /* Theoretical max between source and sink */
> >  int intel_dp_max_common_rate(struct intel_dp *intel_dp)
> >  {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_caps.h b/drivers/gpu/drm/i915/display/intel_dp_link_caps.h
> > index b2eb61272652e..7ec1612a044ed 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_caps.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_caps.h
> > @@ -14,6 +14,7 @@ struct intel_dp_link_config;
> >  int intel_dp_common_len_rate_limit(const struct intel_dp *intel_dp,
> >  				   int max_rate);
> >  int intel_dp_common_rate(struct intel_dp *intel_dp, int index);
> > +int intel_dp_link_caps_common_rate_idx(struct intel_dp_link_caps *link_caps, int rate);
> >  int intel_dp_max_common_rate(struct intel_dp *intel_dp);
> >  int intel_dp_link_caps_num_common_rates(struct intel_dp_link_caps *link_caps);
> >  void intel_dp_link_caps_all_common_rates(struct intel_dp_link_caps *link_caps,
> > 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 5218220b82b0b..456540925db55 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -1805,9 +1805,8 @@ static int reduce_link_rate(struct intel_dp *intel_dp, int current_rate)
> >  	if (forced_params.rate)
> >  		return -1;
> >  
> > -	rate_index = intel_dp_rate_index(intel_dp->common_rates,
> > -					 intel_dp->num_common_rates,
> > -					 current_rate);
> > +	rate_index = intel_dp_link_caps_common_rate_idx(link_caps,
> > +							current_rate);
> >  
> >  	if (rate_index <= 0)
> >  		return -1;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_test.c b/drivers/gpu/drm/i915/display/intel_dp_test.c
> > index 5cfa1dd411dab..0b791eee3b910 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_test.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_test.c
> > @@ -14,6 +14,7 @@
> >  #include "intel_display_regs.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp.h"
> > +#include "intel_dp_link_caps.h"
> >  #include "intel_dp_link_training.h"
> >  #include "intel_dp_mst.h"
> >  #include "intel_dp_test.h"
> > @@ -32,6 +33,7 @@ void intel_dp_test_compute_config(struct intel_dp *intel_dp,
> >  				  struct intel_crtc_state *pipe_config,
> >  				  struct link_config_limits *limits)
> >  {
> > +	struct intel_dp_link_caps *link_caps = intel_dp->link.caps;
> >  	struct intel_display *display = to_intel_display(intel_dp);
> >  
> >  	/* For DP Compliance we override the computed bpp for the pipe */
> > @@ -54,9 +56,8 @@ void intel_dp_test_compute_config(struct intel_dp *intel_dp,
> >  		 */
> >  		if (intel_dp_link_params_valid(intel_dp, intel_dp->compliance.test_link_rate,
> >  					       intel_dp->compliance.test_lane_count)) {
> > -			index = intel_dp_rate_index(intel_dp->common_rates,
> > -						    intel_dp->num_common_rates,
> > -						    intel_dp->compliance.test_link_rate);
> > +			index = intel_dp_link_caps_common_rate_idx(link_caps,
> > +								   intel_dp->compliance.test_link_rate);
> >  			if (index >= 0) {
> >  				limits->min_rate = intel_dp->compliance.test_link_rate;
> >  				limits->max_rate = intel_dp->compliance.test_link_rate;
> 
> -- 
> Jani Nikula, Intel