Re: [PATCH] drm/i915/irq: wait a little before queuing the hotplug work function

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 11 Jun 2015, Hugh Greenberg <hugegreenbug@gmail.com> 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@intel.com> 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@gmail.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx