On Thu, 05 Mar 2026, Arun R Murthy <[email protected]> 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 <[email protected]> > --- > 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
