public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Egbert Eich <eich@suse.de>
To: intel-gfx@lists.freedesktop.org
Cc: Egbert Eich <eich@suse.de>, Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2
Date: Wed, 23 Sep 2015 16:15:27 +0200	[thread overview]
Message-ID: <1443017727-27813-1-git-send-email-eich@suse.de> (raw)
In-Reply-To: <20150902144619.GI1367@phenom.ffwll.local>

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

  parent reply	other threads:[~2015-09-23 14:16 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-01 20:21 [PATCH 0/4] Fix numerous issues with HPDstorm handling Egbert Eich
2015-09-01 20:21 ` [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable() Egbert Eich
2015-09-01 21:27   ` Lukas Wunner
2015-09-01 22:10     ` Egbert Eich
2015-09-01 22:31       ` Lukas Wunner
2015-09-02  4:57         ` Egbert Eich
2015-09-01 22:50     ` Egbert Eich
2015-09-02 11:57   ` Daniel Vetter
2015-09-01 20:21 ` [PATCH 2/4] drm/i915: Call " Egbert Eich
2015-09-02 11:58   ` Daniel Vetter
2015-09-23 14:13     ` [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2 Egbert Eich
2015-09-23 14:13       ` [PATCH 2/2] drm/i915: Call " Egbert Eich
2015-09-23 14:50       ` [PATCH 1/2] drm: Add a " Daniel Vetter
2015-09-24 12:46         ` Jani Nikula
2015-09-25  6:00           ` Egbert Eich
2015-09-25  7:52             ` Jani Nikula
2015-09-29 14:35               ` Jani Nikula
2015-09-30  8:38         ` Jani Nikula
2015-09-01 20:21 ` [PATCH 3/4] drm/i915: Use the correct hpd_status list for non-G4xx/VLV Egbert Eich
2015-09-02 12:00   ` Daniel Vetter
2015-09-02 12:25     ` Imre Deak
2015-09-02 13:42       ` Jani Nikula
2015-09-01 20:21 ` [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt Egbert Eich
2015-09-02 12:06   ` Daniel Vetter
2015-09-02 14:19     ` Egbert Eich
2015-09-02 14:32       ` Jani Nikula
2015-09-02 14:58         ` Egbert Eich
2015-09-02 14:46       ` Daniel Vetter
2015-09-02 15:17         ` Egbert Eich
2015-09-23 14:15         ` Egbert Eich [this message]
2015-09-23 14:57           ` [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2 Daniel Vetter
2015-09-23 15:43             ` Egbert Eich
2015-09-25  6:09               ` [PATCH] drm/i915: On reset/suspend disable hpd pins & cancel pending delayed work Egbert Eich
2015-09-25 12:29                 ` Ville Syrjälä
2015-09-28  7:36                   ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1443017727-27813-1-git-send-email-eich@suse.de \
    --to=eich@suse.de \
    --cc=daniel.vetter@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox