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.d...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
