Re: [PATCH i-g-t v3 1/3] lib/i915/i915_dp: add UHBR helpers and const-correct set_link_params

Hello Sowmiya,

On 05-08-2026 14:14, Sowmiya S wrote:
> - const char * for i915_dp_set_link_params() to allow string literals
> - Add i915_dp_is_uhbr_rate(): returns true for link rates >= 10 Gbps
> - Add i915_dp_get_next_lower_rate(): parses force_link_rate debugfs list
>    using strtok_r(), base-10 strtol() with errno/endptr checks
The message is a bare bullet list of what changed, with no "why". For a lib
patch that is fine as far as it goes, but please add a line saying these are
prep for 3/3 - as it stands, 1/3 adds two exported functions with no 
caller in
the tree, and a bisector landing here has no idea they are about to be 
used.

Also, imperative mood for the subject/body is the usual convention here

>
> Signed-off-by: Sowmiya S <[email protected]>
> ---
>   lib/i915/i915_dp.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-
>   lib/i915/i915_dp.h |  4 ++-
>   2 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/lib/i915/i915_dp.c b/lib/i915/i915_dp.c
> index e54058580..bf66347f5 100644
> --- a/lib/i915/i915_dp.c
> +++ b/lib/i915/i915_dp.c
> @@ -334,7 +334,7 @@ void i915_dp_reset_link_params(int drm_fd, igt_output_t *output)
>    * to set link rate and lane count to auto on exit
>    */
>   void i915_dp_set_link_params(int drm_fd, igt_output_t *output,
> -			     char *link_rate, char *lane_count)
> +			     const char *link_rate, const char *lane_count)
>   {
>   	bool valid;
>   	drmModeConnector *temp;
> @@ -390,3 +390,63 @@ int i915_dp_get_max_supported_rate(int drm_fd, const igt_output_t *output)
>   
>   	return max_rate;
>   }
> +
> +/**
> + * i915_dp_is_uhbr_rate:
> + * @link_rate: DP link rate in 10 kbit/s units, as reported by the
> + *	       i915_dp_*_link_rate debugfs files
> + *
> + * UHBR (Ultra High Bit Rate) link rates use 128b/132b channel encoding,
> + * everything below uses legacy 8b/10b. UHBR10 is 10 Gbps, i.e. 1000000 in
> + * 10 kbit/s units. Mirrors the kernel's drm_dp_is_uhbr_rate().
> + *
> + * Returns: true if @link_rate is a UHBR rate, false otherwise.
> + */
> +bool i915_dp_is_uhbr_rate(int link_rate)
> +{
> +	return link_rate >= 1000000;
> +}
  The magic 1000000 is now spelled out only in the doc block above. A named
  constant next to it would make the body self-documenting and would 
give the
  next test something to print in an error message:

         /* 10 kbit/s units */
         #define I915_DP_UHBR10_LINK_RATE        1000000

  Since this is a one-line predicate on an int, static inline in i915_dp.h
  (which is what I suggested last time) avoids a cross-TU call for a
  comparison. Not important, purely a preference - if you keep it 
out-of-line,
  fine.
> +
> +/**
> + * i915_dp_get_next_lower_rate:
> + * @drm_fd: A drm file descriptor
> + * @output: Target output
> + * @rate: reference link rate in 10 kbit/s units
> + *
> + * Parses the i915_dp_force_link_rate debugfs list (the source rates, printed
> + * with an "auto" entry and "[..]"/"*" markers) and returns the highest
> + * supported link rate strictly below @rate.
> + *
> + * Returns: highest supported rate below @rate in 10 kbit/s units, or 0 if none.
> + */
> +int i915_dp_get_next_lower_rate(int drm_fd, igt_output_t *output, int rate)
> +{
> +	char buf[512], *token, *saveptr = NULL;
> +	int res, next = 0;
> +
> +	res = igt_debugfs_read_connector_file(drm_fd, igt_output_name(output),
> +					      "i915_dp_force_link_rate",
> +					      buf, sizeof(buf));
> +	igt_assert_f(res == 0, "Unable to read %s/i915_dp_force_link_rate\n",
> +		     igt_output_name(output));
> +
> +	/* Delimiters strip the "auto" whitespace and the [ ] * markers. */
> +	for (token = strtok_r(buf, " \t\n[]*", &saveptr); token;
> +	     token = strtok_r(NULL, " \t\n[]*", &saveptr)) {
> +		char *endptr;
> +		long r;
> +
> +		if (!strcmp(token, "auto"))
> +			continue;
> +
> +		errno = 0;
> +		r = strtol(token, &endptr, 10);
> +		if (errno || endptr == token || *endptr)
> +			continue;
> +
> +		if (r < rate && r > next)
> +			next = r;
> +	}
> +
> +	return next;
> +}

this is still the source-rate list, which was point (b) on the v2 posting,
and I do not think it survives contract with the kernel. 
i915_dp_force_link_rate
is written by i915_dp_force_link_rate_write() ->

         static int parse_link_rate(...)
         {
                 ...
                 if (intel_dp_rate_index(intel_dp->source_rates,
intel_dp->num_source_rates,
                                         rate) < 0)
                         ret = -EINVAL;
         }

i.e. the write only validates against source_rates and *succeeds* for a rate
the sink never advertised. It is then consumed by:

         static int forced_link_rate(struct intel_dp_link_caps *link_caps)
         {
                 ...
                 len = intel_dp_common_len_rate_limit(link_caps,
  link_caps->forced_params.rate);
                 if (len == 0)
                         return intel_dp_common_rate(link_caps, 0);

                 return intel_dp_common_rate(link_caps, len - 1);
         }

so the effective pin is silently clamped down to the highest *common*
(source AND sink) rate <= what you wrote. Concretely, on a sink that 
advertises
UHBR20 and UHBR10 but not UHBR13.5 - which is legal, the three UHBR 
rates are
advertised independently - the caller in 3/3 does:

         write 2000000   -> pinned at 2000000, not sustained
         write 1350000   -> silently pinned at 1000000, logs
                            "Link rate 1350000 not sustained (got ...)"
         write 1000000   -> pinned at 1000000 again

so you get one log line naming a rate the hardware was never at, and one
full extra modeset (plus a fresh FB set - see my comment on 3/3) 
re-testing a
configuration that already failed.

intel_dp_allowed_link_configs is the file that gives you source AND sink AND
the current limits, printed as "<lanes>x<rate>". Parsing that instead would
make this function do what its name says.

Minor while you are here: r is a long and next is an int, so "next = r" 
is an
implicit narrowing. Harmless for link rates, but making r an int (or next a
long) keeps -Wconversion-clean builds happy for whoever turns that on.


Thanks and Regards
Kunal Joshi

> diff --git a/lib/i915/i915_dp.h b/lib/i915/i915_dp.h
> index b13629147..6b39d0560 100644
> --- a/lib/i915/i915_dp.h
> +++ b/lib/i915/i915_dp.h
> @@ -17,7 +17,9 @@ int i915_dp_get_pending_lt_failures(int drm_fd, igt_output_t *output);
>   int i915_dp_get_pending_retrain(int drm_fd, igt_output_t *output);
>   void i915_dp_reset_link_params(int drm_fd, igt_output_t *output);
>   void i915_dp_set_link_params(int drm_fd, igt_output_t *output,
> -			     char *link_rate, char *lane_count);
> +			     const char *link_rate, const char *lane_count);
>   int i915_dp_get_max_supported_rate(int drm_fd, const igt_output_t *output);
> +int i915_dp_get_next_lower_rate(int drm_fd, igt_output_t *output, int rate);
> +bool i915_dp_is_uhbr_rate(int link_rate);
>   
>   #endif