On Fri, 06 Jun 2025, Imre Deak <imre.d...@intel.com> wrote:
> On Fri, Jun 06, 2025 at 04:50:03PM +0300, Imre Deak wrote:
>> On Fri, Jun 06, 2025 at 04:44:37PM +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. The only known problematic
>> > > monitor is an external SST sink, so keep the quirk disabled always for
>> > > eDP and MST sinks. Reenable the quirk after a hotplug event and after
>> > > resuming from a power state without hotplug support, until the
>> > > subsequent EDID based detection.
>> > >
>> > > 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/i915/display/intel_dp.c | 11 +++++++++--
>> > > drivers/gpu/drm/i915/display/intel_dp_aux.c | 2 ++
>> > > drivers/gpu/drm/i915/display/intel_hotplug.c | 10 ++++++++++
>> > > 3 files changed, 21 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > > index 208a953b04a2f..d65a18fc1aeb9 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > > @@ -5747,6 +5747,11 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>> > > /* Below we depend on display info having been updated */
>> > > drm_edid_connector_update(&connector->base, drm_edid);
>> > >
>> > > + if (!intel_dp_is_edp(intel_dp))
>> >
>> > Why does this depend on !edp?
>> >
>> > Feels like unnecessary optimization based on your knowledge of that one
>> > specific display.
>>
>> The detection itself requires probing before each DPCD access. I want to
>> avoid it whenever possible and since the quirk is relevant only the
>> particular HP external display, we can avoid the probing on eDP
>> completely.
>
> Ok, the eDP check here can be removed, as the panel's EDID panel ID
> should not match the quirk's panel ID. Will remove it.
I'm wondering if we should add a local "do the quirk" helper that checks
for eDP and mst and the actual quirk. Not sure if it would make this
more readable.
BR,
Jani.
>
>> > > + drm_dp_dpcd_set_probe_quirk(&intel_dp->aux,
>> > > +
>> > > drm_edid_has_quirk(&connector->base,
>> > > +
>> > > DRM_EDID_QUIRK_DP_DPCD_PROBE));
>> > > +
>> > > vrr_capable = intel_vrr_is_capable(connector);
>> > > drm_dbg_kms(display->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
>> > > connector->base.base.id, connector->base.name,
>> > > str_yes_no(vrr_capable));
>> > > @@ -5881,6 +5886,7 @@ intel_dp_detect(struct drm_connector *_connector,
>> > > intel_dp_print_rates(intel_dp);
>> > >
>> > > if (intel_dp->is_mst) {
>> > > + drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, false);
>> >
>> > Isn't this excessive? Haven't we already set the quirk state?
>>
>> No, this is the MST root connector's detection and we don't read the EDID
>> for it (we read it for MST non-root connectors, but those are not
>> relavant in any case). So this should be set here explicitly, with the
>> same justification as above for eDP (on MST the probing is never needed,
>> so we can avoid it on such links completely).
>>
>> >
>> > > /*
>> > > * If we are in MST mode then this connector
>> > > * won't appear connected or have anything
>> > > @@ -6321,10 +6327,11 @@ intel_dp_hpd_pulse(struct intel_digital_port
>> > > *dig_port, bool long_hpd)
>> > > * complete the DP tunnel BW request for the latter
>> > > connector/encoder
>> > > * waiting for this encoder's DPRX read, perform a dummy read
>> > > here.
>> > > */
>> > > - if (long_hpd)
>> > > + if (long_hpd) {
>> > > + drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, true);
>> > > +
>> > > intel_dp_read_dprx_caps(intel_dp, dpcd);
>> > >
>> > > - if (long_hpd) {
>> > > intel_dp->reset_link_params = true;
>> > > intel_dp_invalidate_source_oui(intel_dp);
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> > > b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> > > index bf8e8e0cc19c9..410252ba6fd16 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> > > @@ -835,6 +835,8 @@ void intel_dp_aux_init(struct intel_dp *intel_dp)
>> > >
>> > > intel_dp->aux.transfer = intel_dp_aux_transfer;
>> > > cpu_latency_qos_add_request(&intel_dp->pm_qos,
>> > > PM_QOS_DEFAULT_VALUE);
>> > > +
>> > > + drm_dp_dpcd_set_probe_quirk(&intel_dp->aux,
>> > > !intel_dp_is_edp(intel_dp));
>> > > }
>> > >
>> > > static enum aux_ch default_aux_ch(struct intel_encoder *encoder)
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
>> > > b/drivers/gpu/drm/i915/display/intel_hotplug.c
>> > > index 74fe398663d63..1093c6c3714c0 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
>> > > @@ -33,6 +33,7 @@
>> > > #include "intel_display_core.h"
>> > > #include "intel_display_rpm.h"
>> > > #include "intel_display_types.h"
>> > > +#include "intel_dp.h"
>> > > #include "intel_hdcp.h"
>> > > #include "intel_hotplug.h"
>> > > #include "intel_hotplug_irq.h"
>> > > @@ -906,9 +907,18 @@ void intel_hpd_poll_enable(struct intel_display
>> > > *display)
>> > > */
>> > > void intel_hpd_poll_disable(struct intel_display *display)
>> > > {
>> > > + struct intel_encoder *encoder;
>> > > +
>> > > if (!HAS_DISPLAY(display))
>> > > return;
>> > >
>> > > + for_each_intel_dp(display->drm, encoder) {
>> > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> > > +
>> > > + if (!intel_dp_is_edp(intel_dp))
>> > > + drm_dp_dpcd_set_probe_quirk(&intel_dp->aux,
>> > > true);
>> > > + }
>> > > +
>> > > WRITE_ONCE(display->hotplug.poll_enabled, false);
>> > >
>> > > spin_lock_irq(&display->irq.lock);
>> >
>> > --
>> > Jani Nikula, Intel
--
Jani Nikula, Intel
