| Message ID | 20200603221859.9600-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 <imre.deak@intel.com> > --- > 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 > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 04, 2020 at 06:12:27PM +0300, Ville Syrjälä wrote: > 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. Ah, correct (and this is what makes actually sense). For reference it's described under "5.1.4 Source Device Behavior upon HPD Pulse Detection" > > 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 <imre.deak@intel.com> > > --- > > 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. Ok, will add a compact form of the commit log explanation. > > > + > > + 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. Ok. > > > +} > > + > > 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. Ok. > > > }; > > > > #define DP_MAX_PAYLOAD (sizeof(unsigned long) * 8) > > -- > > 2.23.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
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; + + 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); +} + 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); }; #define DP_MAX_PAYLOAD (sizeof(unsigned long) * 8)
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. 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 <imre.deak@intel.com> --- 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(-)