On Thu, Jun 04, 2020 at 01:18:59AM +0300, Imre Deak wrote: > Some TypeC -> native DP adapters, at least the Club CAC-1557 adapter, > incorrectly filter out HPD short pulses with a duration less than ~540 > usec, leading to MST probe failures. > > According to the DP alt mode specification adapters should forward short > pulses with a duration greater than 250 usec. According to the DP > specificatin DP sources should detect short pulses in the > 500 usec -> 2 ms range.
IIRC it was 250 usec -> 2 ms as well in the DP spec. 500 usec -> 1 ms is the duration of the short hpd the signalling side should use. > Based on this filtering out short pulses with a > duration less than 540 usec is incorrect. > > To make such adapters work add support for a driver polling on MST > inerrupt flags, and wire this up in the i915 driver. The sink can clear > an interrupt it raised after 110 ms if the source doesn't respond, so > use a 50 ms poll period to avoid missing an interrupt. Polling of the > MST interrupt flags is explicitly allowed by the DP specification. > > This fixes MST probe failures I saw using this adapter and a DELL U2515H > monitor. > > v2: > - Fix the wait event timeout for the no-poll case. > > Signed-off-by: Imre Deak <[email protected]> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 19 ++++++++++++++++--- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 +++++++++++++++ > include/drm/drm_dp_mst_helper.h | 1 + > 3 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 5bc72e800b85..4e987a513df8 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1178,11 +1178,24 @@ static int drm_dp_mst_wait_tx_reply(struct > drm_dp_mst_branch *mstb, > struct drm_dp_sideband_msg_tx *txmsg) > { > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > + unsigned long wait_timeout = msecs_to_jiffies(4000); > + unsigned long wait_expires = jiffies + wait_timeout; > int ret; > > - ret = wait_event_timeout(mgr->tx_waitq, > - check_txmsg_state(mgr, txmsg), > - (4 * HZ)); > + for (;;) { > + ret = wait_event_timeout(mgr->tx_waitq, > + check_txmsg_state(mgr, txmsg), > + mgr->cbs->update_hpd_irq_state ? > + msecs_to_jiffies(50) : > + wait_timeout); > + > + if (ret || !mgr->cbs->update_hpd_irq_state || > + time_after(jiffies, wait_expires)) > + break; First I thought this was changing the behaviour when the callback isn't provided, but then I noticed the ?: stuff for the timeout. I think this stuff deserves a comment to explain why we would ever do such a thing instead of simply waiting like we did before. > + > + mgr->cbs->update_hpd_irq_state(mgr); > + } > + > mutex_lock(&mgr->qlock); > if (ret > 0) { > if (txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT) { > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index d18b406f2a7d..1ff7d0096262 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -765,8 +765,23 @@ static struct drm_connector > *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > return NULL; > } > > +static void > +intel_dp_mst_update_hpd_irq_state(struct drm_dp_mst_topology_mgr *mgr) > +{ > + struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > + > + spin_lock_irq(&i915->irq_lock); > + i915->hotplug.short_port_mask |= BIT(dig_port->base.port); > + spin_unlock_irq(&i915->irq_lock); > + > + queue_work(i915->hotplug.dp_wq, &i915->hotplug.dig_port_work); I might suggest putting this code right next to intel_hpd_irq_handler() so that people can actually see it when working on the hotplug code. > +} > + > static const struct drm_dp_mst_topology_cbs mst_cbs = { > .add_connector = intel_dp_add_mst_connector, > + .update_hpd_irq_state = intel_dp_mst_update_hpd_irq_state, > }; > > static struct intel_dp_mst_encoder * > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 9e1ffcd7cb68..c902f4380200 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -475,6 +475,7 @@ struct drm_dp_mst_topology_mgr; > struct drm_dp_mst_topology_cbs { > /* create a connector for a port */ > struct drm_connector *(*add_connector)(struct drm_dp_mst_topology_mgr > *mgr, struct drm_dp_mst_port *port, const char *path); > + void (*update_hpd_irq_state)(struct drm_dp_mst_topology_mgr *mgr); I guess a bit of docs for this might be nice. Maybe s/update/poll/ might make the intention more clear? Not sure. > }; > > #define DP_MAX_PAYLOAD (sizeof(unsigned long) * 8) > -- > 2.23.1 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
