Re: [PATCH] drm/dp: Change AUX DPCD probe address from LANE0_1_STATUS to TRAINING_PATTERN_SET

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jul 14, 2025 at 11:06:52PM +0300, Imre Deak wrote:
> On Mon, Jul 14, 2025 at 11:00:08AM +0200, Thomas Zimmermann wrote:
> > 
> > Am 14.07.25 um 10:57 schrieb Imre Deak:
> > > Hi,
> > > 
> > > On Thu, Jul 10, 2025 at 02:28:28PM +0300, Imre Deak wrote:
> > > > Hi Thomas, Maxime, Maarten,
> > > > 
> > > > the patch this change fixes (commit a40c5d727b81) was merged via
> > > > drm-intel and is also part of v6.16-rc4 (there cherry-picked in commit
> > > > a3ef3c2da675).
> > > > 
> > > > Are you ok with merging this fix via drm-intel as well, so that it could
> > > > be still merged to v6.16 before that's released?
> > > any objections to merging this patch to drm-intel? If not, could someone
> > > ack it?
> > 
> > Sure, a-b me.
> 
> Ok, thanks.
> 
> I pushed the patch to drm-intel-next, thanks for the report, testing,
> review and ack.
> 
> I'd like to request that this patch be applied to drm-intel-fixes as
> well, so that it can still make it to v6.16. To do that, the change
> needs to be rebased on drm-intel-fixes, I attached the corresponding
> patch.

pushed to drm-intel-fixes. Thanks for the backport

> 
> Thanks,
> Imre
> 
> > Best regards
> > Thomas
> > 
> > > 
> > > Thanks,
> > > Imre
> > > 
> > > > Thanks,
> > > > Imre
> > > > 
> > > > On Wed, Jul 09, 2025 at 12:23:31AM +0300, Imre Deak wrote:
> > > > > Commit a40c5d727b81 ("drm/dp: Change AUX DPCD probe address from
> > > > > DPCD_REV to LANE0_1_STATUS") stopped using the DPCD_REV register for
> > > > > DPCD probing, since this results in link training failures at least when
> > > > > using an Intel Barlow Ridge TBT hub at UHBR link rates (the
> > > > > DP_INTRA_HOP_AUX_REPLY_INDICATION never getting cleared after the failed
> > > > > link training). Since accessing DPCD_REV during link training is
> > > > > prohibited by the DP Standard, LANE0_1_STATUS (0x202) was used instead,
> > > > > as it falls within the Standard's valid register address range
> > > > > (0x102-0x106, 0x202-0x207, 0x200c-0x200f, 0x2216) and it fixed the link
> > > > > training on the above TBT hub.
> > > > > 
> > > > > However, reading the LANE0_1_STATUS register also has a side-effect at
> > > > > least on a Novatek eDP panel, as reported on the Closes: link below,
> > > > > resulting in screen flickering on that panel. One clear side-effect when
> > > > > doing the 1-byte probe reads from LANE0_1_STATUS during link training
> > > > > before reading out the full 6 byte link status starting at the same
> > > > > address is that the panel will report the link training as completed
> > > > > with voltage swing 0. This is different from the normal, flicker-free
> > > > > scenario when no DPCD probing is done, the panel reporting the link
> > > > > training complete with voltage swing 2.
> > > > > 
> > > > > Using the TRAINING_PATTERN_SET register for DPCD probing doesn't have
> > > > > the above side-effect, the panel will link train with voltage swing 2 as
> > > > > expected and it will stay flicker-free. This register is also in the
> > > > > above valid register range and is unlikely to have a side-effect as that
> > > > > of LANE0_1_STATUS: Reading LANE0_1_STATUS is part of the link training
> > > > > CR/EQ sequences and so it may cause a state change in the sink - even if
> > > > > inadvertently as I suspect in the case of the above Novatek panel. As
> > > > > opposed to this, reading TRAINING_PATTERN_SET is not part of the link
> > > > > training sequence (it must be only written once at the beginning of the
> > > > > CR/EQ sequences), so it's unlikely to cause any state change in the
> > > > > sink.
> > > > > 
> > > > > As a side-note, this Novatek panel also lacks support for TPS3, while
> > > > > claiming support for HBR2, which violates the DP Standard (the Standard
> > > > > mandating TPS3 for HBR2).
> > > > > 
> > > > > Besides the Novatek panel (PSR 1), which this change fixes, I also
> > > > > verified the change on a Samsung (PSR 1) and an Analogix (PSR 2) eDP
> > > > > panel as well as on the Intel Barlow Ridge TBT hub.
> > > > > 
> > > > > Note that in the drm-tip tree (targeting the v6.17 kernel version) the
> > > > > i915 and xe drivers keep DPCD probing enabled only for the panel known
> > > > > to require this (HP ZR24w), hence those drivers in drm-tip are not
> > > > > affected by the above problem.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Fixes: a40c5d727b81 ("drm/dp: Change AUX DPCD probe address from DPCD_REV to LANE0_1_STATUS")
> > > > > Reported-and-tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > > > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14558
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/display/drm_dp_helper.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > index 1c3920297906b..1ecc3df7e3167 100644
> > > > > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > @@ -742,7 +742,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> > > > >   	int ret;
> > > > >   	if (dpcd_access_needs_probe(aux)) {
> > > > > -		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
> > > > > +		ret = drm_dp_dpcd_probe(aux, DP_TRAINING_PATTERN_SET);
> > > > >   		if (ret < 0)
> > > > >   			return ret;
> > > > >   	}
> > > > > -- 
> > > > > 2.44.2
> > > > > 
> > 
> > -- 
> > --
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Frankenstrasse 146, 90461 Nuernberg, Germany
> > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> > HRB 36809 (AG Nuernberg)
> > 

> From d468740cb8688070ebbd534c2849b49729ae2af8 Mon Sep 17 00:00:00 2001
> From: Imre Deak <imre.deak@intel.com>
> Date: Wed, 9 Jul 2025 00:23:31 +0300
> Subject: [PATCH] drm/dp: Change AUX DPCD probe address from LANE0_1_STATUS to
>  TRAINING_PATTERN_SET
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Commit a40c5d727b81 ("drm/dp: Change AUX DPCD probe address from
> DPCD_REV to LANE0_1_STATUS") stopped using the DPCD_REV register for
> DPCD probing, since this results in link training failures at least when
> using an Intel Barlow Ridge TBT hub at UHBR link rates (the
> DP_INTRA_HOP_AUX_REPLY_INDICATION never getting cleared after the failed
> link training). Since accessing DPCD_REV during link training is
> prohibited by the DP Standard, LANE0_1_STATUS (0x202) was used instead,
> as it falls within the Standard's valid register address range
> (0x102-0x106, 0x202-0x207, 0x200c-0x200f, 0x2216) and it fixed the link
> training on the above TBT hub.
> 
> However, reading the LANE0_1_STATUS register also has a side-effect at
> least on a Novatek eDP panel, as reported on the Closes: link below,
> resulting in screen flickering on that panel. One clear side-effect when
> doing the 1-byte probe reads from LANE0_1_STATUS during link training
> before reading out the full 6 byte link status starting at the same
> address is that the panel will report the link training as completed
> with voltage swing 0. This is different from the normal, flicker-free
> scenario when no DPCD probing is done, the panel reporting the link
> training complete with voltage swing 2.
> 
> Using the TRAINING_PATTERN_SET register for DPCD probing doesn't have
> the above side-effect, the panel will link train with voltage swing 2 as
> expected and it will stay flicker-free. This register is also in the
> above valid register range and is unlikely to have a side-effect as that
> of LANE0_1_STATUS: Reading LANE0_1_STATUS is part of the link training
> CR/EQ sequences and so it may cause a state change in the sink - even if
> inadvertently as I suspect in the case of the above Novatek panel. As
> opposed to this, reading TRAINING_PATTERN_SET is not part of the link
> training sequence (it must be only written once at the beginning of the
> CR/EQ sequences), so it's unlikely to cause any state change in the
> sink.
> 
> As a side-note, this Novatek panel also lacks support for TPS3, while
> claiming support for HBR2, which violates the DP Standard (the Standard
> mandating TPS3 for HBR2).
> 
> Besides the Novatek panel (PSR 1), which this change fixes, I also
> verified the change on a Samsung (PSR 1) and an Analogix (PSR 2) eDP
> panel as well as on the Intel Barlow Ridge TBT hub.
> 
> Note that in the drm-tip tree (targeting the v6.17 kernel version) the
> i915 and xe drivers keep DPCD probing enabled only for the panel known
> to require this (HP ZR24w), hence those drivers in drm-tip are not
> affected by the above problem.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Fixes: a40c5d727b81 ("drm/dp: Change AUX DPCD probe address from DPCD_REV to LANE0_1_STATUS")
> Reported-and-tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14558
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://lore.kernel.org/r/20250708212331.112898-1-imre.deak@intel.com
> (cherry picked from commit bba9aa41654036534d86b198f5647a9ce15ebd7f)
> [Imre: Rebased on drm-intel-fixes]
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index dc622c78db9d..ea78c6c8ca7a 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -725,7 +725,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  	 * monitor doesn't power down exactly after the throw away read.
>  	 */
>  	if (!aux->is_remote) {
> -		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
> +		ret = drm_dp_dpcd_probe(aux, DP_TRAINING_PATTERN_SET);
>  		if (ret < 0)
>  			return ret;
>  	}
> -- 
> 2.44.2
>