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