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

[Imre Deak <imre.deak@intel.com>]

On Wed, Jun 04, 2025 at 01:11:08PM +0300, Jani Nikula wrote:
> On Tue, 03 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.
> >
> > 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 | 13 ++++++++++++-
> >  drivers/gpu/drm/drm_edid.c              |  3 +++
> >  include/drm/display/drm_dp_helper.h     |  6 ++++++
> >  include/drm/drm_edid.h                  |  3 ++-
> >  4 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index dc622c78db9d4..4dad677ac6ebe 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -691,6 +691,17 @@ 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
> > + * @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);
> 
> We don't use this yet, which feels a bit odd.

It's used in the next driver patch and I wanted to keep the DRM core and
driver changes separate.

> > +
> >  /**
> >   * drm_dp_dpcd_read() - read a series of bytes from the DPCD
> >   * @aux: DisplayPort AUX channel (SST or MST)
> > @@ -724,7 +735,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> >  	 * 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 (!aux->is_remote && !READ_ONCE(aux->dpcd_probe_disabled)) {
> 
> I think it would be worth it to add a small helper to decide whether to
> do a dpcd probe. It would include the ->is_remote check as well, and the
> big comment could be moved there, simplifying drm_dp_dpcd_read().

Ok, can do this.

> 
> >  		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 e867315253493..9250b425ae230 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -255,6 +255,9 @@ static const struct edid_quirk {
> >  };
> >  
> >  static const struct edid_quirk global_edid_quirk_list[] = {
> > +	/* HP ZR24w DP AUX DPCD access requires probing to prevent corruption. */
> > +	DRM_EDID_QUIRK(drm_edid_encode_panel_id('H', 'W', 'P', 0x2869), PANEL_NAME_ANY,
> > +		       BIT(DRM_EDID_QUIRK_DP_DPCD_PROBE)),
> 
> So I think we should keep using EDID_QUIRK(), and maybe add
> EDID_QUIRK_NAME() companion to also assign a name != NULL, so we don't
> have to pass PANEL_NAME_ANY to all uses. Less verbose that way I think.

Imo one macro should accept a quirk matching any panel ID or any panel
name; but for now I can drop the change adding a way for that and use
EDID_QUIRK() here.

> 
> >  };
> >  
> >  /*
> > 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;
> 
> Is this a negative just so it's false by default?

Yes.

> 
> >  };
> >  
> >  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 3d8e168521c82..a878805c81d97 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -110,7 +110,8 @@ struct detailed_data_string {
> >  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
> >  
> >  enum drm_edid_quirk {
> > -	DRM_EDID_QUIRK_NONE,
> > +	/* Do a dummy read before DPCD accesses, to prevent corruption. */
> > +	DRM_EDID_QUIRK_DP_DPCD_PROBE,
> >  };
> >  
> >  struct detailed_data_monitor_range {
> 
> -- 
> Jani Nikula, Intel