On Thu, Jun 05, 2025 at 04:11:55PM +0300, Jani Nikula wrote:
> On Thu, 05 Jun 2025, Imre Deak <imre.d...@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.syrj...@linux.intel.com>
> > Cc: Jani Nikula <jani.nik...@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.d...@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.
Ok, makes sense, I can rename it also removing the repetition in the
name to drm_dp_dcpd_set_probe().
> Not a big deal.
>
> Reviewed-by: Jani Nikula <jani.nik...@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
