On Thu, 11 Jun 2015, Hugh Greenberg <hugegreenbug@xxxxxxxxx> wrote: > What if you fired the uevent regardless of the previous state? Not sending the uevent is actually a side effect of this bug, not the main problem. I think we need something like this no matter what. Unconditionally sending the uevent is a separate question. BR, Jani. > >> On Jun 11, 2015, at 2:35 AM, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: >> >> Currently it's possible this happens when a (non-DP) cable is unplugged: >> >> - user starts unplugging cable >> - hotplug irq fires >> - hotplug work function runs >> - connector detect function runs >> - ddc pin is still connected, edid read succeeds >> -> we decide nothing changed, no uevent >> - cable completely unplugged >> -> our state is inconsistent with reality, no power save >> >> The user plugs cable back in: >> >> - most of the same at first >> - ddc pin connected, edid read succeeds >> -> we decide nothing changed because we thought the cable was plugged >> in all along, no uevent >> - cable completely plugged >> -> our state is somewhat consistent, however monitor might be >> different, the link might not recover? >> >> With our current implementation we rely on the hotplug pin being *both* >> the last to be connected on plug *and* last to be disconnected on >> unplug. The educated guess is that this is not the case. >> >> Per the logs in the case of the referenced bug, the hdmi detect code >> runs within a few *microseconds* after the hotplug irq, and the EDID has >> been successfully read within 25 ms of the irq. If the DDC lines are >> still connected when the hotplug irq fires, the user has to be blazingly >> fast to complete the unplug before the detect code runs. >> >> We can afford to wait a little before queuing the work function for a >> bit more reliability. Obviously it's still possible for the user to >> unplug the cable really slowly, but let's at least give the user a >> fighting chance to unplug fast enough. >> >> I'd love to claim the proposed delay of 400 ms is based on real life >> measured data and rigorous analysis, but in truth it is just a gut >> feeling based compromise between solving the issue and meeting some >> vague real time human interaction deadline for having a picture on >> screen. (However I expect any criticism to be more substantiated than >> "my gut feeling is better than yours".) >> >> An alternative would be to check the hotplug state in the irq handler, >> but there have been reliability problems with it in the past, and we've >> opted not to use it. [citation needed] >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82593 >> Tested-by: Hugh Greenberg <hugegreenbug@xxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 2 +- >> drivers/gpu/drm/i915/i915_drv.h | 2 +- >> drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++---- >> 3 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 78ef0bb53c36..002767ddfe06 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -552,7 +552,7 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) >> 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_delayed_work_sync(&dev_priv->hotplug.reenable_work); >> } >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 611fbd86c1cc..a8a99f658772 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -221,7 +221,7 @@ enum hpd_pin { >> for ((__pin) = (HPD_NONE + 1); (__pin) < HPD_NUM_PINS; (__pin)++) >> >> struct i915_hotplug { >> - struct work_struct hotplug_work; >> + struct delayed_work hotplug_work; >> >> struct { >> unsigned long last_jiffies; >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index 56db9e747464..bab67c279c22 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -828,6 +828,8 @@ static bool intel_hpd_irq_event(struct drm_device *dev, >> return true; >> } >> >> +#define HOTPLUG_DELAY_MS 400 >> + >> static void i915_digport_work_func(struct work_struct *work) >> { >> struct drm_i915_private *dev_priv = >> @@ -872,7 +874,8 @@ 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); >> + mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work, >> + msecs_to_jiffies(HOTPLUG_DELAY_MS)); >> } >> } >> >> @@ -884,7 +887,7 @@ 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->dev; >> struct drm_mode_config *mode_config = &dev->mode_config; >> struct intel_connector *intel_connector; >> @@ -1601,7 +1604,8 @@ static void intel_hpd_irq_handler(struct drm_device *dev, >> 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); >> + mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work, >> + msecs_to_jiffies(HOTPLUG_DELAY_MS)); >> } >> >> static void gmbus_irq_handler(struct drm_device *dev) >> @@ -4397,7 +4401,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) >> { >> struct drm_device *dev = dev_priv->dev; >> >> - 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->rps.work, gen6_pm_rps_work); >> INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work); >> -- >> 2.1.4 >> -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
