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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
