Re: [PATCH v3 4/5] drm/dp: Add an EDID quirk for the DPCD register access probe

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

On Thu, 05 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> Reading DPCD registers has side-effects and some of these can cause a
> problem for instance during link training. Based on this it's better to
> avoid the probing quirk done before each DPCD register read, limiting
> this to the monitor which requires it. Add an EDID quirk for this. Leave
> the quirk enabled by default, allowing it to be disabled after the
> monitor is detected.
>
> v2: Fix lockdep wrt. drm_dp_aux::hw_mutex when calling
>     drm_dp_dpcd_set_probe_quirk() with a dependent lock already held.
> v3: Add a helper for determining if DPCD probing is needed. (Jani)
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 42 +++++++++++++++++--------
>  drivers/gpu/drm/drm_edid.c              |  3 ++
>  include/drm/display/drm_dp_helper.h     |  6 ++++
>  include/drm/drm_edid.h                  |  3 ++
>  4 files changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index dc622c78db9d4..d0b9f672d743c 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -691,6 +691,34 @@ void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
>  
> +/**
> + * drm_dp_dpcd_set_dpcd_probe_quirk() - Set whether a probing before DPCD access is done

Musing, not sure if this needs to be called "quirk". This is just used
to enable/disable the extra probe. aux->dpcd_probe_disabled doesn't
mention the quirk either, and shouldn't.

Not a big deal.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> + * @aux: DisplayPort AUX channel
> + * @enable: Enable the probing if required
> + */
> +void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable)
> +{
> +	WRITE_ONCE(aux->dpcd_probe_disabled, !enable);
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_set_probe_quirk);
> +
> +static bool dpcd_access_needs_probe(struct drm_dp_aux *aux)
> +{
> +	/*
> +	 * HP ZR24w corrupts the first DPCD access after entering power save
> +	 * mode. Eg. on a read, the entire buffer will be filled with the same
> +	 * byte. Do a throw away read to avoid corrupting anything we care
> +	 * about. Afterwards things will work correctly until the monitor
> +	 * gets woken up and subsequently re-enters power save mode.
> +	 *
> +	 * The user pressing any button on the monitor is enough to wake it
> +	 * up, so there is no particularly good place to do the workaround.
> +	 * We just have to do it before any DPCD access and hope that the
> +	 * monitor doesn't power down exactly after the throw away read.
> +	 */
> +	return !aux->is_remote && !READ_ONCE(aux->dpcd_probe_disabled);
> +}
> +
>  /**
>   * drm_dp_dpcd_read() - read a series of bytes from the DPCD
>   * @aux: DisplayPort AUX channel (SST or MST)
> @@ -712,19 +740,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  {
>  	int ret;
>  
> -	/*
> -	 * HP ZR24w corrupts the first DPCD access after entering power save
> -	 * mode. Eg. on a read, the entire buffer will be filled with the same
> -	 * byte. Do a throw away read to avoid corrupting anything we care
> -	 * about. Afterwards things will work correctly until the monitor
> -	 * gets woken up and subsequently re-enters power save mode.
> -	 *
> -	 * The user pressing any button on the monitor is enough to wake it
> -	 * up, so there is no particularly good place to do the workaround.
> -	 * We just have to do it before any DPCD access and hope that the
> -	 * monitor doesn't power down exactly after the throw away read.
> -	 */
> -	if (!aux->is_remote) {
> +	if (dpcd_access_needs_probe(aux)) {
>  		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
>  		if (ret < 0)
>  			return ret;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9cca1e6e4736c..5f45820ad62d5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -248,6 +248,9 @@ static const struct edid_quirk {
>  	/* OSVR HDK and HDK2 VR Headsets */
>  	EDID_QUIRK('S', 'V', 'R', 0x1019, BIT(EDID_QUIRK_NON_DESKTOP)),
>  	EDID_QUIRK('A', 'U', 'O', 0x1111, BIT(EDID_QUIRK_NON_DESKTOP)),
> +
> +	/* HP ZR24w DP AUX DPCD access requires probing to prevent corruption. */
> +	EDID_QUIRK('H', 'W', 'P', 0x2869, BIT(DRM_EDID_QUIRK_DP_DPCD_PROBE)),
>  };
>  
>  /*
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index e4ca35143ff96..75fa9548aefa0 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -523,10 +523,16 @@ struct drm_dp_aux {
>  	 * @no_zero_sized: If the hw can't use zero sized transfers (NVIDIA)
>  	 */
>  	bool no_zero_sized;
> +
> +	/**
> +	 * @dpcd_probe_disabled: If probing before a DPCD access is disabled.
> +	 */
> +	bool dpcd_probe_disabled;
>  };
>  
>  int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
>  void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
> +void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable);
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  			 void *buffer, size_t size);
>  ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 77fd42608e706..3d1aecfec9b2a 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -110,6 +110,9 @@ struct detailed_data_string {
>  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
>  
>  enum drm_edid_quirk {
> +	/* Do a dummy read before DPCD accesses, to prevent corruption. */
> +	DRM_EDID_QUIRK_DP_DPCD_PROBE,
> +
>  	DRM_EDID_QUIRK_NUM,
>  };

-- 
Jani Nikula, Intel