Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug

[Imre Deak <imre.deak@intel.com>]

On Mon, Feb 25, 2019 at 01:02:45PM +0200, Jani Nikula wrote:
> On Fri, 22 Feb 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> > From: Imre Deak <imre.deak@intel.com>
> >
> > There is some scenarios that we are aware that sink probe can fail,
> > so lets add the infrastructure to let hotplug() hook to request
> > another probe after some time.
> 
> See also
> 
> http://patchwork.freedesktop.org/patch/msgid/20180925071836.24711-1-jani.nikula@intel.com

Yes, the idea for a generic way to retry the hotplug detection came
after discussing with Ville and Jani about the similar issue with HDMI
sinks. The only difference wrt. Jani's patch above is that here we only
delay the detection if the connector probe thinks it's necessary. This
then also allows for a longer (up to 1sec) delay needed by the buggy
TypeC dongles.

> 
> BR,
> Jani.
> 
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
> >  drivers/gpu/drm/i915/intel_ddi.c     | 12 +++---
> >  drivers/gpu/drm/i915/intel_dp.c      | 12 +++---
> >  drivers/gpu/drm/i915/intel_drv.h     | 17 +++++++--
> >  drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_sdvo.c    |  8 ++--
> >  7 files changed, 79 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 37175414ce89..8149be62a630 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
> >  	 */
> >  	synchronize_irq(dev_priv->drm.irq);
> >  	flush_work(&dev_priv->hotplug.dig_port_work);
> > -	flush_work(&dev_priv->hotplug.hotplug_work);
> > +	flush_delayed_work(&dev_priv->hotplug.hotplug_work);
> >  
> >  	seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
> >  	seq_printf(m, "Detected: %s\n",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bb0e75e43987..5703526bddab 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -156,7 +156,7 @@ enum hpd_pin {
> >  #define HPD_STORM_DEFAULT_THRESHOLD 50
> >  
> >  struct i915_hotplug {
> > -	struct work_struct hotplug_work;
> > +	struct delayed_work hotplug_work;
> >  
> >  	struct {
> >  		unsigned long last_jiffies;
> > @@ -168,6 +168,7 @@ struct i915_hotplug {
> >  		} state;
> >  	} stats[HPD_NUM_PINS];
> >  	u32 event_bits;
> > +	u32 retry_bits;
> >  	struct delayed_work reenable_work;
> >  
> >  	u32 long_port_mask;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ea83071a22c4..1676a87f18cb 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
> >  	return modeset_pipe(&crtc->base, ctx);
> >  }
> >  
> > -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> > -			      struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_ddi_hotplug(struct intel_encoder *encoder,
> > +		  struct intel_connector *connector,
> > +		  bool irq_received)
> >  {
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	bool changed;
> > +	enum intel_hotplug_state state;
> >  	int ret;
> >  
> > -	changed = intel_encoder_hotplug(encoder, connector);
> > +	state = intel_encoder_hotplug(encoder, connector, irq_received);
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> >  
> > @@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> >  	drm_modeset_acquire_fini(&ctx);
> >  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >  
> > -	return changed;
> > +	return state;
> >  }
> >  
> >  static struct intel_connector *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e1a051c0fbfe..0bb43eaa154c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
> >   * retrain the link to get a picture. That's in case no
> >   * userspace component reacted to intermittent HPD dip.
> >   */
> > -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> > -			     struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_dp_hotplug(struct intel_encoder *encoder,
> > +		 struct intel_connector *connector,
> > +		 bool irq_received)
> >  {
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	bool changed;
> > +	enum intel_hotplug_state state;
> >  	int ret;
> >  
> > -	changed = intel_encoder_hotplug(encoder, connector);
> > +	state = intel_encoder_hotplug(encoder, connector, irq_received);
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> >  
> > @@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
> >  	drm_modeset_acquire_fini(&ctx);
> >  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >  
> > -	return changed;
> > +	return state;
> >  }
> >  
> >  static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 81ec73e4a083..de62af970dc1 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -226,14 +226,21 @@ struct intel_fbdev {
> >  	struct mutex hpd_lock;
> >  };
> >  
> > +enum intel_hotplug_state {
> > +	INTEL_HOTPLUG_NOCHANGE,
> > +	INTEL_HOTPLUG_CHANGED,
> > +	INTEL_HOTPLUG_RETRY,
> > +};
> > +
> >  struct intel_encoder {
> >  	struct drm_encoder base;
> >  
> >  	enum intel_output_type type;
> >  	enum port port;
> >  	unsigned int cloneable;
> > -	bool (*hotplug)(struct intel_encoder *encoder,
> > -			struct intel_connector *connector);
> > +	enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
> > +					    struct intel_connector *connector,
> > +					    bool irq_received);
> >  	enum intel_output_type (*compute_output_type)(struct intel_encoder *,
> >  						      struct intel_crtc_state *,
> >  						      struct drm_connector_state *);
> > @@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
> >  void intel_dvo_init(struct drm_i915_private *dev_priv);
> >  /* intel_hotplug.c */
> >  void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > -			   struct intel_connector *connector);
> > +enum intel_hotplug_state
> > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > +		      struct intel_connector *connector,
> > +		      bool irq_received);
> >  
> >  /* legacy fbdev emulation in intel_fbdev.c */
> >  #ifdef CONFIG_DRM_FBDEV_EMULATION
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> > index b8937c788f03..3eaa06526f6b 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
> >  
> >  #define HPD_STORM_DETECT_PERIOD		1000
> >  #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
> > +#define HPD_RETRY_DELAY			1000
> >  
> >  /**
> >   * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
> > @@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> >  	intel_runtime_pm_put(dev_priv, wakeref);
> >  }
> >  
> > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > -			   struct intel_connector *connector)
> > +enum intel_hotplug_state
> > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > +		      struct intel_connector *connector,
> > +		      bool irq_received)
> >  {
> >  	struct drm_device *dev = connector->base.dev;
> >  	enum drm_connector_status old_status;
> > @@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> >  		drm_helper_probe_detect(&connector->base, NULL, false);
> >  
> >  	if (old_status == connector->base.status)
> > -		return false;
> > +		return INTEL_HOTPLUG_NOCHANGE;
> >  
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
> >  		      connector->base.base.id,
> > @@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> >  		      drm_get_connector_status_name(old_status),
> >  		      drm_get_connector_status_name(connector->base.status));
> >  
> > -	return true;
> > +	return INTEL_HOTPLUG_CHANGED;
> >  }
> >  
> >  static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
> > @@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
> >  		spin_lock_irq(&dev_priv->irq_lock);
> >  		dev_priv->hotplug.event_bits |= old_bits;
> >  		spin_unlock_irq(&dev_priv->irq_lock);
> > -		schedule_work(&dev_priv->hotplug.hotplug_work);
> > +		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> >  	}
> >  }
> >  
> > @@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
> >  static void i915_hotplug_work_func(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > -		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
> > +		container_of(work, struct drm_i915_private,
> > +			     hotplug.hotplug_work.work);
> >  	struct drm_device *dev = &dev_priv->drm;
> >  	struct intel_connector *intel_connector;
> >  	struct intel_encoder *intel_encoder;
> >  	struct drm_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	bool changed = false;
> > +	u32 retry = 0;
> >  	u32 hpd_event_bits;
> > +	u32 hpd_retry_bits;
> >  
> >  	mutex_lock(&dev->mode_config.mutex);
> >  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
> > @@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >  
> >  	hpd_event_bits = dev_priv->hotplug.event_bits;
> >  	dev_priv->hotplug.event_bits = 0;
> > +	hpd_retry_bits = dev_priv->hotplug.retry_bits;
> > +	dev_priv->hotplug.retry_bits = 0;
> >  
> >  	/* Enable polling for connectors which had HPD IRQ storms */
> >  	intel_hpd_irq_storm_switch_to_polling(dev_priv);
> > @@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >  
> >  	drm_connector_list_iter_begin(dev, &conn_iter);
> >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > +		u32 hpd_bit;
> > +
> >  		intel_connector = to_intel_connector(connector);
> >  		if (!intel_connector->encoder)
> >  			continue;
> >  		intel_encoder = intel_connector->encoder;
> > -		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> > +		hpd_bit = BIT(intel_encoder->hpd_pin);
> > +		if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
> >  			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> >  				      connector->name, intel_encoder->hpd_pin);
> >  
> > -			changed |= intel_encoder->hotplug(intel_encoder,
> > -							  intel_connector);
> > +			switch (intel_encoder->hotplug(intel_encoder,
> > +						       intel_connector,
> > +						       hpd_event_bits & hpd_bit)) {
> > +			case INTEL_HOTPLUG_NOCHANGE:
> > +				break;
> > +			case INTEL_HOTPLUG_CHANGED:
> > +				changed = true;
> > +				break;
> > +			case INTEL_HOTPLUG_RETRY:
> > +				retry |= hpd_bit;
> > +				break;
> > +			}
> >  		}
> >  	}
> >  	drm_connector_list_iter_end(&conn_iter);
> > @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >  
> >  	if (changed)
> >  		drm_kms_helper_hotplug_event(dev);
> > +
> > +	if (retry) {
> > +		spin_lock_irq(&dev_priv->irq_lock);
> > +		dev_priv->hotplug.retry_bits |= retry;
> > +		spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > +		mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
> > +				 msecs_to_jiffies(HPD_RETRY_DELAY));
> > +	}
> >  }
> >  
> >  
> > @@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> >  	if (queue_dig)
> >  		queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
> >  	if (queue_hp)
> > -		schedule_work(&dev_priv->hotplug.hotplug_work);
> > +		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> >  }
> >  
> >  /**
> > @@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
> >  
> >  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> >  {
> > -	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> > +	INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> > +			  i915_hotplug_work_func);
> >  	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
> >  	INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
> >  	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
> > @@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> >  	dev_priv->hotplug.long_port_mask = 0;
> >  	dev_priv->hotplug.short_port_mask = 0;
> >  	dev_priv->hotplug.event_bits = 0;
> > +	dev_priv->hotplug.retry_bits = 0;
> >  
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  
> >  	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> > -	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> > +	cancel_delayed_work_sync(&dev_priv->hotplug.hotplug_work);
> >  	cancel_work_sync(&dev_priv->hotplug.poll_init_work);
> >  	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index e7b0884ba5a5..92793475b2b9 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
> >  			     &intel_sdvo->hotplug_active, 2);
> >  }
> >  
> > -static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
> > -			       struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_sdvo_hotplug(struct intel_encoder *encoder,
> > +		   struct intel_connector *connector,
> > +		   bool irq_received)
> >  {
> >  	intel_sdvo_enable_hotplug(encoder);
> >  
> > -	return intel_encoder_hotplug(encoder, connector);
> > +	return intel_encoder_hotplug(encoder, connector, irq_received);
> >  }
> >  
> >  static bool
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx