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

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

[Jani Nikula]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 11 Jun 2015, 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>

Oh, this should probably also be

Cc: stable@vger.kernel.org

> ---
>  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

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

[Chris Wilson]

On Thu, Jun 11, 2015 at 11:35:59AM +0300, Jani Nikula 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".)

400ms is a tag too long. 250ms would be my preference, but 400ms is what
was tested with, let's go with it until we find resume time is now >400ms
I wonder if we would want to flush the hotplug_work on init/resume.

> 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]

Easiest example is https://bugs.freedesktop.org/show_bug.cgi?id=76464.
But yes we had lots of feedback when we used the live state.

> 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>

Looks familiar, https://bugs.freedesktop.org/show_bug.cgi?id=82551
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Hugh Greenberg]

What if you fired the uevent regardless of the previous state?

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

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

[Jani Nikula]

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

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

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6568
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  312/312              312/312
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Thu, Jun 11, 2015 at 10:13:26AM -0600, Hugh Greenberg wrote:
> What if you fired the uevent regardless of the previous state?

Userspace will do a full probe (maybe several thanks to all the
different listeners) on all connectors following a hotplug uevent. If we
can avoid that that is one less headache.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 11 Jun 2015, 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>

Daniel wants a fancier solution: queing a one-shot re-check after a
while in the cases that the connector state appears to not have
changed. It'll take a while until I get to it. But this patch is not
going anywhere.

BR,
Jani.



> ---
>  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

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

[Daniel Vetter]

On Mon, Jun 15, 2015 at 04:32:24PM +0300, Jani Nikula wrote:
> On Thu, 11 Jun 2015, 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>
> 
> Daniel wants a fancier solution: queing a one-shot re-check after a
> while in the cases that the connector state appears to not have
> changed. It'll take a while until I get to it. But this patch is not
> going anywhere.

For a bit of context with an abitrary delay we have a bad tuning problem
of people complaining that the latency after a hotplug is too noticeable
while other folks complain that it's still to fast. And we have all the
infrastructure and tracking already to know which hpd pins have fired - we
already only re-probe connectors where we expect a change.

Hence adding an additional reprobe when there's no change when we expect
one isn't invasive, doesn't result in needless reprobing and the delay can
be picked such that even the slowest user won't be able to botch things up
any more ;-)

Also adding a reprobe would allow us to resurrect an opportunistic check
for the hpd pins (instead of the hard one we've had earlier and had to
back out again from hdmi due to crappy hw). Which is something that's been
requested piles of times (together with caching edids and stuff) and hence
fits into the overall plan better.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx