Re: [PATCH RFC 1/4] drm/display/dp: Read LTTPR caps without DPRX caps

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 05 Mar 2026, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> We at present have drm_dp_Read_lttpr_common_caps to read the LTTPR caps,
> but this function required DPRX caps to be passed. As per the DP2.1 spec
> section 3.6.8.6.1, section 2.12.1, section 2.12.3 (Link Policy) the
> LTTPR caps is to be read first followed by the DPRX capability.
> Hence adding another function to read the LTTPR caps without the need
> for DPRX caps.

If the spec says something, why are we keeping the function that does it
the other way?

> In order to handle the issue
> https://gitlab.freedesktop.org/drm/intel/-/issues/4531
> of reading corrupted values for LTTPR caps on few pannels with DP Rev 1.2
> the workaround of reducing the block size to 1 and reading one block at a
> time is done by checking for a valid link rate.
>
> Fixes: 657586e474bd ("drm/i915: Add a DP1.2 compatible way to read LTTPR capabilities")

You're not calling the code being added here. This can't fix anything on
its own. This is not how the Fixes: tag works.

> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 63 +++++++++++++++++++++++++++++++++
>  include/drm/display/drm_dp_helper.h     |  2 ++
>  2 files changed, 65 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index a697cc227e28964cd8322803298178e7d788e820..9fe7db73027a43b01c4d12927f1f0e61444658e5 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -3050,6 +3050,69 @@ static int drm_dp_read_lttpr_regs(struct drm_dp_aux *aux,
>  	return 0;
>  }
>  
> +static bool drm_dp_valid_link_rate(u8 link_rate)
> +{
> +	switch (link_rate) {
> +	case 0x06:
> +	case 0x0a:
> +	case 0x14:
> +	case 0x1e:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +/**
> + * drm_dp_read_lttpr_caps - read the LTTPR capabilities
> + * @aux: DisplayPort AUX channel
> + * @caps: buffer to return the capability info in
> + *
> + * Read capabilities common to all LTTPRs.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_read_lttpr_caps(struct drm_dp_aux *aux,
> +			   u8 caps[DP_LTTPR_COMMON_CAP_SIZE])
> +{
> +	/*
> +	 * At least the DELL P2715Q monitor with a DPCD_REV < 0x14 returns
> +	 * corrupted values when reading from the 0xF0000- range with a block
> +	 * size bigger than 1.
> +	 * For DP as per the spec DP2.1 section 3.6.8.6.1, section 2.12.1, section
> +	 * 2.12.3 (Link Policy) the LTTPR caps is to be read first followed by the
> +	 * DPRX capability.
> +	 * So ideally we dont have DPCD_REV yet to check for the revision, instead
> +	 * check for the correctness of the read value and in found corrupted read
> +	 * block by block.
> +	 */
> +	int block_size;
> +	int offset;
> +	int ret;
> +	int address = DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV;
> +	int buf_size = DP_LTTPR_COMMON_CAP_SIZE;
> +
> +	ret = drm_dp_dpcd_read_data(aux, address, &caps, buf_size);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (caps[0] == 0x14) {
> +		if (!drm_dp_valid_link_rate(caps[1])) {

So you first read the whole thing once, and then in some cases read the
whole thing again one byte at a time?

Everything about this smells like a quirk for a specific display, not
something you do normally. We shouldn't have to have two ways to read
the lttpr caps in the normal case.

> +			block_size = 1;

What's the point with the variable?

> +			for (offset = 0; offset < buf_size; offset += block_size) {
> +				ret = drm_dp_dpcd_read_data(aux,
> +							    address + offset,
> +							    &caps[offset],
> +							    block_size);
> +				if (ret < 0)
> +					return ret;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_read_lttpr_caps);
> +
>  /**
>   * drm_dp_read_lttpr_common_caps - read the LTTPR common capabilities
>   * @aux: DisplayPort AUX channel
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 1d0acd58f48676f60ff6a07cc6812f72cbb452e8..def145e67011c325b790c807f934b288304260c1 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -755,6 +755,8 @@ bool drm_dp_read_sink_count_cap(struct drm_connector *connector,
>  				const struct drm_dp_desc *desc);
>  int drm_dp_read_sink_count(struct drm_dp_aux *aux);
>  
> +int drm_dp_read_lttpr_caps(struct drm_dp_aux *aux,
> +				  u8 caps[DP_LTTPR_COMMON_CAP_SIZE]);
>  int drm_dp_read_lttpr_common_caps(struct drm_dp_aux *aux,
>  				  const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  				  u8 caps[DP_LTTPR_COMMON_CAP_SIZE]);

-- 
Jani Nikula, Intel