diff mbox series

[3/4] drm/i915: Split pch irq handling to ack+handler

Message ID 20190415154904.31055-4-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Finish the ack+handler split for irq handler | expand

Commit Message

Ville Syrjälä April 15, 2019, 3:49 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The proper way to process interrupts is to first acknowledge them
all, and later process them. Start down that path for pch interrupts
by collecting the relevant register values into a struct so that
we can carry them from the ack part to the handler part.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 198 +++++++++++++++++++++-----------
 1 file changed, 131 insertions(+), 67 deletions(-)

Comments

Chris Wilson April 15, 2019, 4:48 p.m. UTC | #1 
Quoting Ville Syrjala (2019-04-15 16:49:03)
> @@ -2563,15 +2613,20 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
>  
>         /* check event from PCH */
>         if (de_iir & DE_PCH_EVENT) {
> -               u32 pch_iir = I915_READ(SDEIIR);
> +               struct pch_irq_regs pch = {};

If I am following along correctly, we don't need the memset here as we
only ever check dependent members after a guard (such as the iir or
hotplug trigger).

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä April 15, 2019, 4:56 p.m. UTC | #2 
On Mon, Apr 15, 2019 at 05:48:04PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-04-15 16:49:03)
> > @@ -2563,15 +2613,20 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> >  
> >         /* check event from PCH */
> >         if (de_iir & DE_PCH_EVENT) {
> > -               u32 pch_iir = I915_READ(SDEIIR);
> > +               struct pch_irq_regs pch = {};
> 
> If I am following along correctly, we don't need the memset here as we
> only ever check dependent members after a guard (such as the iir or
> hotplug trigger).

IIRC I did the zero inits to avoid false positives from the compiler
once these structs get hoisted up to the main irq handler. But I must
admit it's been a while since I wrote the basic form of this the memory
is getting hazy.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b6f7b98b9ddb..14e0e9fe1853 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2276,6 +2276,19 @@  static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 	return ret;
 }
 
+struct pch_irq_regs {
+	u32 iir;
+	u32 serr_int; /* cpt/lpt */
+	union {
+		struct hpd_irq_regs hpd; /* ibx+ */
+		struct hpd_irq_regs ddi; /* icp+ */
+	};
+	union {
+		struct hpd_irq_regs hpd2; /* spt+ */
+		struct hpd_irq_regs tc; /* icp+ */
+	};
+};
+
 static void ibx_hpd_irq_ack(struct drm_i915_private *dev_priv,
 			    struct hpd_irq_regs *hpd)
 {
@@ -2312,15 +2325,21 @@  static void ibx_hpd_irq_handler(struct drm_i915_private *dev_priv,
 	intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
 }
 
-static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
+static void ibx_irq_ack(struct drm_i915_private *dev_priv,
+			struct pch_irq_regs *pch)
 {
-	struct hpd_irq_regs hpd = {};
-	int pipe;
+	pch->hpd.hotplug_trigger = pch->iir & SDE_HOTPLUG_MASK;
 
-	hpd.hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
+	ibx_hpd_irq_ack(dev_priv, &pch->hpd);
+}
 
-	ibx_hpd_irq_ack(dev_priv, &hpd);
-	ibx_hpd_irq_handler(dev_priv, &hpd, hpd_ibx);
+static void ibx_irq_handler(struct drm_i915_private *dev_priv,
+			    const struct pch_irq_regs *pch)
+{
+	u32 pch_iir = pch->iir;
+	enum pipe pipe;
+
+	ibx_hpd_irq_handler(dev_priv, &pch->hpd, hpd_ibx);
 
 	if (pch_iir & SDE_AUDIO_POWER_MASK) {
 		int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -2386,9 +2405,17 @@  static void ivb_err_int_handler(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN7_ERR_INT, err_int);
 }
 
-static void cpt_serr_int_handler(struct drm_i915_private *dev_priv)
+static void cpt_serr_int_ack(struct drm_i915_private *dev_priv,
+			     struct pch_irq_regs *pch)
 {
-	u32 serr_int = I915_READ(SERR_INT);
+	pch->serr_int = I915_READ(SERR_INT);
+	I915_WRITE(SERR_INT, pch->serr_int);
+}
+
+static void cpt_serr_int_handler(struct drm_i915_private *dev_priv,
+				 const struct pch_irq_regs *pch)
+{
+	u32 serr_int = pch->serr_int;
 	enum pipe pipe;
 
 	if (serr_int & SERR_INT_POISON)
@@ -2397,19 +2424,26 @@  static void cpt_serr_int_handler(struct drm_i915_private *dev_priv)
 	for_each_pipe(dev_priv, pipe)
 		if (serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pipe))
 			intel_pch_fifo_underrun_irq_handler(dev_priv, pipe);
-
-	I915_WRITE(SERR_INT, serr_int);
 }
 
-static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
+static void cpt_irq_ack(struct drm_i915_private *dev_priv,
+			struct pch_irq_regs *pch)
 {
-	struct hpd_irq_regs hpd = {};
-	int pipe;
+	pch->hpd.hotplug_trigger = pch->iir & SDE_HOTPLUG_MASK_CPT;
+
+	ibx_hpd_irq_ack(dev_priv, &pch->hpd);
 
-	hpd.hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
+	if (pch->iir & SDE_ERROR_CPT)
+		cpt_serr_int_ack(dev_priv, pch);
+}
+
+static void cpt_irq_handler(struct drm_i915_private *dev_priv,
+			    const struct pch_irq_regs *pch)
+{
+	u32 pch_iir = pch->iir;
+	enum pipe pipe;
 
-	ibx_hpd_irq_ack(dev_priv, &hpd);
-	ibx_hpd_irq_handler(dev_priv, &hpd, hpd_cpt);
+	ibx_hpd_irq_handler(dev_priv, &pch->hpd, hpd_cpt);
 
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) {
 		int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
@@ -2437,33 +2471,41 @@  static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
 					 I915_READ(FDI_RX_IIR(pipe)));
 
 	if (pch_iir & SDE_ERROR_CPT)
-		cpt_serr_int_handler(dev_priv);
+		cpt_serr_int_handler(dev_priv, pch);
 }
 
-static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
+static void icp_irq_ack(struct drm_i915_private *dev_priv,
+			struct pch_irq_regs *pch)
 {
-	struct hpd_irq_regs ddi = {};
-	struct hpd_irq_regs tc = {};
+	pch->ddi.hotplug_trigger = pch->iir & SDE_DDI_MASK_ICP;
+	pch->tc.hotplug_trigger = pch->iir & SDE_TC_MASK_ICP;
+
+	if (pch->ddi.hotplug_trigger) {
+		pch->ddi.dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_DDI);
+		I915_WRITE(SHOTPLUG_CTL_DDI, pch->ddi.dig_hotplug_reg);
+	}
+
+	if (pch->tc.hotplug_trigger) {
+		pch->tc.dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_TC);
+		I915_WRITE(SHOTPLUG_CTL_TC, pch->tc.dig_hotplug_reg);
+	}
+}
+
+static void icp_irq_handler(struct drm_i915_private *dev_priv,
+			    const struct pch_irq_regs *pch)
+{
+	u32 pch_iir = pch->iir;
 	u32 pin_mask = 0, long_mask = 0;
 
-	ddi.hotplug_trigger = pch_iir & SDE_DDI_MASK_ICP;
-	tc.hotplug_trigger = pch_iir & SDE_TC_MASK_ICP;
-
-	if (ddi.hotplug_trigger) {
-		ddi.dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_DDI);
-		I915_WRITE(SHOTPLUG_CTL_DDI, ddi.dig_hotplug_reg);
-
+	if (pch->ddi.hotplug_trigger) {
 		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
-				   &ddi, hpd_icp,
+				   &pch->ddi, hpd_icp,
 				   icp_ddi_port_hotplug_long_detect);
 	}
 
-	if (tc.hotplug_trigger) {
-		tc.dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_TC);
-		I915_WRITE(SHOTPLUG_CTL_TC, tc.dig_hotplug_reg);
-
+	if (pch->tc.hotplug_trigger) {
 		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
-				   &tc, hpd_icp,
+				   &pch->tc, hpd_icp,
 				   icp_tc_port_hotplug_long_detect);
 	}
 
@@ -2474,31 +2516,39 @@  static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
 		gmbus_irq_handler(dev_priv);
 }
 
-static void spt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
+static void spt_irq_ack(struct drm_i915_private *dev_priv,
+			struct pch_irq_regs *pch)
 {
-	u32 pin_mask = 0, long_mask = 0;
-	struct hpd_irq_regs hpd = {};
-	struct hpd_irq_regs hpd2 = {};
-
-	hpd.hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_SPT &
+	pch->hpd.hotplug_trigger = pch->iir & SDE_HOTPLUG_MASK_SPT &
 		~SDE_PORTE_HOTPLUG_SPT;
-	hpd2.hotplug_trigger = pch_iir & SDE_PORTE_HOTPLUG_SPT;
+	pch->hpd2.hotplug_trigger = pch->iir & SDE_PORTE_HOTPLUG_SPT;
 
-	if (hpd.hotplug_trigger) {
-		hpd.dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
-		I915_WRITE(PCH_PORT_HOTPLUG, hpd.dig_hotplug_reg);
+	if (pch->hpd.hotplug_trigger) {
+		pch->hpd.dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
+		I915_WRITE(PCH_PORT_HOTPLUG, pch->hpd.dig_hotplug_reg);
+	}
 
+	if (pch->hpd2.hotplug_trigger) {
+		pch->hpd2.dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG2);
+		I915_WRITE(PCH_PORT_HOTPLUG2, pch->hpd2.dig_hotplug_reg);
+	}
+}
+
+static void spt_irq_handler(struct drm_i915_private *dev_priv,
+			    const struct pch_irq_regs *pch)
+{
+	u32 pch_iir = pch->iir;
+	u32 pin_mask = 0, long_mask = 0;
+
+	if (pch->hpd.hotplug_trigger) {
 		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
-				   &hpd, hpd_spt,
+				   &pch->hpd, hpd_spt,
 				   spt_port_hotplug_long_detect);
 	}
 
-	if (hpd2.hotplug_trigger) {
-		hpd2.dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG2);
-		I915_WRITE(PCH_PORT_HOTPLUG2, hpd2.dig_hotplug_reg);
-
+	if (pch->hpd2.hotplug_trigger) {
 		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
-				   &hpd2, hpd_spt,
+				   &pch->hpd2, hpd_spt,
 				   spt_port_hotplug2_long_detect);
 	}
 
@@ -2563,15 +2613,20 @@  static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
 
 	/* check event from PCH */
 	if (de_iir & DE_PCH_EVENT) {
-		u32 pch_iir = I915_READ(SDEIIR);
+		struct pch_irq_regs pch = {};
 
-		if (HAS_PCH_CPT(dev_priv))
-			cpt_irq_handler(dev_priv, pch_iir);
-		else
-			ibx_irq_handler(dev_priv, pch_iir);
+		pch.iir = I915_READ(SDEIIR);
+
+		if (HAS_PCH_CPT(dev_priv)) {
+			cpt_irq_ack(dev_priv, &pch);
+			cpt_irq_handler(dev_priv, &pch);
+		} else {
+			ibx_irq_ack(dev_priv, &pch);
+			ibx_irq_handler(dev_priv, &pch);
+		}
 
 		/* should clear PCH hotplug event before clear CPU irq */
-		I915_WRITE(SDEIIR, pch_iir);
+		I915_WRITE(SDEIIR, pch.iir);
 	}
 
 	if (IS_GEN(dev_priv, 5) && de_iir & DE_PCU_EVENT)
@@ -2613,12 +2668,15 @@  static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
 
 	/* check event from PCH */
 	if (!HAS_PCH_NOP(dev_priv) && (de_iir & DE_PCH_EVENT_IVB)) {
-		u32 pch_iir = I915_READ(SDEIIR);
+		struct pch_irq_regs pch = {};
 
-		cpt_irq_handler(dev_priv, pch_iir);
+		pch.iir = I915_READ(SDEIIR);
+
+		cpt_irq_ack(dev_priv, &pch);
+		cpt_irq_handler(dev_priv, &pch);
 
 		/* clear PCH hotplug event before clear CPU irq */
-		I915_WRITE(SDEIIR, pch_iir);
+		I915_WRITE(SDEIIR, pch.iir);
 	}
 }
 
@@ -2902,22 +2960,28 @@  gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 
 	if (HAS_PCH_SPLIT(dev_priv) && !HAS_PCH_NOP(dev_priv) &&
 	    master_ctl & GEN8_DE_PCH_IRQ) {
+		struct pch_irq_regs pch = {};
+
 		/*
 		 * FIXME(BDW): Assume for now that the new interrupt handling
 		 * scheme also closed the SDE interrupt handling race we've seen
 		 * on older pch-split platforms. But this needs testing.
 		 */
-		iir = I915_READ(SDEIIR);
-		if (iir) {
-			I915_WRITE(SDEIIR, iir);
+		pch.iir = I915_READ(SDEIIR);
+		if (pch.iir) {
+			I915_WRITE(SDEIIR, pch.iir);
 			ret = IRQ_HANDLED;
 
-			if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
-				icp_irq_handler(dev_priv, iir);
-			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
-				spt_irq_handler(dev_priv, iir);
-			else
-				cpt_irq_handler(dev_priv, iir);
+			if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) {
+				icp_irq_ack(dev_priv, &pch);
+				icp_irq_handler(dev_priv, &pch);
+			} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT) {
+				spt_irq_ack(dev_priv, &pch);
+				spt_irq_handler(dev_priv, &pch);
+			} else {
+				cpt_irq_ack(dev_priv, &pch);
+				cpt_irq_handler(dev_priv, &pch);
+			}
 		} else {
 			/*
 			 * Like on previous PCH there seems to be something