[Intel-gfx] [PATCH v2 3/3] drm/i915/dp_mst: Work around out-of-spec adapters filtering short pulses

[Imre Deak]

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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel