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

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

 



On Thu, 2019-07-11 at 13:55 -0700, Shane McKee wrote:
> On Thu, Jul 11, 2019 at 11:05:15PM +0300, Ville Syrjälä wrote:
> > On Wed, Jul 10, 2019 at 03:14:59PM -0700, José Roberto de Souza
> > wrote:
> > > From: Imre Deak <imre.deak@xxxxxxxxx>
> > > 
> > > 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.
> > > 
> > > v2: Handle shared HPD pins (Imre)
> > > v3: Rebased
> > > v4: Renamed INTEL_HOTPLUG_NOCHANGE to INTEL_HOTPLUG_UNCHANGED to
> > > keep
> > > it consistent(Rodrigo)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Tested-by: Shane McKee <shane.mckee@xxxxxxxxx>

I missed this =/

I will add it while merging, thanks for testing.


> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c     | 12 ++--
> > >  drivers/gpu/drm/i915/display/intel_dp.c      | 12 ++--
> > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 59
> > > +++++++++++++++-----
> > >  drivers/gpu/drm/i915/display/intel_hotplug.h |  5 +-
> > >  drivers/gpu/drm/i915/display/intel_sdvo.c    |  8 ++-
> > >  drivers/gpu/drm/i915/i915_debugfs.c          |  2 +-
> > >  drivers/gpu/drm/i915/i915_drv.h              |  3 +-
> > >  drivers/gpu/drm/i915/intel_drv.h             | 11 +++-
> > >  8 files changed, 80 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index ad638e7f27bb..734c004800f8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -4047,14 +4047,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);
> > >  
> > > @@ -4076,7 +4078,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/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 0bdb7ecc5a81..4423abbc7907 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -4853,14 +4853,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);
> > >  
> > > @@ -4879,7 +4881,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/display/intel_hotplug.c
> > > b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > index ea3de4acc850..2ca92780c659 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > > @@ -112,6 +112,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
> > > @@ -266,8 +267,10 @@ static void
> > > intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> > >  	intel_runtime_pm_put(&dev_priv->runtime_pm, 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;
> > > @@ -279,7 +282,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_UNCHANGED;
> > >  
> > >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to
> > > %s\n",
> > >  		      connector->base.base.id,
> > > @@ -287,7 +290,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)
> > > @@ -339,7 +342,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);
> > 
> > Random thought: Should open code this with queue_delayed_work()
> > instead
> > so that we can be sure the wq really is the same between this call
> > and the mod_delayed_work()?
> > 
> > >  	}
> > >  }
> > >  
> > > @@ -349,14 +352,16 @@ 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 changed = 0, retry = 0;
> > >  	u32 hpd_event_bits;
> > > +	u32 hpd_retry_bits;
> > >  
> > >  	mutex_lock(&dev->mode_config.mutex);
> > >  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
> > > @@ -365,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);
> > > @@ -373,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_connect
> > > or);
> > > +			switch (intel_encoder->hotplug(intel_encoder,
> > > +						       intel_connector,
> > > +						       hpd_event_bits &
> > > hpd_bit)) {
> > > +			case INTEL_HOTPLUG_UNCHANGED:
> > > +				break;
> > > +			case INTEL_HOTPLUG_CHANGED:
> > > +				changed |= hpd_bit;
> > > +				break;
> > > +			case INTEL_HOTPLUG_RETRY:
> > > +				retry |= hpd_bit;
> > > +				break;
> > > +			}
> > >  		}
> > >  	}
> > >  	drm_connector_list_iter_end(&conn_iter);
> > > @@ -390,6 +410,17 @@ static void i915_hotplug_work_func(struct
> > > work_struct *work)
> > >  
> > >  	if (changed)
> > >  		drm_kms_helper_hotplug_event(dev);
> > > +
> > > +	/* Remove shared HPD pins that have changed */
> > > +	retry &= ~changed;
> > > +	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));
> > > +	}
> > >  }
> > >  
> > >  
> > > @@ -516,7 +547,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);
> > >  }
> > >  
> > >  /**
> > > @@ -636,7 +667,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,
> > > @@ -650,11 +682,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/display/intel_hotplug.h
> > > b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > index 805f897dbb7a..b0cd447b7fbc 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > > @@ -15,8 +15,9 @@ struct intel_connector;
> > >  struct intel_encoder;
> > >  
> > >  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);
> > >  void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > >  			   u32 pin_mask, u32 long_mask);
> > >  void intel_hpd_init(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > index 3fe8eaef6bd8..639317b84e51 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > @@ -1915,12 +1915,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
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 3e4f58f19362..6d4bc983915f 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -4090,7 +4090,7 @@ static int i915_hpd_storm_ctl_show(struct
> > > seq_file *m, void *data)
> > >  	 */
> > >  	intel_synchronize_irq(dev_priv);
> > >  	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 f9878cbef4d9..f471307aa704 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -163,7 +163,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;
> > > @@ -175,6 +175,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_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 24c63ed45c6f..e58d3751ed43 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -101,14 +101,21 @@ struct intel_fbdev {
> > >  	struct mutex hpd_lock;
> > >  };
> > >  
> > > +enum intel_hotplug_state {
> > > +	INTEL_HOTPLUG_UNCHANGED,
> > > +	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 *);
> > > -- 
> > > 2.22.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux