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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel