[Intel-gfx] [PATCH 1/2] drm/i915: Add support for retrying hotplug

[Imre Deak]

On Sat, Mar 16, 2019 at 12:22:30AM +0200, Souza, Jose wrote:
> [...]
> > > > > @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct
> > > > > work_struct *work)
> > > > >  
> > > > >  	if (changed)
> > > > >  		drm_kms_helper_hotplug_event(dev);
> > > > 
> > > > Just realized, that this way we'll do hotplug processing for
> > > > pins with a shared encoder (DDI DP/HDMI) twice, even if one
> > > > encoder's hotplug hook signaled a change (and so there can't be
> > > > any change on other connectors sharing this encoder). I suppose
> > > > preventing calling the hook for a pin that already signalled a
> > > > change would make sense, but that's a separate issue and could
> > > > still lead to retrying needlessly. So I think we should prevent
> > > > the retry here explicitly:
> > > > 
> > > > 	retry &= ~changed;
> > > 
> > > I was not aware of this shared hotplug pin, but it will need more
> > > than that, if the first port sharing a pin have changed but the
> > > second don't it would still mark the pin to retry,
> > 
> > For two connectors sharing an encoder their ports and pins will be
> > the same. So if you do above:
> > 
> > 		case INTEL_HOTPLUG_CHANGED:
> > 			changed |= hpd_bit;
> > 			break;
> > 
> > 		...
> > 
> > you can just avoid the retries for such changed connectors by
> > 
> > 	retry &= ~changed;
> > 	
> > here.	
> 
> So lets say that we have: HDMI-1 on enconder_A and DP-1 on encoder_A,
> user hotplug HDMI-1 and when iterating over all connectors in
> i915_hotplug_work_func() we have this:
> 
> // fist iteration
> ret = intel_encoder->hotplug->hotplug(encoder, HDMI-1);
> //ret = INTEL_HOTPLUG_CHANGED
> 
> // second iteration
> ret = intel_encoder->hotplug->hotplug(encoder, DP-1);
> //ret = INTEL_HOTPLUG_RETRY
> 
> so a 'retry &= ~changed;' in 'case INTEL_HOTPLUG_CHANGED:' alone would
> not prevent it to set retry_bits;

The bits from retry are cleared after the loop which does prevent it.
Here's the updated diff for the function:

@@ -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 changed = 0;
+	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 |= hpd_bit;
+				break;
+			case INTEL_HOTPLUG_RETRY:
+				retry |= hpd_bit;
+				break;
+			}
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
@@ -389,6 +410,17 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
+
+	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));
+	}
 }
 
 
--Imre