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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 09, 2020 at 11:58:18AM -0400, Lyude Paul wrote:
> Hi! Awesome patch series!
> 
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>

Thanks.

> Also re merging via drm-intel-next-queued - I think that should be fine, fwiw
> merging via drm-misc-next might be another option (I've definitely done this in
> the past for series that touched MST and drivers, but I don't have a hard
> preference either way).

Ok, if no objections I'll merge 2/3 via drm-misc-next, that seems to
make more sense.

Could you also take a look at
https://patchwork.freedesktop.org/series/78100/

I should've CC'd you.

> On Tue, 2020-06-09 at 15:15 +0300, Imre Deak wrote:
> > Hi Dave, Lyude,
> > 
> > are you ok to merge this patchset via the drm-intel-next-queued tree?
> > 
> > --Imre
> > 
> > On Thu, Jun 04, 2020 at 09:45:00PM +0300, Imre Deak wrote:
> > > Some TypeC -> native DP adapters, at least the Club 3D 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 Standard 2.0 section 5.1.4:
> > > - DP sinks should generate short pulses in the 500 usec -> 1 msec range
> > > - DP sources should detect short pulses in the 250 usec -> 2 msec range
> > > 
> > > According to the DP Alt Mode on TypeC Standard section 3.9.2, adapters
> > > should detect and forward short pulses according to how sources should
> > > detect them as specified in the DP Standard (250 usec -> 2 msec).
> > > 
> > > Based on the above 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 msec if the source doesn't respond, so
> > > use a 50 msec poll period to avoid missing an interrupt. Polling of the
> > > MST interrupt flags is explicitly allowed by the DP Standard.
> > > 
> > > 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.
> > > v3 (Ville):
> > > - Fix the short pulse duration limits in the commit log prescribed by the
> > >   DP Standard.
> > > - Add code comment explaining why/how polling is used.
> > > - Factor out a helper to schedule the port's hpd irq handler and move it
> > >   to the rest of hotplug handlers.
> > > - Document the new MST callback.
> > > - s/update_hpd_irq_state/poll_hpd_irq/
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c        | 32 ++++++++++++++++++--
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 10 ++++++
> > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 18 +++++++++++
> > >  drivers/gpu/drm/i915/display/intel_hotplug.h |  2 ++
> > >  include/drm/drm_dp_mst_helper.h              |  9 ++++++
> > >  5 files changed, 68 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..2a309fb2c4cc 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1178,11 +1178,37 @@ 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 (;;) {
> > > +		/*
> > > +		 * If the driver provides a way for this, change to
> > > +		 * poll-waiting for the MST reply interrupt if we didn't receive
> > > +		 * it for 50 msec. This would cater for cases where the HPD
> > > +		 * pulse signal got lost somewhere, even though the sink raised
> > > +		 * the corresponding MST interrupt correctly. One example is the
> > > +		 * Club 3D CAC-1557 TypeC -> DP adapter which for some reason
> > > +		 * filters out short pulses with a duration less than ~540 usec.
> > > +		 *
> > > +		 * The poll period is 50 msec to avoid missing an interrupt
> > > +		 * after the sink has cleared it (after a 110msec timeout
> > > +		 * since it raised the interrupt).
> > > +		 */
> > > +		ret = wait_event_timeout(mgr->tx_waitq,
> > > +					 check_txmsg_state(mgr, txmsg),
> > > +					 mgr->cbs->poll_hpd_irq ?
> > > +						msecs_to_jiffies(50) :
> > > +						wait_timeout);
> > > +
> > > +		if (ret || !mgr->cbs->poll_hpd_irq ||
> > > +		    time_after(jiffies, wait_expires))
> > > +			break;
> > > +
> > > +		mgr->cbs->poll_hpd_irq(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..9be52643205d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -33,6 +33,7 @@
> > >  #include "intel_connector.h"
> > >  #include "intel_ddi.h"
> > >  #include "intel_display_types.h"
> > > +#include "intel_hotplug.h"
> > >  #include "intel_dp.h"
> > >  #include "intel_dp_mst.h"
> > >  #include "intel_dpio_phy.h"
> > > @@ -765,8 +766,17 @@ static struct drm_connector
> > > *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> > >  	return NULL;
> > >  }
> > >  
> > > +static void
> > > +intel_dp_mst_poll_hpd_irq(struct drm_dp_mst_topology_mgr *mgr)
> > > +{
> > > +	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > > +
> > > +	intel_hpd_trigger_irq(dp_to_dig_port(intel_dp));
> > > +}
> > > +
> > >  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> > >  	.add_connector = intel_dp_add_mst_connector,
> > > +	.poll_hpd_irq = intel_dp_mst_poll_hpd_irq,
> > >  };
> > >  
> > >  static struct intel_dp_mst_encoder *
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > index 4f6f560e093e..664f88354101 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > @@ -347,6 +347,24 @@ static void i915_digport_work_func(struct work_struct
> > > *work)
> > >  	}
> > >  }
> > >  
> > > +/**
> > > + * intel_hpd_trigger_irq - trigger an hpd irq event for a port
> > > + * @dig_port: digital port
> > > + *
> > > + * Trigger an HPD interrupt event for the given port, emulating a short
> > > pulse
> > > + * generated by the sink, and schedule the dig port work to handle it.
> > > + */
> > > +void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
> > > +{
> > > +	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);
> > > +}
> > > +
> > >  /*
> > >   * Handle hotplug events outside the interrupt handler proper.
> > >   */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > index 777b0743257e..a704d7c94d16 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > @@ -10,6 +10,7 @@
> > >  
> > >  struct drm_i915_private;
> > >  struct intel_connector;
> > > +struct intel_digital_port;
> > >  struct intel_encoder;
> > >  enum port;
> > >  
> > > @@ -18,6 +19,7 @@ enum intel_hotplug_state intel_encoder_hotplug(struct
> > > intel_encoder *encoder,
> > >  					       struct intel_connector
> > > *connector);
> > >  void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > >  			   u32 pin_mask, u32 long_mask);
> > > +void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
> > >  void intel_hpd_init(struct drm_i915_private *dev_priv);
> > >  void intel_hpd_init_work(struct drm_i915_private *dev_priv);
> > >  void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index 9e1ffcd7cb68..b230ff6f7081 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -475,6 +475,15 @@ 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);
> > > +	/*
> > > +	 * Checks for any pending MST interrupts, passing them to MST core for
> > > +	 * processing, the same way an HPD IRQ pulse handler would do this.
> > > +	 * If provided MST core calls this callback from a poll-waiting loop
> > > +	 * when waiting for MST down message replies. The driver is expected
> > > +	 * to guard against a race between this callback and the driver's HPD
> > > +	 * IRQ pulse handler.
> > > +	 */
> > > +	void (*poll_hpd_irq)(struct drm_dp_mst_topology_mgr *mgr);
> > >  };
> > >  
> > >  #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
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux