[PATCH 0/4] Fix numerous issues with HPDstorm handling

[PATCH 0/4] Fix numerous issues with HPDstorm handling

[Egbert Eich]

The following 4 patches fix issues with the HPD storm detection.
Two of them have been introduced quite recently, one has been
around since this code was implemented.
Work arounds like:
commit 3ff04a160a891e56cdcee5c198d4c764d1c8c78b
("drm/i915: Don't WARN nor handle unexpected hpd interrupts on gmch platforms")
and
commit 3432087ef846d760427eceff0ff4e7d0a2565b8a
("drm/i915: Only WARN about a stuck hotplug irq ONCE")
may actually become obsolete now.

Egbert Eich (4):
  drm: Add a non-locking version of drm_kms_helper_poll_enable().
  drm/i915: Call non-locking version of drm_kms_helper_poll_enable()
  drm/i915: Use the correct hpd_status list for non-G4xx/VLV
  drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

 drivers/gpu/drm/drm_probe_helper.c   | 19 ++++++++++++++++---
 drivers/gpu/drm/i915/i915_irq.c      |  2 +-
 drivers/gpu/drm/i915/intel_crt.c     | 17 ++++++++++++-----
 drivers/gpu/drm/i915/intel_hotplug.c |  2 +-
 include/drm/drm_crtc_helper.h        |  1 +
 5 files changed, 31 insertions(+), 10 deletions(-)

-- 
1.8.4.5

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

[PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

drm_kms_helper_poll_enable() was converted to lock the mode_config
mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").

This disregarded the cases where this function is called from a context
where this mutex is already locked.

Add a non-locking version as well.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++---
 include/drm/drm_crtc_helper.h      |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index d734780..a93ad1b 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 	return 1;
 }
 
+/**
+ * drm_kms_helper_poll_enable_no_lock - re-enable output polling.
+ * @dev: drm_device
+ *
+ * This function re-enables the output polling work without
+ * locking the mode_config mutex.
+ *
+ * This is like drm_kms_helper_poll_enable() however it is to be
+ * called from a context where the mode_config mutex is locked
+ * already.
+ */
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
-static void __drm_kms_helper_poll_enable(struct drm_device *dev)
+void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev)
 {
 	bool poll = false;
 	struct drm_connector *connector;
@@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev)
 	if (poll)
 		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
 }
+EXPORT_SYMBOL(drm_kms_helper_poll_enable_no_lock);
+
 
 static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector,
 							      uint32_t maxX, uint32_t maxY, bool merge_type_bits)
@@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 
 	/* Re-enable polling in case the global poll config changed. */
 	if (drm_kms_helper_poll != dev->mode_config.poll_running)
-		__drm_kms_helper_poll_enable(dev);
+		drm_kms_helper_poll_enable_no_lock(dev);
 
 	dev->mode_config.poll_running = drm_kms_helper_poll;
 
@@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
 void drm_kms_helper_poll_enable(struct drm_device *dev)
 {
 	mutex_lock(&dev->mode_config.mutex);
-	__drm_kms_helper_poll_enable(dev);
+	drm_kms_helper_poll_enable_no_lock(dev);
 	mutex_unlock(&dev->mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_enable);
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 2a747a9..f96703d 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
 
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
+extern void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev);
 
 #endif
-- 
1.8.4.5

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

[PATCH 2/4] drm/i915: Call non-locking version of drm_kms_helper_poll_enable()

[Egbert Eich]

drm_kms_helper_poll_enable() is called from a context in
intel_hpd_irq_storm_disable() where the the mode_config mutex is
already locked.
When this function was converted to lock this mutex in:

commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 9 23:44:26 2015 +0200

    drm/probe-helper: Grab mode_config.mutex in poll_init/enable

a deadlock occurred.
Call the newly implemented non-locking version of this function.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/intel_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 53c0173..77dd5b6 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -180,7 +180,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 
 	/* Enable polling and queue hotplug re-enabling. */
 	if (hpd_disabled) {
-		drm_kms_helper_poll_enable(dev);
+		drm_kms_helper_poll_enable_no_lock(dev);
 		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
 				 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
 	}
-- 
1.8.4.5

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

[PATCH 3/4] drm/i915: Use the correct hpd_status list for non-G4xx/VLV

[Egbert Eich]

This copy-and-past error was introduced in:

commit fd63e2a972c670887e5e8a08440111d3812c0996
Author: Imre Deak <imre.deak@intel.com>
Date:   Tue Jul 21 15:32:44 2015 -0700

    drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8485bea..ad99dae 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1559,7 +1559,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
 		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
 		intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
-				   hotplug_trigger, hpd_status_g4x,
+				   hotplug_trigger, hpd_status_i915,
 				   i9xx_port_hotplug_long_detect);
 		intel_hpd_irq_handler(dev, pin_mask, long_mask);
 	}
-- 
1.8.4.5

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

[PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
when HPD interrupt storms occur.
Since the interrupt handler changes the enabled interrupt lines when it
detects a storm this races with intel_crt_detect_hotplug().
To avoid this, shiled the rmw cycles with IRQ save spinlocks.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/intel_crt.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index af5e43b..685f3de 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -376,9 +376,10 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 hotplug_en, orig, stat;
+	u32 hotplug_en, stat;
 	bool ret = false;
 	int i, tries = 0;
+	unsigned long irqflags;
 
 	if (HAS_PCH_SPLIT(dev))
 		return intel_ironlake_crt_detect_hotplug(connector);
@@ -395,12 +396,14 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 		tries = 2;
 	else
 		tries = 1;
-	hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
-	hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
 
 	for (i = 0; i < tries ; i++) {
 		/* turn on the FORCE_DETECT */
-		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+		hotplug_en = I915_READ(PORT_HOTPLUG_EN);
+		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en |
+			   CRT_HOTPLUG_FORCE_DETECT);
+		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 		/* wait for FORCE_DETECT to go off */
 		if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
 			      CRT_HOTPLUG_FORCE_DETECT) == 0,
@@ -416,7 +419,11 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 	I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
 
 	/* and put the bits back */
-	I915_WRITE(PORT_HOTPLUG_EN, orig);
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	hotplug_en = I915_READ(PORT_HOTPLUG_EN);
+	hotplug_en &= ~CRT_HOTPLUG_FORCE_DETECT;
+	I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return ret;
 }
-- 
1.8.4.5

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

Re: [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Lukas Wunner]

Hi Egbert,

On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
> drm_kms_helper_poll_enable() was converted to lock the mode_config
> mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
> 
> This disregarded the cases where this function is called from a context
> where this mutex is already locked.

Which ones would that be?


> + * drm_kms_helper_poll_enable_no_lock - re-enable output polling.

It seems DRM convention is to append _locked or _unlocked, e.g.:
drm_fb_helper_restore_fbdev_mode_unlocked
drm_gem_object_unreference_unlocked


Best regards,

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

Re: [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

Lukas Wunner writes:
 > Hi Egbert,
 > 
 > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
 > > drm_kms_helper_poll_enable() was converted to lock the mode_config
 > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
 > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
 > > 
 > > This disregarded the cases where this function is called from a context
 > > where this mutex is already locked.
 > 
 > Which ones would that be?

Have you missed the next patch in the series?

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

Re: [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Lukas Wunner]

Hi Egbert,

On Wed, Sep 02, 2015 at 12:10:19AM +0200, Egbert Eich wrote:
> Lukas Wunner writes:
>  > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
>  > > drm_kms_helper_poll_enable() was converted to lock the mode_config
>  > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
>  > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
>  > > 
>  > > This disregarded the cases where this function is called from a context
>  > > where this mutex is already locked.
>  > 
>  > Which ones would that be?
> 
> Have you missed the next patch in the series?

Sorry, I should have asked more precisely: Is this the only one?
If so it may make sense to modify i915_hotplug_work_func or
intel_hpd_irq_storm_disable instead.

Best regards,

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

Re: [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

Lukas Wunner writes:
 > 
 > It seems DRM convention is to append _locked or _unlocked, e.g.:
 > drm_fb_helper_restore_fbdev_mode_unlocked
 > drm_gem_object_unreference_unlocked
 > 

Oh, I missed that. 
Did you check what these functions actually do - and compare it to
what I try to achieve?

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

Re: [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

Lukas Wunner writes:
 > Hi Egbert,
 > 
 > On Wed, Sep 02, 2015 at 12:10:19AM +0200, Egbert Eich wrote:
 > > Lukas Wunner writes:
 > >  > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
 > >  > > drm_kms_helper_poll_enable() was converted to lock the mode_config
 > >  > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
 > >  > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
 > >  > > 
 > >  > > This disregarded the cases where this function is called from a context
 > >  > > where this mutex is already locked.
 > >  > 
 > >  > Which ones would that be?
 > > 
 > > Have you missed the next patch in the series?
 > 
 > Sorry, I should have asked more precisely: Is this the only one?
 > If so it may make sense to modify i915_hotplug_work_func or
 > intel_hpd_irq_storm_disable instead.
 > 

The non-locking version existed before. I've just exported it so it cah
be used by drivers.
Moreover this is a helper function - why should it be restricted to 
callers which don't have the mode_config mutex grabbed?
I could be talked into changing the log message.

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

Re: [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Daniel Vetter]

On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
> drm_kms_helper_poll_enable() was converted to lock the mode_config
> mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
> 
> This disregarded the cases where this function is called from a context
> where this mutex is already locked.
> 
> Add a non-locking version as well.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++---
>  include/drm/drm_crtc_helper.h      |  1 +
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index d734780..a93ad1b 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  	return 1;
>  }
>  
> +/**
> + * drm_kms_helper_poll_enable_no_lock - re-enable output polling.
> + * @dev: drm_device
> + *
> + * This function re-enables the output polling work without
> + * locking the mode_config mutex.
> + *
> + * This is like drm_kms_helper_poll_enable() however it is to be
> + * called from a context where the mode_config mutex is locked
> + * already.
> + */
>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> -static void __drm_kms_helper_poll_enable(struct drm_device *dev)
> +void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev)

Please use _locked to stay consistent with other such functions. With that
address this lgtm.
-Daniel

>  {
>  	bool poll = false;
>  	struct drm_connector *connector;
> @@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev)
>  	if (poll)
>  		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
>  }
> +EXPORT_SYMBOL(drm_kms_helper_poll_enable_no_lock);
> +
>  
>  static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector,
>  							      uint32_t maxX, uint32_t maxY, bool merge_type_bits)
> @@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>  
>  	/* Re-enable polling in case the global poll config changed. */
>  	if (drm_kms_helper_poll != dev->mode_config.poll_running)
> -		__drm_kms_helper_poll_enable(dev);
> +		drm_kms_helper_poll_enable_no_lock(dev);
>  
>  	dev->mode_config.poll_running = drm_kms_helper_poll;
>  
> @@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>  void drm_kms_helper_poll_enable(struct drm_device *dev)
>  {
>  	mutex_lock(&dev->mode_config.mutex);
> -	__drm_kms_helper_poll_enable(dev);
> +	drm_kms_helper_poll_enable_no_lock(dev);
>  	mutex_unlock(&dev->mode_config.mutex);
>  }
>  EXPORT_SYMBOL(drm_kms_helper_poll_enable);
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index 2a747a9..f96703d 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
>  
>  extern void drm_kms_helper_poll_disable(struct drm_device *dev);
>  extern void drm_kms_helper_poll_enable(struct drm_device *dev);
> +extern void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev);
>  
>  #endif
> -- 
> 1.8.4.5
> 

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

Re: [PATCH 2/4] drm/i915: Call non-locking version of drm_kms_helper_poll_enable()

[Daniel Vetter]

On Tue, Sep 01, 2015 at 10:21:33PM +0200, Egbert Eich wrote:
> drm_kms_helper_poll_enable() is called from a context in
> intel_hpd_irq_storm_disable() where the the mode_config mutex is
> already locked.
> When this function was converted to lock this mutex in:
> 
> commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jul 9 23:44:26 2015 +0200
> 
>     drm/probe-helper: Grab mode_config.mutex in poll_init/enable
> 
> a deadlock occurred.
> Call the newly implemented non-locking version of this function.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

Oops. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> if you do the
s/_no_lock/_locked/ for consistency on both patch 1&2.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index 53c0173..77dd5b6 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -180,7 +180,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
>  
>  	/* Enable polling and queue hotplug re-enabling. */
>  	if (hpd_disabled) {
> -		drm_kms_helper_poll_enable(dev);
> +		drm_kms_helper_poll_enable_no_lock(dev);
>  		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
>  				 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
>  	}
> -- 
> 1.8.4.5
> 

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

Re: [PATCH 3/4] drm/i915: Use the correct hpd_status list for non-G4xx/VLV

[Daniel Vetter]

On Tue, Sep 01, 2015 at 10:21:34PM +0200, Egbert Eich wrote:
> This copy-and-past error was introduced in:
> 
> commit fd63e2a972c670887e5e8a08440111d3812c0996
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Tue Jul 21 15:32:44 2015 -0700
> 
>     drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

I recommend to cc: the authors/reviewers of the patch which broke things
so they have a better chance to spot your patch and make amends by quickly
reviewing your fix. Anyway Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jani, this all seems to be for you.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8485bea..ad99dae 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1559,7 +1559,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>  		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>  
>  		intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
> -				   hotplug_trigger, hpd_status_g4x,
> +				   hotplug_trigger, hpd_status_i915,
>  				   i9xx_port_hotplug_long_detect);
>  		intel_hpd_irq_handler(dev, pin_mask, long_mask);
>  	}
> -- 
> 1.8.4.5
> 

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

Re: [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Daniel Vetter]

On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
> A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
> when HPD interrupt storms occur.
> Since the interrupt handler changes the enabled interrupt lines when it
> detects a storm this races with intel_crt_detect_hotplug().
> To avoid this, shiled the rmw cycles with IRQ save spinlocks.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

I think this only reduces one source of such races, but fundamentally we
can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
right when the strom code frobs around. Plugging the race with this known
interrupt source here doesn't fix the fundamental issue.

Also I think the actual interrupt deliver is fairly asynchronous, both in
the hardware and in the sw handling. E.g. spin_lock_irq does not
synchronize with the interrupt handler on SMP systems, it only guarantees
that it's not running concurrently on the same cpu (which would deadlock).

I think fundamentally this race is unfixable.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index af5e43b..685f3de 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -376,9 +376,10 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 hotplug_en, orig, stat;
> +	u32 hotplug_en, stat;
>  	bool ret = false;
>  	int i, tries = 0;
> +	unsigned long irqflags;
>  
>  	if (HAS_PCH_SPLIT(dev))
>  		return intel_ironlake_crt_detect_hotplug(connector);
> @@ -395,12 +396,14 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  		tries = 2;
>  	else
>  		tries = 1;
> -	hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
> -	hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
>  
>  	for (i = 0; i < tries ; i++) {
>  		/* turn on the FORCE_DETECT */
> -		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> +		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +		hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> +		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en |
> +			   CRT_HOTPLUG_FORCE_DETECT);
> +		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  		/* wait for FORCE_DETECT to go off */
>  		if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
>  			      CRT_HOTPLUG_FORCE_DETECT) == 0,
> @@ -416,7 +419,11 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  	I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
>  
>  	/* and put the bits back */
> -	I915_WRITE(PORT_HOTPLUG_EN, orig);
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> +	hotplug_en &= ~CRT_HOTPLUG_FORCE_DETECT;
> +	I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return ret;
>  }
> -- 
> 1.8.4.5
> 

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

Re: [PATCH 3/4] drm/i915: Use the correct hpd_status list for non-G4xx/VLV

[Imre Deak]

On ke, 2015-09-02 at 14:00 +0200, Daniel Vetter wrote:
> On Tue, Sep 01, 2015 at 10:21:34PM +0200, Egbert Eich wrote:
> > This copy-and-past error was introduced in:
> > 
> > commit fd63e2a972c670887e5e8a08440111d3812c0996
> > Author: Imre Deak <imre.deak@intel.com>
> > Date:   Tue Jul 21 15:32:44 2015 -0700
> > 
> >     drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins
> > 
> > Signed-off-by: Egbert Eich <eich@suse.de>
> 
> I recommend to cc: the authors/reviewers of the patch which broke things
> so they have a better chance to spot your patch and make amends by quickly
> reviewing your fix. Anyway Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks for catching this. Ville also noticed it and has sent already a
fix for it:
http://lists.freedesktop.org/archives/intel-gfx/2015-August/074676.html

--Imre

> 
> Jani, this all seems to be for you.
> -Daniel
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 8485bea..ad99dae 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1559,7 +1559,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
> >  		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
> >  
> >  		intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
> > -				   hotplug_trigger, hpd_status_g4x,
> > +				   hotplug_trigger, hpd_status_i915,
> >  				   i9xx_port_hotplug_long_detect);
> >  		intel_hpd_irq_handler(dev, pin_mask, long_mask);
> >  	}
> > -- 
> > 1.8.4.5
> > 
> 


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

Re: [PATCH 3/4] drm/i915: Use the correct hpd_status list for non-G4xx/VLV

[Jani Nikula]

On Wed, 02 Sep 2015, Imre Deak <imre.deak@intel.com> wrote:
> On ke, 2015-09-02 at 14:00 +0200, Daniel Vetter wrote:
>> On Tue, Sep 01, 2015 at 10:21:34PM +0200, Egbert Eich wrote:
>> > This copy-and-past error was introduced in:
>> > 
>> > commit fd63e2a972c670887e5e8a08440111d3812c0996
>> > Author: Imre Deak <imre.deak@intel.com>
>> > Date:   Tue Jul 21 15:32:44 2015 -0700
>> > 
>> >     drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins
>> > 
>> > Signed-off-by: Egbert Eich <eich@suse.de>
>> 
>> I recommend to cc: the authors/reviewers of the patch which broke things
>> so they have a better chance to spot your patch and make amends by quickly
>> reviewing your fix. Anyway Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Thanks for catching this. Ville also noticed it and has sent already a
> fix for it:
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/074676.html

I pushed that one as it came first.

BR,
Jani.


>
> --Imre
>
>> 
>> Jani, this all seems to be for you.
>> -Daniel
>> > ---
>> >  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index 8485bea..ad99dae 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -1559,7 +1559,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>> >  		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>> >  
>> >  		intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
>> > -				   hotplug_trigger, hpd_status_g4x,
>> > +				   hotplug_trigger, hpd_status_i915,
>> >  				   i9xx_port_hotplug_long_detect);
>> >  		intel_hpd_irq_handler(dev, pin_mask, long_mask);
>> >  	}
>> > -- 
>> > 1.8.4.5
>> > 
>> 
>
>

-- 
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 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Daniel Vetter writes:
 > On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
 > > A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
 > > when HPD interrupt storms occur.
 > > Since the interrupt handler changes the enabled interrupt lines when it
 > > detects a storm this races with intel_crt_detect_hotplug().
 > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
 > > 
 > > Signed-off-by: Egbert Eich <eich@suse.de>
 > 
 > I think this only reduces one source of such races, but fundamentally we
 > can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
 > right when the strom code frobs around. Plugging the race with this known

This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
order of 10^4 / sec.

 > interrupt source here doesn't fix the fundamental issue.
 > 
 > Also I think the actual interrupt deliver is fairly asynchronous, both in
 > the hardware and in the sw handling. E.g. spin_lock_irq does not
 > synchronize with the interrupt handler on SMP systems, it only guarantees
 > that it's not running concurrently on the same cpu (which would deadlock).
 > 
 > I think fundamentally this race is unfixable.


There is one important race we avoid with this patch: It is that
we change the PORT_HOTPLUG_EN concurrently in the interrupt handler
(thru xxx_hpd_irq_setup() and in intel_crt_detect_hotplug()).

With the test system that I have here the old version of the code
easily runs into this as the time spent inside intel_crt_detect_hotplug() 
is quite long - especially when no CRT is connected.

What happens intel_crt_detect_hotplug() reads the value of PORT_HOTPLUG_EN
at entry, then frobs around for ages, during this time several HPD interrupts
occur, the storm is detected, the bit related to the stormy line is unset
then after ages intel_crt_detect_hotplug() decides to give up and restores
the value it read on entry.

To remedy this this patch reads the value of PORT_HOTPLUG_EN whenever 
it is going to change it and adds the spin locks to make sure the 
read-modify-write cycles don't happen concurrently.

PORT_HOTPLUG_EN is only touched in two places: in xxx_hpd_irq_setup()
and in intel_crt_detect_hotplug(), the former can be called from an
interrupt handler.

Not sure why you see a problem here.

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

Re: [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Jani Nikula]

On Wed, 02 Sep 2015, Egbert Eich <eich@suse.com> wrote:
> This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
> order of 10^4 / sec.

Makes you wonder if either you have faulty hardware or we are
configuring the hardware wrong (we overlook some configuration about
some voltage/duration threshold maybe, or get irqs from a line that's
floating, or something).

BR,
Jani.


-- 
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 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Daniel Vetter]

On Wed, Sep 02, 2015 at 04:19:00PM +0200, Egbert Eich wrote:
> Daniel Vetter writes:
>  > On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
>  > > A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
>  > > when HPD interrupt storms occur.
>  > > Since the interrupt handler changes the enabled interrupt lines when it
>  > > detects a storm this races with intel_crt_detect_hotplug().
>  > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
>  > > 
>  > > Signed-off-by: Egbert Eich <eich@suse.de>
>  > 
>  > I think this only reduces one source of such races, but fundamentally we
>  > can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
>  > right when the strom code frobs around. Plugging the race with this known
> 
> This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
> order of 10^4 / sec.
> 
>  > interrupt source here doesn't fix the fundamental issue.
>  > 
>  > Also I think the actual interrupt deliver is fairly asynchronous, both in
>  > the hardware and in the sw handling. E.g. spin_lock_irq does not
>  > synchronize with the interrupt handler on SMP systems, it only guarantees
>  > that it's not running concurrently on the same cpu (which would deadlock).
>  > 
>  > I think fundamentally this race is unfixable.
> 
> 
> There is one important race we avoid with this patch: It is that
> we change the PORT_HOTPLUG_EN concurrently in the interrupt handler
> (thru xxx_hpd_irq_setup() and in intel_crt_detect_hotplug()).
> 
> With the test system that I have here the old version of the code
> easily runs into this as the time spent inside intel_crt_detect_hotplug() 
> is quite long - especially when no CRT is connected.
> 
> What happens intel_crt_detect_hotplug() reads the value of PORT_HOTPLUG_EN
> at entry, then frobs around for ages, during this time several HPD interrupts
> occur, the storm is detected, the bit related to the stormy line is unset
> then after ages intel_crt_detect_hotplug() decides to give up and restores
> the value it read on entry.
> 
> To remedy this this patch reads the value of PORT_HOTPLUG_EN whenever 
> it is going to change it and adds the spin locks to make sure the 
> read-modify-write cycles don't happen concurrently.
> 
> PORT_HOTPLUG_EN is only touched in two places: in xxx_hpd_irq_setup()
> and in intel_crt_detect_hotplug(), the former can be called from an
> interrupt handler.
> 
> Not sure why you see a problem here.

Hm I missed that this same register is also accessed by the irq handler
code, and it's not just that touching these bits can cause interrupts. So
yeah we need your patch, but it needs to be clearer in the commit message
that there's also trouble with concurrent register access to
PORT_HOTPLUG_EN.

Also I think a commen in the code why we grab that spinlock would be good.
For that extracting a small helper to manipulate the register (like we do
with other irq mask registers with functions like ilk_update_gt_irq) would
be good - then we have just one place to put that commment.
-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

Re: [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Jani Nikula writes:
 > On Wed, 02 Sep 2015, Egbert Eich <eich@suse.com> wrote:
 > > This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
 > > order of 10^4 / sec.
 > 
 > Makes you wonder if either you have faulty hardware or we are
 > configuring the hardware wrong (we overlook some configuration about
 > some voltage/duration threshold maybe, or get irqs from a line that's
 > floating, or something).

It is faulty hardware. But it is not a single machine that broke.
It is an entire series. IMHO due to bad signal routing and poor shilding
there is crosstalk on the SDVO lines signaling the plug status.
Since SDVO uses PCIe lines it is AC coupled, if I recall correctly
from reading the specs long time ago, one status is signalled by a
10MHz signal, the other by 20MHz.

At the time when I implemented this I've seen other reports from systems 
which showed similar problems under certain conditions(*) - although not 
quite as bad, therefore I thought of a general solution to get rid of 
this once and for all. If this had only been one system with this problem, 
I would just have blacklisted it.

(*) It seems that this somewhat depends on the video mode set (supports
the crosstalk theory) but I also had a report where this occurred at
certain charging levels and whether a power supply was connected or
not.

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

Re: [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Daniel Vetter writes:
 > On Wed, Sep 02, 2015 at 04:19:00PM +0200, Egbert Eich wrote:
 > 
 > Hm I missed that this same register is also accessed by the irq handler
 > code, and it's not just that touching these bits can cause interrupts. So
 > yeah we need your patch, but it needs to be clearer in the commit message
 > that there's also trouble with concurrent register access to
 > PORT_HOTPLUG_EN.
 > 
 > Also I think a commen in the code why we grab that spinlock would be good.
 > For that extracting a small helper to manipulate the register (like we do
 > with other irq mask registers with functions like ilk_update_gt_irq) would
 > be good - then we have just one place to put that commment.

OK, I will come up with a suggestion.

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

[PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

drm_kms_helper_poll_enable() was converted to lock the mode_config
mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").

This disregarded the cases where this function is called from a context
where this mutex is already locked.

Add a non-locking version as well.

Changes since v1:
- use function name suffix '_locked' for the function that
  is to be called from a locked context.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++---
 include/drm/drm_crtc_helper.h      |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index d734780..2b9ce37 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 	return 1;
 }
 
+/**
+ * drm_kms_helper_poll_enable_locked - re-enable output polling.
+ * @dev: drm_device
+ *
+ * This function re-enables the output polling work without
+ * locking the mode_config mutex.
+ *
+ * This is like drm_kms_helper_poll_enable() however it is to be
+ * called from a context where the mode_config mutex is locked
+ * already.
+ */
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
-static void __drm_kms_helper_poll_enable(struct drm_device *dev)
+void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 {
 	bool poll = false;
 	struct drm_connector *connector;
@@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev)
 	if (poll)
 		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
 }
+EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
+
 
 static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector,
 							      uint32_t maxX, uint32_t maxY, bool merge_type_bits)
@@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 
 	/* Re-enable polling in case the global poll config changed. */
 	if (drm_kms_helper_poll != dev->mode_config.poll_running)
-		__drm_kms_helper_poll_enable(dev);
+		drm_kms_helper_poll_enable_locked(dev);
 
 	dev->mode_config.poll_running = drm_kms_helper_poll;
 
@@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
 void drm_kms_helper_poll_enable(struct drm_device *dev)
 {
 	mutex_lock(&dev->mode_config.mutex);
-	__drm_kms_helper_poll_enable(dev);
+	drm_kms_helper_poll_enable_locked(dev);
 	mutex_unlock(&dev->mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_enable);
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 2a747a9..3febb4b 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
 
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
+extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
 
 #endif
-- 
1.8.4.5

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

[PATCH 2/2] drm/i915: Call non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

drm_kms_helper_poll_enable() is called from a context in
intel_hpd_irq_storm_disable() where the the mode_config mutex is
already locked.
When this function was converted to lock this mutex in
commit 8c4ccc4ab6f6 ("drm/probe-helper: Grab mode_config.mutex
in poll_init/enable") a deadlock occurred.
Call the newly implemented non-locking version of this function.

Changes since v1:
- use function name suffix '_locked' for the function that
  is to be called from a locked context.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 53c0173..b177857 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -180,7 +180,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 
 	/* Enable polling and queue hotplug re-enabling. */
 	if (hpd_disabled) {
-		drm_kms_helper_poll_enable(dev);
+		drm_kms_helper_poll_enable_locked(dev);
 		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
 				 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
 	}
-- 
1.8.4.5

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

[PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

[Egbert Eich]

An HPD interrupt may fire while we are in a function that changes
the PORT_HOTPLUG_EN register - especially when an HPD interrupt
storm occurs.
Since the interrupt handler changes the enabled HPD lines when it
detects such a storm the read-modify-write cycles may interfere.
To avoid this, shiled the rmw cycles with IRQ save spinlocks.

Changes since v1:
- Implement a function which takes care of accessing PORT_HOTPLUG_EN.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++
 drivers/gpu/drm/i915/i915_irq.c  | 64 ++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_crt.c | 11 ++++---
 3 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bf33d6e..a6b7576 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2737,6 +2737,9 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 
 void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv);
 void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv);
+void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
+				   uint32_t mask,
+				   uint32_t bits);
 void
 ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask);
 void
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a8aa797..ff85eae 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -167,6 +167,44 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
 
 static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
 
+/* For display hotplug interrupt */
+static inline void
+i915_hotplug_interrupt_update_locked(struct drm_i915_private *dev_priv,
+				     uint32_t mask,
+				     uint32_t bits)
+{
+	uint32_t val;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+	WARN_ON(bits & ~mask);
+
+	val = I915_READ(PORT_HOTPLUG_EN);
+	val &= ~mask;
+	val |= bits;
+	I915_WRITE(PORT_HOTPLUG_EN, val);
+}
+
+/**
+ * i915_hotplug_interrupt_update - update hotplug interrupt enable
+ * @dev_priv: driver private
+ * @mask: bits to update
+ * @bits: bits to enable
+ * NOTE: the HPD enable bits are modified both inside and outside
+ * of an interrupt context. To avoid that read-modify-write cycles
+ * interfer, these bits are protected by a spinlock. Since this
+ * function is usually not called from a context where the lock is
+ * held already, this function acquires the lock itself. A non-locking
+ * version is also available.
+ */
+void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
+				   uint32_t mask,
+				   uint32_t bits)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	i915_hotplug_interrupt_update_locked(dev_priv, mask, bits);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 /**
  * ilk_update_display_irq - update DEIMR
  * @dev_priv: driver private
@@ -3074,7 +3112,7 @@ static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
+	i915_hotplug_interrupt_update(dev_priv, 0xFFFFFFFF, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
 	for_each_pipe(dev_priv, pipe)
@@ -3490,7 +3528,7 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
 {
 	dev_priv->irq_mask = ~0;
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
+	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
 	I915_WRITE(VLV_IIR, 0xffffffff);
@@ -3864,7 +3902,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
 	int pipe;
 
 	if (I915_HAS_HOTPLUG(dev)) {
-		I915_WRITE(PORT_HOTPLUG_EN, 0);
+		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	}
 
@@ -3898,7 +3936,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
 		I915_USER_INTERRUPT;
 
 	if (I915_HAS_HOTPLUG(dev)) {
-		I915_WRITE(PORT_HOTPLUG_EN, 0);
+		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 		POSTING_READ(PORT_HOTPLUG_EN);
 
 		/* Enable in IER... */
@@ -4060,7 +4098,7 @@ static void i915_irq_uninstall(struct drm_device * dev)
 	int pipe;
 
 	if (I915_HAS_HOTPLUG(dev)) {
-		I915_WRITE(PORT_HOTPLUG_EN, 0);
+		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	}
 
@@ -4081,7 +4119,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe;
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
+	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
 	I915_WRITE(HWSTAM, 0xeffe);
@@ -4142,7 +4180,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
+	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
 	i915_enable_asle_pipestat(dev);
@@ -4157,22 +4195,22 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	hotplug_en = I915_READ(PORT_HOTPLUG_EN);
-	hotplug_en &= ~HOTPLUG_INT_EN_MASK;
 	/* Note HDMI and DP share hotplug bits */
 	/* enable bits are the same for all generations */
-	hotplug_en |= intel_hpd_enabled_irqs(dev, hpd_mask_i915);
+	hotplug_en = intel_hpd_enabled_irqs(dev, hpd_mask_i915);
 	/* Programming the CRT detection parameters tends
 	   to generate a spurious hotplug event about three
 	   seconds later.  So just do it once.
 	*/
 	if (IS_G4X(dev))
 		hotplug_en |= CRT_HOTPLUG_ACTIVATION_PERIOD_64;
-	hotplug_en &= ~CRT_HOTPLUG_VOLTAGE_COMPARE_MASK;
 	hotplug_en |= CRT_HOTPLUG_VOLTAGE_COMPARE_50;
 
 	/* Ignore TV since it's buggy */
-	I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+	i915_hotplug_interrupt_update_locked(dev_priv,
+				      (HOTPLUG_INT_EN_MASK
+				       | CRT_HOTPLUG_VOLTAGE_COMPARE_MASK),
+				      hotplug_en);
 }
 
 static irqreturn_t i965_irq_handler(int irq, void *arg)
@@ -4285,7 +4323,7 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	if (!dev_priv)
 		return;
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
+	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
 	I915_WRITE(HWSTAM, 0xffffffff);
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index af5e43b..6ce38e3 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -376,7 +376,7 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 hotplug_en, orig, stat;
+	u32 stat;
 	bool ret = false;
 	int i, tries = 0;
 
@@ -395,12 +395,12 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 		tries = 2;
 	else
 		tries = 1;
-	hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
-	hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
 
 	for (i = 0; i < tries ; i++) {
 		/* turn on the FORCE_DETECT */
-		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+		i915_hotplug_interrupt_update(dev_priv,
+					      CRT_HOTPLUG_FORCE_DETECT,
+					      CRT_HOTPLUG_FORCE_DETECT);
 		/* wait for FORCE_DETECT to go off */
 		if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
 			      CRT_HOTPLUG_FORCE_DETECT) == 0,
@@ -415,8 +415,7 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 	/* clear the interrupt we just generated, if any */
 	I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
 
-	/* and put the bits back */
-	I915_WRITE(PORT_HOTPLUG_EN, orig);
+	i915_hotplug_interrupt_update(dev_priv, CRT_HOTPLUG_FORCE_DETECT, 0);
 
 	return ret;
 }
-- 
1.8.4.5

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

Re: [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Daniel Vetter]

On Wed, Sep 23, 2015 at 04:13:00PM +0200, Egbert Eich wrote:
> drm_kms_helper_poll_enable() was converted to lock the mode_config
> mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
> 
> This disregarded the cases where this function is called from a context
> where this mutex is already locked.
> 
> Add a non-locking version as well.
> 
> Changes since v1:
> - use function name suffix '_locked' for the function that
>   is to be called from a locked context.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jani can you please pick these two up for -fixes since the 2nd patch fixes
a regression?

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++---
>  include/drm/drm_crtc_helper.h      |  1 +
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index d734780..2b9ce37 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  	return 1;
>  }
>  
> +/**
> + * drm_kms_helper_poll_enable_locked - re-enable output polling.
> + * @dev: drm_device
> + *
> + * This function re-enables the output polling work without
> + * locking the mode_config mutex.
> + *
> + * This is like drm_kms_helper_poll_enable() however it is to be
> + * called from a context where the mode_config mutex is locked
> + * already.
> + */
>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> -static void __drm_kms_helper_poll_enable(struct drm_device *dev)
> +void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>  {
>  	bool poll = false;
>  	struct drm_connector *connector;
> @@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev)
>  	if (poll)
>  		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
>  }
> +EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
> +
>  
>  static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector,
>  							      uint32_t maxX, uint32_t maxY, bool merge_type_bits)
> @@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>  
>  	/* Re-enable polling in case the global poll config changed. */
>  	if (drm_kms_helper_poll != dev->mode_config.poll_running)
> -		__drm_kms_helper_poll_enable(dev);
> +		drm_kms_helper_poll_enable_locked(dev);
>  
>  	dev->mode_config.poll_running = drm_kms_helper_poll;
>  
> @@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>  void drm_kms_helper_poll_enable(struct drm_device *dev)
>  {
>  	mutex_lock(&dev->mode_config.mutex);
> -	__drm_kms_helper_poll_enable(dev);
> +	drm_kms_helper_poll_enable_locked(dev);
>  	mutex_unlock(&dev->mode_config.mutex);
>  }
>  EXPORT_SYMBOL(drm_kms_helper_poll_enable);
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index 2a747a9..3febb4b 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
>  
>  extern void drm_kms_helper_poll_disable(struct drm_device *dev);
>  extern void drm_kms_helper_poll_enable(struct drm_device *dev);
> +extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
>  
>  #endif
> -- 
> 1.8.4.5
> 

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

Re: [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

[Daniel Vetter]

On Wed, Sep 23, 2015 at 04:15:27PM +0200, Egbert Eich wrote:
> An HPD interrupt may fire while we are in a function that changes
> the PORT_HOTPLUG_EN register - especially when an HPD interrupt
> storm occurs.
> Since the interrupt handler changes the enabled HPD lines when it
> detects such a storm the read-modify-write cycles may interfere.
> To avoid this, shiled the rmw cycles with IRQ save spinlocks.
> 
> Changes since v1:
> - Implement a function which takes care of accessing PORT_HOTPLUG_EN.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

Looks pretty. Queued for -next, thanks for the patch (assuming that we
don't need this for -fixes since there's no bug report linked). Please
correct me so I can drop this and let Jani pick it up instead.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 ++
>  drivers/gpu/drm/i915/i915_irq.c  | 64 ++++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_crt.c | 11 ++++---
>  3 files changed, 59 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bf33d6e..a6b7576 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2737,6 +2737,9 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  
>  void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv);
>  void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv);
> +void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
> +				   uint32_t mask,
> +				   uint32_t bits);
>  void
>  ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask);
>  void
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a8aa797..ff85eae 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -167,6 +167,44 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
>  
>  static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>  
> +/* For display hotplug interrupt */
> +static inline void
> +i915_hotplug_interrupt_update_locked(struct drm_i915_private *dev_priv,
> +				     uint32_t mask,
> +				     uint32_t bits)
> +{
> +	uint32_t val;
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +	WARN_ON(bits & ~mask);
> +
> +	val = I915_READ(PORT_HOTPLUG_EN);
> +	val &= ~mask;
> +	val |= bits;
> +	I915_WRITE(PORT_HOTPLUG_EN, val);
> +}
> +
> +/**
> + * i915_hotplug_interrupt_update - update hotplug interrupt enable
> + * @dev_priv: driver private
> + * @mask: bits to update
> + * @bits: bits to enable
> + * NOTE: the HPD enable bits are modified both inside and outside
> + * of an interrupt context. To avoid that read-modify-write cycles
> + * interfer, these bits are protected by a spinlock. Since this
> + * function is usually not called from a context where the lock is
> + * held already, this function acquires the lock itself. A non-locking
> + * version is also available.
> + */
> +void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
> +				   uint32_t mask,
> +				   uint32_t bits)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	i915_hotplug_interrupt_update_locked(dev_priv, mask, bits);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
>  /**
>   * ilk_update_display_irq - update DEIMR
>   * @dev_priv: driver private
> @@ -3074,7 +3112,7 @@ static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
>  {
>  	enum pipe pipe;
>  
> -	I915_WRITE(PORT_HOTPLUG_EN, 0);
> +	i915_hotplug_interrupt_update(dev_priv, 0xFFFFFFFF, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
>  	for_each_pipe(dev_priv, pipe)
> @@ -3490,7 +3528,7 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
>  {
>  	dev_priv->irq_mask = ~0;
>  
> -	I915_WRITE(PORT_HOTPLUG_EN, 0);
> +	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  	POSTING_READ(PORT_HOTPLUG_EN);
>  
>  	I915_WRITE(VLV_IIR, 0xffffffff);
> @@ -3864,7 +3902,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
>  	int pipe;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> -		I915_WRITE(PORT_HOTPLUG_EN, 0);
> +		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  	}
>  
> @@ -3898,7 +3936,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  		I915_USER_INTERRUPT;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> -		I915_WRITE(PORT_HOTPLUG_EN, 0);
> +		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  		POSTING_READ(PORT_HOTPLUG_EN);
>  
>  		/* Enable in IER... */
> @@ -4060,7 +4098,7 @@ static void i915_irq_uninstall(struct drm_device * dev)
>  	int pipe;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> -		I915_WRITE(PORT_HOTPLUG_EN, 0);
> +		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  	}
>  
> @@ -4081,7 +4119,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe;
>  
> -	I915_WRITE(PORT_HOTPLUG_EN, 0);
> +	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
>  	I915_WRITE(HWSTAM, 0xeffe);
> @@ -4142,7 +4180,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  	I915_WRITE(IER, enable_mask);
>  	POSTING_READ(IER);
>  
> -	I915_WRITE(PORT_HOTPLUG_EN, 0);
> +	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  	POSTING_READ(PORT_HOTPLUG_EN);
>  
>  	i915_enable_asle_pipestat(dev);
> @@ -4157,22 +4195,22 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> -	hotplug_en &= ~HOTPLUG_INT_EN_MASK;
>  	/* Note HDMI and DP share hotplug bits */
>  	/* enable bits are the same for all generations */
> -	hotplug_en |= intel_hpd_enabled_irqs(dev, hpd_mask_i915);
> +	hotplug_en = intel_hpd_enabled_irqs(dev, hpd_mask_i915);
>  	/* Programming the CRT detection parameters tends
>  	   to generate a spurious hotplug event about three
>  	   seconds later.  So just do it once.
>  	*/
>  	if (IS_G4X(dev))
>  		hotplug_en |= CRT_HOTPLUG_ACTIVATION_PERIOD_64;
> -	hotplug_en &= ~CRT_HOTPLUG_VOLTAGE_COMPARE_MASK;
>  	hotplug_en |= CRT_HOTPLUG_VOLTAGE_COMPARE_50;
>  
>  	/* Ignore TV since it's buggy */
> -	I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> +	i915_hotplug_interrupt_update_locked(dev_priv,
> +				      (HOTPLUG_INT_EN_MASK
> +				       | CRT_HOTPLUG_VOLTAGE_COMPARE_MASK),
> +				      hotplug_en);
>  }
>  
>  static irqreturn_t i965_irq_handler(int irq, void *arg)
> @@ -4285,7 +4323,7 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	if (!dev_priv)
>  		return;
>  
> -	I915_WRITE(PORT_HOTPLUG_EN, 0);
> +	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
>  	I915_WRITE(HWSTAM, 0xffffffff);
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index af5e43b..6ce38e3 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -376,7 +376,7 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 hotplug_en, orig, stat;
> +	u32 stat;
>  	bool ret = false;
>  	int i, tries = 0;
>  
> @@ -395,12 +395,12 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  		tries = 2;
>  	else
>  		tries = 1;
> -	hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
> -	hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
>  
>  	for (i = 0; i < tries ; i++) {
>  		/* turn on the FORCE_DETECT */
> -		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> +		i915_hotplug_interrupt_update(dev_priv,
> +					      CRT_HOTPLUG_FORCE_DETECT,
> +					      CRT_HOTPLUG_FORCE_DETECT);
>  		/* wait for FORCE_DETECT to go off */
>  		if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
>  			      CRT_HOTPLUG_FORCE_DETECT) == 0,
> @@ -415,8 +415,7 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  	/* clear the interrupt we just generated, if any */
>  	I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
>  
> -	/* and put the bits back */
> -	I915_WRITE(PORT_HOTPLUG_EN, orig);
> +	i915_hotplug_interrupt_update(dev_priv, CRT_HOTPLUG_FORCE_DETECT, 0);
>  
>  	return ret;
>  }
> -- 
> 1.8.4.5
> 

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

Re: [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

[Egbert Eich]

Daniel Vetter writes:
 > On Wed, Sep 23, 2015 at 04:15:27PM +0200, Egbert Eich wrote:
 > > An HPD interrupt may fire while we are in a function that changes
 > > the PORT_HOTPLUG_EN register - especially when an HPD interrupt
 > > storm occurs.
 > > Since the interrupt handler changes the enabled HPD lines when it
 > > detects such a storm the read-modify-write cycles may interfere.
 > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
 > > 
 > > Changes since v1:
 > > - Implement a function which takes care of accessing PORT_HOTPLUG_EN.
 > > 
 > > Signed-off-by: Egbert Eich <eich@suse.de>
 > 
 > Looks pretty. Queued for -next, thanks for the patch (assuming that we
 > don't need this for -fixes since there's no bug report linked). Please
 > correct me so I can drop this and let Jani pick it up instead.

I didn't bother to file a bug report. I know only one machine that's
affected. 
However the problem this fixes seems to be what caused spurious warnings
which we tried to get rid of with
WARN_ONCE( >>>> INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev) <<<< ,
                                  "Received HPD interrupt on pin %d although disabled\n", i);

as I did not see these warnings on my gen3 when I removed these tests.

BTW: Using 
     i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0) 
in the *_irq_pre/post/uninstall() functions does not help us much 
in terms of avoiding races. 
It can still happen that an interrupts or reenable worker gets fired 
and resets these values after the spinlock is released in
i915_hotplug_interrupt_update().

IHMO one must 
a. cancel the delayed worker, 
b. disable all interrupt pins and
c. call hpd_irq_setup() 
before calling intel_runtime_pm_disable_interrupts() to avoid this race.

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

Re: [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Wed, 23 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 23, 2015 at 04:13:00PM +0200, Egbert Eich wrote:
>> drm_kms_helper_poll_enable() was converted to lock the mode_config
>> mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
>> ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
>> 
>> This disregarded the cases where this function is called from a context
>> where this mutex is already locked.
>> 
>> Add a non-locking version as well.
>> 
>> Changes since v1:
>> - use function name suffix '_locked' for the function that
>>   is to be called from a locked context.
>> 
>> Signed-off-by: Egbert Eich <eich@suse.de>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani can you please pick these two up for -fixes since the 2nd patch fixes
> a regression?
>
> Thanks, Daniel
>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++---
>>  include/drm/drm_crtc_helper.h      |  1 +
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index d734780..2b9ce37 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>  	return 1;
>>  }
>>  
>> +/**
>> + * drm_kms_helper_poll_enable_locked - re-enable output polling.
>> + * @dev: drm_device
>> + *
>> + * This function re-enables the output polling work without
>> + * locking the mode_config mutex.
>> + *
>> + * This is like drm_kms_helper_poll_enable() however it is to be
>> + * called from a context where the mode_config mutex is locked
>> + * already.
>> + */
>>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
>> -static void __drm_kms_helper_poll_enable(struct drm_device *dev)
>> +void drm_kms_helper_poll_enable_locked(struct drm_device *dev)

Shouldn't this be _unlocked?

I thought the convention was that functions that do not acquire locks
are called _unlocked (although they may require a lock to be held when
called). And you might have foo() that grabs locks around a call to
foo_unlocked().

BR,
Jani.



>>  {
>>  	bool poll = false;
>>  	struct drm_connector *connector;
>> @@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev)
>>  	if (poll)
>>  		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
>>  }
>> +EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
>> +
>>  
>>  static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector,
>>  							      uint32_t maxX, uint32_t maxY, bool merge_type_bits)
>> @@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>>  
>>  	/* Re-enable polling in case the global poll config changed. */
>>  	if (drm_kms_helper_poll != dev->mode_config.poll_running)
>> -		__drm_kms_helper_poll_enable(dev);
>> +		drm_kms_helper_poll_enable_locked(dev);
>>  
>>  	dev->mode_config.poll_running = drm_kms_helper_poll;
>>  
>> @@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>>  void drm_kms_helper_poll_enable(struct drm_device *dev)
>>  {
>>  	mutex_lock(&dev->mode_config.mutex);
>> -	__drm_kms_helper_poll_enable(dev);
>> +	drm_kms_helper_poll_enable_locked(dev);
>>  	mutex_unlock(&dev->mode_config.mutex);
>>  }
>>  EXPORT_SYMBOL(drm_kms_helper_poll_enable);
>> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
>> index 2a747a9..3febb4b 100644
>> --- a/include/drm/drm_crtc_helper.h
>> +++ b/include/drm/drm_crtc_helper.h
>> @@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
>>  
>>  extern void drm_kms_helper_poll_disable(struct drm_device *dev);
>>  extern void drm_kms_helper_poll_enable(struct drm_device *dev);
>> +extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
>>  
>>  #endif
>> -- 
>> 1.8.4.5
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
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 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

Jani Nikula writes:
 > 
 > Shouldn't this be _unlocked?
 > 
 > I thought the convention was that functions that do not acquire locks
 > are called _unlocked (although they may require a lock to be held when
 > called). And you might have foo() that grabs locks around a call to
 > foo_unlocked().
 > 

Looking into this, functions that are to be called in a context where
the lock is already held should receive the suffix _locked while
those which do locking themselves and thus need to be called from
a context that doesn't hold this lock already receive the suffix 
_unlocked: the past tense refers to what has happened before.

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

[PATCH] drm/i915: On reset/suspend disable hpd pins & cancel pending delayed work

[Egbert Eich]

This makes sure no hpd interrupt or reenable worker fires when
resetting or suspending.
We already call intel_hpd_init() in most cases on resume and
after reset to undo this.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.c      |  3 +++
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 drivers/gpu/drm/i915/intel_hotplug.c | 28 ++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e2bf9e2..cb8d75f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -620,6 +620,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_dp_mst_suspend(dev);
 
+	intel_hpd_uninit(dev_priv);
 	intel_runtime_pm_disable_interrupts(dev_priv);
 	intel_hpd_cancel_work(dev_priv);
 
@@ -1477,12 +1478,14 @@ static int intel_runtime_suspend(struct device *device)
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_suspend_gt_powersave(dev);
+	intel_hpd_uninit(dev_priv);
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
 	ret = intel_suspend_complete(dev_priv);
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_enable_interrupts(dev_priv);
+		intel_hpd_init(dev_priv);
 
 		return ret;
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a6b7576..3c06629 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2688,6 +2688,7 @@ void i915_firmware_load_error_print(const char *fw_path, int err);
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
+void intel_hpd_uninit(struct drm_i915_private *dev_priv);
 void intel_hpd_init_work(struct drm_i915_private *dev_priv);
 void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
 bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fc00867..fadd4f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3207,16 +3207,17 @@ void intel_finish_reset(struct drm_device *dev)
 	 * The display has been reset as well,
 	 * so need a full re-initialization.
 	 */
+	intel_hpd_uninit(dev_priv);
 	intel_runtime_pm_disable_interrupts(dev_priv);
 	intel_runtime_pm_enable_interrupts(dev_priv);
 
 	intel_modeset_init_hw(dev);
-
+#if 0
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (dev_priv->display.hpd_irq_setup)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
-
+#endif
 	intel_display_resume(dev);
 
 	intel_hpd_init(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index b177857..4f2a179 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -484,6 +484,34 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
+/**
+ * intel_hpd_uninit - disable and deinitialize hpd support
+ * @dev_priv: i915 device instance
+ *
+ * This function disables any pending delayed work for HPD and
+ * disables all hpd pins. Calling this during suspend/reset
+ * avoids conflicts with the reenable worker of interrupt handler
+ * firing.
+ * At resume we call intel_hpd_init() to reenable things.
+ */
+void intel_hpd_uninit(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int i;
+
+	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	for_each_hpd_pin(i) {
+		dev_priv->hotplug.stats[i].state = HPD_DISABLED;
+	}
+	if (dev_priv->display.hpd_irq_setup)
+		dev_priv->display.hpd_irq_setup(dev);
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 {
 	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
-- 
1.8.4.5

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

Re: [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Fri, 25 Sep 2015, Egbert Eich <eich@suse.com> wrote:
> Jani Nikula writes:
>  > 
>  > Shouldn't this be _unlocked?
>  > 
>  > I thought the convention was that functions that do not acquire locks
>  > are called _unlocked (although they may require a lock to be held when
>  > called). And you might have foo() that grabs locks around a call to
>  > foo_unlocked().
>  > 
>
> Looking into this, functions that are to be called in a context where
> the lock is already held should receive the suffix _locked while
> those which do locking themselves and thus need to be called from
> a context that doesn't hold this lock already receive the suffix 
> _unlocked: the past tense refers to what has happened before.

I'm afraid existing conventions trump what makes sense.


BR,
Jani.


-- 
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: On reset/suspend disable hpd pins & cancel pending delayed work

[Ville Syrjälä]

On Fri, Sep 25, 2015 at 08:09:57AM +0200, Egbert Eich wrote:
> This makes sure no hpd interrupt or reenable worker fires when
> resetting or suspending.
> We already call intel_hpd_init() in most cases on resume and
> after reset to undo this.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c |  5 +++--
>  drivers/gpu/drm/i915/intel_hotplug.c | 28 ++++++++++++++++++++++++++++
>  4 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e2bf9e2..cb8d75f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -620,6 +620,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_dp_mst_suspend(dev);
>  
> +	intel_hpd_uninit(dev_priv);
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  	intel_hpd_cancel_work(dev_priv);
>  
> @@ -1477,12 +1478,14 @@ static int intel_runtime_suspend(struct device *device)
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_suspend_gt_powersave(dev);
> +	intel_hpd_uninit(dev_priv);
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  
>  	ret = intel_suspend_complete(dev_priv);
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>  		intel_runtime_pm_enable_interrupts(dev_priv);
> +		intel_hpd_init(dev_priv);
>  
>  		return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a6b7576..3c06629 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2688,6 +2688,7 @@ void i915_firmware_load_error_print(const char *fw_path, int err);
>  /* intel_hotplug.c */
>  void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
>  void intel_hpd_init(struct drm_i915_private *dev_priv);
> +void intel_hpd_uninit(struct drm_i915_private *dev_priv);
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv);
>  void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
>  bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fc00867..fadd4f2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3207,16 +3207,17 @@ void intel_finish_reset(struct drm_device *dev)
>  	 * The display has been reset as well,
>  	 * so need a full re-initialization.
>  	 */
> +	intel_hpd_uninit(dev_priv);
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  	intel_runtime_pm_enable_interrupts(dev_priv);
>  
>  	intel_modeset_init_hw(dev);
> -
> +#if 0
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	if (dev_priv->display.hpd_irq_setup)
>  		dev_priv->display.hpd_irq_setup(dev);
>  	spin_unlock_irq(&dev_priv->irq_lock);
> -
> +#endif

This pattern or hdp_irq_setup()+intel_hpd_init() has been cargo culted
all over the place. Can you just kill off the hpd_irq_setup() stuff from
everywhere where it's not needed?

>  	intel_display_resume(dev);
>  
>  	intel_hpd_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index b177857..4f2a179 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -484,6 +484,34 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> +/**
> + * intel_hpd_uninit - disable and deinitialize hpd support
> + * @dev_priv: i915 device instance
> + *
> + * This function disables any pending delayed work for HPD and
> + * disables all hpd pins. Calling this during suspend/reset
> + * avoids conflicts with the reenable worker of interrupt handler
> + * firing.
> + * At resume we call intel_hpd_init() to reenable things.
> + */
> +void intel_hpd_uninit(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int i;
> +
> +	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);

What if another hpd happens here?

Oh and we already have the intel_hpd_cancel_work() function. Sounds like
we need to rethink how that works rather than rolling another function
to do the same thing.

There's also the matter of VLV/CHV where hpd init gets called from the
display power well enable hook. I was thinking that could race with this
stuff, but since this is only called from the suspend I suppose we
shouldn't be re-enabling the display power well at that time.

> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	for_each_hpd_pin(i) {
> +		dev_priv->hotplug.stats[i].state = HPD_DISABLED;
> +	}
> +	if (dev_priv->display.hpd_irq_setup)
> +		dev_priv->display.hpd_irq_setup(dev);
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  {
>  	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> -- 
> 1.8.4.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: On reset/suspend disable hpd pins & cancel pending delayed work

[Daniel Vetter]

On Fri, Sep 25, 2015 at 03:29:40PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 25, 2015 at 08:09:57AM +0200, Egbert Eich wrote:
> > +void intel_hpd_uninit(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int i;
> > +
> > +	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
> 
> What if another hpd happens here?
> 
> Oh and we already have the intel_hpd_cancel_work() function. Sounds like
> we need to rethink how that works rather than rolling another function
> to do the same thing.
> 
> There's also the matter of VLV/CHV where hpd init gets called from the
> display power well enable hook. I was thinking that could race with this
> stuff, but since this is only called from the suspend I suppose we
> shouldn't be re-enabling the display power well at that time.

At least on big core we have checks for intel_display_power_is_enabled in
the irq postinstall hooks and corresponding post_enable code in the power
well code to make sure you can enable interrupts/power wells in any order
and it'll work.

Not sure this is a reasonable design assumption any more, but otoh fixing
that will likely mean we need to fix the power well handling in our driver
load and suspend/resume code.
-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

Re: [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Fri, 25 Sep 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 25 Sep 2015, Egbert Eich <eich@suse.com> wrote:
>> Jani Nikula writes:
>>  > 
>>  > Shouldn't this be _unlocked?
>>  > 
>>  > I thought the convention was that functions that do not acquire locks
>>  > are called _unlocked (although they may require a lock to be held when
>>  > called). And you might have foo() that grabs locks around a call to
>>  > foo_unlocked().
>>  > 
>>
>> Looking into this, functions that are to be called in a context where
>> the lock is already held should receive the suffix _locked while
>> those which do locking themselves and thus need to be called from
>> a context that doesn't hold this lock already receive the suffix 
>> _unlocked: the past tense refers to what has happened before.
>
> I'm afraid existing conventions trump what makes sense.

Egbert, I'm full of shit. Sorry. $BEVERAGE on me next time.

I'll queue these once I figure out through which tree.

BR,
Jani.

-- 
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 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Jani Nikula]

On Wed, 23 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 23, 2015 at 04:13:00PM +0200, Egbert Eich wrote:
>> drm_kms_helper_poll_enable() was converted to lock the mode_config
>> mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
>> ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
>> 
>> This disregarded the cases where this function is called from a context
>> where this mutex is already locked.
>> 
>> Add a non-locking version as well.
>> 
>> Changes since v1:
>> - use function name suffix '_locked' for the function that
>>   is to be called from a locked context.
>> 
>> Signed-off-by: Egbert Eich <eich@suse.de>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani can you please pick these two up for -fixes since the 2nd patch fixes
> a regression?

Pushed to drm-intel-fixes, thanks for the patches and review.

BR,
Jani.


>
> Thanks, Daniel
>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 19 ++++++++++++++++---
>>  include/drm/drm_crtc_helper.h      |  1 +
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index d734780..2b9ce37 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>  	return 1;
>>  }
>>  
>> +/**
>> + * drm_kms_helper_poll_enable_locked - re-enable output polling.
>> + * @dev: drm_device
>> + *
>> + * This function re-enables the output polling work without
>> + * locking the mode_config mutex.
>> + *
>> + * This is like drm_kms_helper_poll_enable() however it is to be
>> + * called from a context where the mode_config mutex is locked
>> + * already.
>> + */
>>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
>> -static void __drm_kms_helper_poll_enable(struct drm_device *dev)
>> +void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>>  {
>>  	bool poll = false;
>>  	struct drm_connector *connector;
>> @@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device *dev)
>>  	if (poll)
>>  		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
>>  }
>> +EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
>> +
>>  
>>  static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector,
>>  							      uint32_t maxX, uint32_t maxY, bool merge_type_bits)
>> @@ -174,7 +187,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>>  
>>  	/* Re-enable polling in case the global poll config changed. */
>>  	if (drm_kms_helper_poll != dev->mode_config.poll_running)
>> -		__drm_kms_helper_poll_enable(dev);
>> +		drm_kms_helper_poll_enable_locked(dev);
>>  
>>  	dev->mode_config.poll_running = drm_kms_helper_poll;
>>  
>> @@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>>  void drm_kms_helper_poll_enable(struct drm_device *dev)
>>  {
>>  	mutex_lock(&dev->mode_config.mutex);
>> -	__drm_kms_helper_poll_enable(dev);
>> +	drm_kms_helper_poll_enable_locked(dev);
>>  	mutex_unlock(&dev->mode_config.mutex);
>>  }
>>  EXPORT_SYMBOL(drm_kms_helper_poll_enable);
>> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
>> index 2a747a9..3febb4b 100644
>> --- a/include/drm/drm_crtc_helper.h
>> +++ b/include/drm/drm_crtc_helper.h
>> @@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
>>  
>>  extern void drm_kms_helper_poll_disable(struct drm_device *dev);
>>  extern void drm_kms_helper_poll_enable(struct drm_device *dev);
>> +extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
>>  
>>  #endif
>> -- 
>> 1.8.4.5
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx