* [PATCH 2/3] drm/i915: Remove the extraneous irqreturn_t from gen8 sub handlers
2017-09-13 18:18 [PATCH 1/3] drm/i915: Trim gen8_irq_handler Chris Wilson
@ 2017-09-13 18:18 ` Chris Wilson
2017-09-13 18:18 ` [PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler Chris Wilson
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2017-09-13 18:18 UTC (permalink / raw)
To: intel-gfx
We know whether or not the irq was intended for us by simpling
inspecting the GEN8_MASTER_IRQ. If that wasn't signaled, the irq was
spurious and that is when, and only when, we should report to the core
that we didn't handle the irq.
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
function old new delta
gen8_irq_handler 1623 1619 -4
gen8_gt_irq_ack 356 308 -48
Total: Before=1271276, After=1271224, chg -0.00%
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_irq.c | 54 ++++++++++++++---------------------------
1 file changed, 18 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e12321cb7403..e1d503b7352e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1326,52 +1326,43 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
tasklet_hi_schedule(&engine->irq_tasklet);
}
-static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
- u32 master_ctl,
- u32 gt_iir[4])
+static void gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
+ u32 master_ctl, u32 gt_iir[4])
{
- irqreturn_t ret = IRQ_NONE;
-
if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
gt_iir[0] = I915_READ_FW(GEN8_GT_IIR(0));
- if (gt_iir[0]) {
+ if (likely(gt_iir[0]))
I915_WRITE_FW(GEN8_GT_IIR(0), gt_iir[0]);
- ret = IRQ_HANDLED;
- } else
+ else
DRM_ERROR("The master control interrupt lied (GT0)!\n");
}
if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
gt_iir[1] = I915_READ_FW(GEN8_GT_IIR(1));
- if (gt_iir[1]) {
+ if (likely(gt_iir[1]))
I915_WRITE_FW(GEN8_GT_IIR(1), gt_iir[1]);
- ret = IRQ_HANDLED;
- } else
+ else
DRM_ERROR("The master control interrupt lied (GT1)!\n");
}
if (master_ctl & GEN8_GT_VECS_IRQ) {
gt_iir[3] = I915_READ_FW(GEN8_GT_IIR(3));
- if (gt_iir[3]) {
+ if (likely(gt_iir[3]))
I915_WRITE_FW(GEN8_GT_IIR(3), gt_iir[3]);
- ret = IRQ_HANDLED;
- } else
+ else
DRM_ERROR("The master control interrupt lied (GT3)!\n");
}
if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
- if (gt_iir[2] & (dev_priv->pm_rps_events |
- dev_priv->pm_guc_events)) {
+ if (likely(gt_iir[2] & (dev_priv->pm_rps_events |
+ dev_priv->pm_guc_events)))
I915_WRITE_FW(GEN8_GT_IIR(2),
gt_iir[2] & (dev_priv->pm_rps_events |
dev_priv->pm_guc_events));
- ret = IRQ_HANDLED;
- } else
+ else
DRM_ERROR("The master control interrupt lied (PM)!\n");
}
-
- return ret;
}
static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
@@ -2387,10 +2378,9 @@ static void bxt_hpd_irq_handler(struct drm_i915_private *dev_priv,
intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
}
-static irqreturn_t
+static void
gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
{
- irqreturn_t ret = IRQ_NONE;
u32 iir;
enum pipe pipe;
@@ -2398,13 +2388,11 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
iir = I915_READ(GEN8_DE_MISC_IIR);
if (iir) {
I915_WRITE(GEN8_DE_MISC_IIR, iir);
- ret = IRQ_HANDLED;
if (iir & GEN8_DE_MISC_GSE)
intel_opregion_asle_intr(dev_priv);
else
DRM_ERROR("Unexpected DE Misc interrupt\n");
- }
- else
+ } else
DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
}
@@ -2415,7 +2403,6 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
bool found = false;
I915_WRITE(GEN8_DE_PORT_IIR, iir);
- ret = IRQ_HANDLED;
tmp_mask = GEN8_AUX_CHANNEL_A;
if (INTEL_GEN(dev_priv) >= 9)
@@ -2468,7 +2455,6 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
continue;
}
- ret = IRQ_HANDLED;
I915_WRITE(GEN8_DE_PIPE_IIR(pipe), iir);
if (iir & GEN8_PIPE_VBLANK)
@@ -2502,9 +2488,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
iir = I915_READ(SDEIIR);
if (iir) {
I915_WRITE(SDEIIR, iir);
- ret = IRQ_HANDLED;
-
- if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
+ if (HAS_PCH_SPT(dev_priv) ||
+ HAS_PCH_KBP(dev_priv) ||
HAS_PCH_CNP(dev_priv))
spt_irq_handler(dev_priv, iir);
else
@@ -2517,8 +2502,6 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
DRM_DEBUG_DRIVER("The master control interrupt lied (SDE)!\n");
}
}
-
- return ret;
}
#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
@@ -2529,7 +2512,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
{
struct drm_i915_private *dev_priv = arg;
u32 master_ctl, gt_iir[4];
- irqreturn_t ret = IRQ_NONE;
if (!intel_irqs_enabled(dev_priv))
return IRQ_NONE;
@@ -2542,11 +2524,11 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
/* Find, clear, then process each source of interrupt */
if (master_ctl & GEN8_GT_IRQ_BITS)
- ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
+ gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
if (master_ctl & ~GEN8_GT_IRQ_BITS) {
disable_rpm_wakeref_asserts(dev_priv);
- ret |= gen8_de_irq_handler(dev_priv, master_ctl);
+ gen8_de_irq_handler(dev_priv, master_ctl);
enable_rpm_wakeref_asserts(dev_priv);
}
@@ -2555,7 +2537,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
if (master_ctl & GEN8_GT_IRQ_BITS)
gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
- return ret;
+ return IRQ_HANDLED;
}
struct wedge_me {
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler
2017-09-13 18:18 [PATCH 1/3] drm/i915: Trim gen8_irq_handler Chris Wilson
2017-09-13 18:18 ` [PATCH 2/3] drm/i915: Remove the extraneous irqreturn_t from gen8 sub handlers Chris Wilson
@ 2017-09-13 18:18 ` Chris Wilson
2017-09-14 16:02 ` Ville Syrjälä
2017-09-13 18:42 ` ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Trim gen8_irq_handler Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-09-13 18:18 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala
When handling context-switch interrupts, we are very latency sensitive;
every unnecessary mmio (uncached) read contributes toward a large
decrease in request throughput. An example is gem_exec_whisper, which
ping-pongs between the engines, where avoiding the pipe underflow
checking reduces the runtime of the test from 29s to 22s. On the other
hand, we are now not checking for pipe underflows at 100KHz, more or
less reducing it to a check every pageflip (60Hz) or modeset at worst.
add/remove: 0/0 grow/shrink: 1/2 up/down: 24/-40 (-16)
function old new delta
valleyview_pipestat_irq_ack 259 283 +24
valleyview_irq_handler 521 517 -4
cherryview_irq_handler 457 421 -36
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 123 ++++++++++++++++++++++++----------------
1 file changed, 73 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e1d503b7352e..345d73bd4039 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1703,16 +1703,17 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
}
}
-static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
+static bool valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
u32 iir, u32 pipe_stats[I915_MAX_PIPES])
{
+ bool handled = false;
int pipe;
spin_lock(&dev_priv->irq_lock);
if (!dev_priv->display_irqs_enabled) {
spin_unlock(&dev_priv->irq_lock);
- return;
+ return false;
}
for_each_pipe(dev_priv, pipe) {
@@ -1744,8 +1745,10 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
if (iir & iir_bit)
mask |= dev_priv->pipestat_irq_mask[pipe];
- if (!mask)
+ if (!mask) {
+ pipe_stats[pipe] = 0;
continue;
+ }
reg = PIPESTAT(pipe);
mask |= PIPESTAT_INT_ENABLE_MASK;
@@ -1757,8 +1760,12 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
PIPESTAT_INT_STATUS_MASK))
I915_WRITE(reg, pipe_stats[pipe]);
+
+ handled = true;
}
spin_unlock(&dev_priv->irq_lock);
+
+ return handled;
}
static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv,
@@ -1836,8 +1843,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
do {
u32 iir, gt_iir, pm_iir;
- u32 pipe_stats[I915_MAX_PIPES] = {};
+ u32 pipe_stats[I915_MAX_PIPES];
u32 hotplug_status = 0;
+ bool has_pipe_stats;
u32 ier = 0;
gt_iir = I915_READ(GTIIR);
@@ -1876,7 +1884,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
/* Call regardless, as some status bits might not be
* signalled in iir */
- valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
+ has_pipe_stats =
+ valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
if (iir & (I915_LPE_PIPE_A_INTERRUPT |
I915_LPE_PIPE_B_INTERRUPT))
@@ -1901,7 +1910,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
if (hotplug_status)
i9xx_hpd_irq_handler(dev_priv, hotplug_status);
- valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
+ if (has_pipe_stats)
+ valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
} while (0);
enable_rpm_wakeref_asserts(dev_priv);
@@ -1914,27 +1924,14 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
struct drm_device *dev = arg;
struct drm_i915_private *dev_priv = to_i915(dev);
irqreturn_t ret = IRQ_NONE;
+ u32 master_ctl;
if (!intel_irqs_enabled(dev_priv))
return IRQ_NONE;
- /* IRQs are synced during runtime_suspend, we don't require a wakeref */
- disable_rpm_wakeref_asserts(dev_priv);
-
+ master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
do {
- u32 master_ctl, iir;
- u32 gt_iir[4] = {};
- u32 pipe_stats[I915_MAX_PIPES] = {};
- u32 hotplug_status = 0;
- u32 ier = 0;
-
- master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
- iir = I915_READ(VLV_IIR);
-
- if (master_ctl == 0 && iir == 0)
- break;
-
- ret = IRQ_HANDLED;
+ u32 iir;
/*
* Theory on interrupt generation, based on empirical evidence:
@@ -1949,44 +1946,70 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
* don't end up clearing all the VLV_IIR and GEN8_MASTER_IRQ_CONTROL
* bits this time around.
*/
- I915_WRITE(GEN8_MASTER_IRQ, 0);
- ier = I915_READ(VLV_IER);
- I915_WRITE(VLV_IER, 0);
+ if (master_ctl & ~GEN8_MASTER_IRQ_CONTROL) {
+ u32 gt_iir[4];
- gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
+ I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
- if (iir & I915_DISPLAY_PORT_INTERRUPT)
- hotplug_status = i9xx_hpd_irq_ack(dev_priv);
+ gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
- /* Call regardless, as some status bits might not be
- * signalled in iir */
- valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
+ I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
+ ret = IRQ_HANDLED;
- if (iir & (I915_LPE_PIPE_A_INTERRUPT |
- I915_LPE_PIPE_B_INTERRUPT |
- I915_LPE_PIPE_C_INTERRUPT))
- intel_lpe_audio_irq_handler(dev_priv);
+ gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
+ master_ctl = 0;
+ }
- /*
- * VLV_IIR is single buffered, and reflects the level
- * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
- */
- if (iir)
- I915_WRITE(VLV_IIR, iir);
+ iir = I915_READ_FW(VLV_IIR);
+ if (iir) {
+ u32 pipe_stats[I915_MAX_PIPES];
+ u32 hotplug_status = 0;
+ bool has_pipe_stats = false;
+ u32 ier;
- I915_WRITE(VLV_IER, ier);
- I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
- POSTING_READ(GEN8_MASTER_IRQ);
+ /*
+ * IRQs are synced during runtime_suspend,
+ * we don't require a wakeref
+ */
+ disable_rpm_wakeref_asserts(dev_priv);
- gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
- if (hotplug_status)
- i9xx_hpd_irq_handler(dev_priv, hotplug_status);
+ ier = I915_READ_FW(VLV_IER);
+ I915_WRITE_FW(VLV_IER, 0);
- valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
- } while (0);
+ if (iir & I915_DISPLAY_PORT_INTERRUPT)
+ hotplug_status = i9xx_hpd_irq_ack(dev_priv);
- enable_rpm_wakeref_asserts(dev_priv);
+ if (iir & (I915_LPE_PIPE_A_INTERRUPT |
+ I915_LPE_PIPE_B_INTERRUPT |
+ I915_LPE_PIPE_C_INTERRUPT))
+ intel_lpe_audio_irq_handler(dev_priv);
+
+ /*
+ * Call regardless, as some status bits might not be
+ * signalled in iir.
+ */
+ has_pipe_stats =
+ valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
+
+ /*
+ * VLV_IIR is single buffered, and reflects the level
+ * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
+ */
+ I915_WRITE_FW(VLV_IIR, iir);
+ I915_WRITE_FW(VLV_IER, ier);
+ ret = IRQ_HANDLED;
+
+ if (hotplug_status)
+ i9xx_hpd_irq_handler(dev_priv, hotplug_status);
+
+ if (has_pipe_stats)
+ valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
+
+ enable_rpm_wakeref_asserts(dev_priv);
+ master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
+ }
+ } while (master_ctl & ~GEN8_MASTER_IRQ_CONTROL);
return ret;
}
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler
2017-09-13 18:18 ` [PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler Chris Wilson
@ 2017-09-14 16:02 ` Ville Syrjälä
0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2017-09-14 16:02 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Mika Kuoppala
On Wed, Sep 13, 2017 at 07:18:46PM +0100, Chris Wilson wrote:
> When handling context-switch interrupts, we are very latency sensitive;
> every unnecessary mmio (uncached) read contributes toward a large
> decrease in request throughput. An example is gem_exec_whisper, which
> ping-pongs between the engines, where avoiding the pipe underflow
> checking reduces the runtime of the test from 29s to 22s. On the other
> hand, we are now not checking for pipe underflows at 100KHz, more or
> less reducing it to a check every pageflip (60Hz) or modeset at worst.
>
> add/remove: 0/0 grow/shrink: 1/2 up/down: 24/-40 (-16)
> function old new delta
> valleyview_pipestat_irq_ack 259 283 +24
> valleyview_irq_handler 521 517 -4
> cherryview_irq_handler 457 421 -36
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 123 ++++++++++++++++++++++++----------------
> 1 file changed, 73 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e1d503b7352e..345d73bd4039 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1703,16 +1703,17 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
> }
> }
>
> -static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> +static bool valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> u32 iir, u32 pipe_stats[I915_MAX_PIPES])
> {
> + bool handled = false;
> int pipe;
>
> spin_lock(&dev_priv->irq_lock);
>
> if (!dev_priv->display_irqs_enabled) {
> spin_unlock(&dev_priv->irq_lock);
> - return;
> + return false;
> }
>
> for_each_pipe(dev_priv, pipe) {
> @@ -1744,8 +1745,10 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> if (iir & iir_bit)
> mask |= dev_priv->pipestat_irq_mask[pipe];
>
> - if (!mask)
> + if (!mask) {
> + pipe_stats[pipe] = 0;
> continue;
> + }
>
> reg = PIPESTAT(pipe);
> mask |= PIPESTAT_INT_ENABLE_MASK;
> @@ -1757,8 +1760,12 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> PIPESTAT_INT_STATUS_MASK))
> I915_WRITE(reg, pipe_stats[pipe]);
> +
> + handled = true;
> }
> spin_unlock(&dev_priv->irq_lock);
> +
> + return handled;
> }
>
> static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv,
> @@ -1836,8 +1843,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>
> do {
> u32 iir, gt_iir, pm_iir;
> - u32 pipe_stats[I915_MAX_PIPES] = {};
> + u32 pipe_stats[I915_MAX_PIPES];
> u32 hotplug_status = 0;
> + bool has_pipe_stats;
> u32 ier = 0;
>
> gt_iir = I915_READ(GTIIR);
> @@ -1876,7 +1884,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>
> /* Call regardless, as some status bits might not be
> * signalled in iir */
> - valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> + has_pipe_stats =
> + valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>
> if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> I915_LPE_PIPE_B_INTERRUPT))
> @@ -1901,7 +1910,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> if (hotplug_status)
> i9xx_hpd_irq_handler(dev_priv, hotplug_status);
>
> - valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> + if (has_pipe_stats)
> + valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> } while (0);
>
> enable_rpm_wakeref_asserts(dev_priv);
> @@ -1914,27 +1924,14 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> struct drm_device *dev = arg;
> struct drm_i915_private *dev_priv = to_i915(dev);
> irqreturn_t ret = IRQ_NONE;
> + u32 master_ctl;
>
> if (!intel_irqs_enabled(dev_priv))
> return IRQ_NONE;
>
> - /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> - disable_rpm_wakeref_asserts(dev_priv);
> -
> + master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> do {
> - u32 master_ctl, iir;
> - u32 gt_iir[4] = {};
> - u32 pipe_stats[I915_MAX_PIPES] = {};
> - u32 hotplug_status = 0;
> - u32 ier = 0;
> -
> - master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> - iir = I915_READ(VLV_IIR);
> -
> - if (master_ctl == 0 && iir == 0)
> - break;
> -
> - ret = IRQ_HANDLED;
> + u32 iir;
>
> /*
> * Theory on interrupt generation, based on empirical evidence:
> @@ -1949,44 +1946,70 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> * don't end up clearing all the VLV_IIR and GEN8_MASTER_IRQ_CONTROL
> * bits this time around.
> */
> - I915_WRITE(GEN8_MASTER_IRQ, 0);
> - ier = I915_READ(VLV_IER);
> - I915_WRITE(VLV_IER, 0);
> + if (master_ctl & ~GEN8_MASTER_IRQ_CONTROL) {
> + u32 gt_iir[4];
>
> - gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> + I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
>
> - if (iir & I915_DISPLAY_PORT_INTERRUPT)
> - hotplug_status = i9xx_hpd_irq_ack(dev_priv);
> + gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
>
> - /* Call regardless, as some status bits might not be
> - * signalled in iir */
> - valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> + I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> + ret = IRQ_HANDLED;
I suspect this won't work correctly. If I'm correct on how the edge
triggering works we really need to have both MASTER_IRQ_CONTROL and IER
disabled at the same time. Otherwise one can prevent the other from
generating an edge to the CPU interrupt logic.
>
> - if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> - I915_LPE_PIPE_B_INTERRUPT |
> - I915_LPE_PIPE_C_INTERRUPT))
> - intel_lpe_audio_irq_handler(dev_priv);
> + gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
> + master_ctl = 0;
> + }
>
> - /*
> - * VLV_IIR is single buffered, and reflects the level
> - * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> - */
> - if (iir)
> - I915_WRITE(VLV_IIR, iir);
> + iir = I915_READ_FW(VLV_IIR);
> + if (iir) {
> + u32 pipe_stats[I915_MAX_PIPES];
> + u32 hotplug_status = 0;
> + bool has_pipe_stats = false;
> + u32 ier;
>
> - I915_WRITE(VLV_IER, ier);
> - I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> - POSTING_READ(GEN8_MASTER_IRQ);
> + /*
> + * IRQs are synced during runtime_suspend,
> + * we don't require a wakeref
> + */
> + disable_rpm_wakeref_asserts(dev_priv);
>
> - gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>
> - if (hotplug_status)
> - i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> + ier = I915_READ_FW(VLV_IER);
> + I915_WRITE_FW(VLV_IER, 0);
>
> - valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> - } while (0);
> + if (iir & I915_DISPLAY_PORT_INTERRUPT)
> + hotplug_status = i9xx_hpd_irq_ack(dev_priv);
>
> - enable_rpm_wakeref_asserts(dev_priv);
> + if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> + I915_LPE_PIPE_B_INTERRUPT |
> + I915_LPE_PIPE_C_INTERRUPT))
> + intel_lpe_audio_irq_handler(dev_priv);
> +
> + /*
> + * Call regardless, as some status bits might not be
> + * signalled in iir.
> + */
> + has_pipe_stats =
> + valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> +
> + /*
> + * VLV_IIR is single buffered, and reflects the level
> + * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> + */
> + I915_WRITE_FW(VLV_IIR, iir);
> + I915_WRITE_FW(VLV_IER, ier);
> + ret = IRQ_HANDLED;
> +
> + if (hotplug_status)
> + i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> +
> + if (has_pipe_stats)
> + valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> +
> + enable_rpm_wakeref_asserts(dev_priv);
> + master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> + }
> + } while (master_ctl & ~GEN8_MASTER_IRQ_CONTROL);
>
> return ret;
> }
> --
> 2.14.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Trim gen8_irq_handler
2017-09-13 18:18 [PATCH 1/3] drm/i915: Trim gen8_irq_handler Chris Wilson
2017-09-13 18:18 ` [PATCH 2/3] drm/i915: Remove the extraneous irqreturn_t from gen8 sub handlers Chris Wilson
2017-09-13 18:18 ` [PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler Chris Wilson
@ 2017-09-13 18:42 ` Patchwork
2017-09-14 7:20 ` [PATCH 1/3] " Pandiyan, Dhinakaran
2017-09-14 15:46 ` Ville Syrjälä
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-09-13 18:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Trim gen8_irq_handler
URL : https://patchwork.freedesktop.org/series/30311/
State : warning
== Summary ==
Series 30311v1 series starting with [1/3] drm/i915: Trim gen8_irq_handler
https://patchwork.freedesktop.org/api/1.0/series/30311/revisions/1/mbox/
Test chamelium:
Subgroup dp-crc-fast:
pass -> FAIL (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
fail -> PASS (fi-snb-2600) fdo#100215
Test kms_flip:
Subgroup basic-flip-vs-modeset:
pass -> SKIP (fi-skl-x1585l) fdo#101781
Subgroup basic-plain-flip:
dmesg-warn -> PASS (fi-cfl-s) fdo#102294 +1
Test kms_frontbuffer_tracking:
Subgroup basic:
dmesg-warn -> PASS (fi-bdw-5557u) fdo#102473
Test kms_pipe_crc_basic:
Subgroup bad-nb-words-1:
pass -> DMESG-WARN (fi-cfl-s)
Subgroup bad-nb-words-3:
pass -> DMESG-WARN (fi-cfl-s)
Subgroup bad-pipe:
pass -> DMESG-WARN (fi-cfl-s)
Subgroup bad-source:
pass -> DMESG-WARN (fi-cfl-s)
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (fi-byt-n2820) fdo#101705
pass -> FAIL (fi-skl-6700k) fdo#100367
fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#102473 https://bugs.freedesktop.org/show_bug.cgi?id=102473
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fdo#100367 https://bugs.freedesktop.org/show_bug.cgi?id=100367
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:446s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:453s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:376s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:530s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:268s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:511s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:503s
fi-byt-n2820 total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:500s
fi-cfl-s total:289 pass:219 dwarn:38 dfail:0 fail:0 skip:32 time:555s
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:455s
fi-glk-2a total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:594s
fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:430s
fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:412s
fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:437s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:487s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:462s
fi-kbl-7500u total:289 pass:263 dwarn:1 dfail:0 fail:1 skip:24 time:489s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:576s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:585s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:551s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:456s
fi-skl-6700k total:289 pass:264 dwarn:0 dfail:0 fail:1 skip:24 time:511s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:501s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:463s
fi-skl-x1585l total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:471s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:568s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:1 skip:39 time:423s
fd4b78812edddb5118f97329ba04f36483e16e2a drm-tip: 2017y-09m-13d-17h-15m-44s UTC integration manifest
7dadef59de65 drm/i915: Skip pipestats for GT operations in chv/vlv irq handler
4908579a5dea drm/i915: Remove the extraneous irqreturn_t from gen8 sub handlers
2be56fc2436f drm/i915: Trim gen8_irq_handler
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5687/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler
2017-09-13 18:18 [PATCH 1/3] drm/i915: Trim gen8_irq_handler Chris Wilson
` (2 preceding siblings ...)
2017-09-13 18:42 ` ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Trim gen8_irq_handler Patchwork
@ 2017-09-14 7:20 ` Pandiyan, Dhinakaran
2017-09-14 8:37 ` Chris Wilson
2017-09-14 15:46 ` Ville Syrjälä
4 siblings, 1 reply; 10+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-14 7:20 UTC (permalink / raw)
To: chris@chris-wilson.co.uk; +Cc: intel-gfx@lists.freedesktop.org
On Wed, 2017-09-13 at 19:18 +0100, Chris Wilson wrote:
> The goal here is to trim an excess posting read and keep the predicates
Curious why we do the posting reads, is that a hardware requirement?
> tight (reusing the same predicate throughout for GT ack/handling).
>
> add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
> function old new delta
> gen8_gt_irq_handler 282 301 +19
> cherryview_irq_handler 450 457 +7
> gen8_irq_handler 1653 1623 -30
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5dbf2da..e12321cb7403 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
> }
>
> static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> - u32 gt_iir[4])
> + u32 master_ctl, u32 gt_iir[4])
> {
> - if (gt_iir[0]) {
> + if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
> gen8_cs_irq_handler(dev_priv->engine[RCS],
> gt_iir[0], GEN8_RCS_IRQ_SHIFT);
> gen8_cs_irq_handler(dev_priv->engine[BCS],
> gt_iir[0], GEN8_BCS_IRQ_SHIFT);
> }
>
> - if (gt_iir[1]) {
> + if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
> gen8_cs_irq_handler(dev_priv->engine[VCS],
> gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
> gen8_cs_irq_handler(dev_priv->engine[VCS2],
> gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
> }
>
> - if (gt_iir[3])
> + if (master_ctl & GEN8_GT_VECS_IRQ) {
> gen8_cs_irq_handler(dev_priv->engine[VECS],
> gt_iir[3], GEN8_VECS_IRQ_SHIFT);
> + }
>
> - if (gt_iir[2] & dev_priv->pm_rps_events)
> - gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> + if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> + if (gt_iir[2] & dev_priv->pm_rps_events)
> + gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>
> - if (gt_iir[2] & dev_priv->pm_guc_events)
> - gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> + if (gt_iir[2] & dev_priv->pm_guc_events)
> + gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> + }
> }
>
> static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> POSTING_READ(GEN8_MASTER_IRQ);
>
> - gen8_gt_irq_handler(dev_priv, gt_iir);
> + gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>
> if (hotplug_status)
> i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> @@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> return ret;
> }
>
> +#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
> + GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
> + GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
> +
> static irqreturn_t gen8_irq_handler(int irq, void *arg)
> {
> - struct drm_device *dev = arg;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - u32 master_ctl;
> - u32 gt_iir[4] = {};
> - irqreturn_t ret;
> + struct drm_i915_private *dev_priv = arg;
> + u32 master_ctl, gt_iir[4];
> + irqreturn_t ret = IRQ_NONE;
>
> if (!intel_irqs_enabled(dev_priv))
> return IRQ_NONE;
>
> - master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> - master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
> + master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> if (!master_ctl)
> return IRQ_NONE;
>
> I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
>
> - /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> - disable_rpm_wakeref_asserts(dev_priv);
> -
> /* Find, clear, then process each source of interrupt */
> - ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> - gen8_gt_irq_handler(dev_priv, gt_iir);
> - ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> + if (master_ctl & GEN8_GT_IRQ_BITS)
> + ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> +
> + if (master_ctl & ~GEN8_GT_IRQ_BITS) {
> + disable_rpm_wakeref_asserts(dev_priv);
> + ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> + enable_rpm_wakeref_asserts(dev_priv);
> + }
>
> I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> - POSTING_READ_FW(GEN8_MASTER_IRQ);
>
> - enable_rpm_wakeref_asserts(dev_priv);
> + if (master_ctl & GEN8_GT_IRQ_BITS)
> + gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>
> return ret;
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler
2017-09-14 7:20 ` [PATCH 1/3] " Pandiyan, Dhinakaran
@ 2017-09-14 8:37 ` Chris Wilson
2017-09-14 19:46 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-09-14 8:37 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx@lists.freedesktop.org
Quoting Pandiyan, Dhinakaran (2017-09-14 08:20:21)
>
> On Wed, 2017-09-13 at 19:18 +0100, Chris Wilson wrote:
> > The goal here is to trim an excess posting read and keep the predicates
>
> Curious why we do the posting reads, is that a hardware requirement?
In most cases, no. In very few cases, we do need a delay in order to be
sure the hw has processed the write before we continue (e.g. when
enabling interrupts). But really it all started as a bad idea to try and
move the regs over to WC with manual flushing, and cargo-culted since
then. The challenge is that now those delays are in place, removing them
requires care.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler
2017-09-14 8:37 ` Chris Wilson
@ 2017-09-14 19:46 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 10+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-14 19:46 UTC (permalink / raw)
To: chris@chris-wilson.co.uk; +Cc: intel-gfx@lists.freedesktop.org
On Thu, 2017-09-14 at 09:37 +0100, Chris Wilson wrote:
> Quoting Pandiyan, Dhinakaran (2017-09-14 08:20:21)
> >
> > On Wed, 2017-09-13 at 19:18 +0100, Chris Wilson wrote:
> > > The goal here is to trim an excess posting read and keep the predicates
> >
> > Curious why we do the posting reads, is that a hardware requirement?
>
> In most cases, no. In very few cases, we do need a delay in order to be
> sure the hw has processed the write before we continue (e.g. when
> enabling interrupts). But really it all started as a bad idea to try and
> move the regs over to WC with manual flushing, and cargo-culted since
> then.
That explains the writes followed by a read in this file! Thanks for the
interesting piece of background.
> The challenge is that now those delays are in place, removing them
> requires care.
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler
2017-09-13 18:18 [PATCH 1/3] drm/i915: Trim gen8_irq_handler Chris Wilson
` (3 preceding siblings ...)
2017-09-14 7:20 ` [PATCH 1/3] " Pandiyan, Dhinakaran
@ 2017-09-14 15:46 ` Ville Syrjälä
2017-09-14 15:54 ` Chris Wilson
4 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2017-09-14 15:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Wed, Sep 13, 2017 at 07:18:44PM +0100, Chris Wilson wrote:
> The goal here is to trim an excess posting read and keep the predicates
> tight (reusing the same predicate throughout for GT ack/handling).
>
> add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
> function old new delta
> gen8_gt_irq_handler 282 301 +19
> cherryview_irq_handler 450 457 +7
> gen8_irq_handler 1653 1623 -30
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5dbf2da..e12321cb7403 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
> }
>
> static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> - u32 gt_iir[4])
> + u32 master_ctl, u32 gt_iir[4])
> {
> - if (gt_iir[0]) {
> + if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
> gen8_cs_irq_handler(dev_priv->engine[RCS],
> gt_iir[0], GEN8_RCS_IRQ_SHIFT);
> gen8_cs_irq_handler(dev_priv->engine[BCS],
> gt_iir[0], GEN8_BCS_IRQ_SHIFT);
> }
>
> - if (gt_iir[1]) {
> + if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
> gen8_cs_irq_handler(dev_priv->engine[VCS],
> gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
> gen8_cs_irq_handler(dev_priv->engine[VCS2],
> gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
> }
>
> - if (gt_iir[3])
> + if (master_ctl & GEN8_GT_VECS_IRQ) {
> gen8_cs_irq_handler(dev_priv->engine[VECS],
> gt_iir[3], GEN8_VECS_IRQ_SHIFT);
> + }
>
> - if (gt_iir[2] & dev_priv->pm_rps_events)
> - gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> + if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> + if (gt_iir[2] & dev_priv->pm_rps_events)
> + gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>
> - if (gt_iir[2] & dev_priv->pm_guc_events)
> - gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> + if (gt_iir[2] & dev_priv->pm_guc_events)
> + gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> + }
> }
>
> static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> POSTING_READ(GEN8_MASTER_IRQ);
>
> - gen8_gt_irq_handler(dev_priv, gt_iir);
> + gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>
> if (hotplug_status)
> i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> @@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> return ret;
> }
>
> +#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
> + GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
> + GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
> +
> static irqreturn_t gen8_irq_handler(int irq, void *arg)
> {
> - struct drm_device *dev = arg;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - u32 master_ctl;
> - u32 gt_iir[4] = {};
> - irqreturn_t ret;
> + struct drm_i915_private *dev_priv = arg;
> + u32 master_ctl, gt_iir[4];
> + irqreturn_t ret = IRQ_NONE;
>
> if (!intel_irqs_enabled(dev_priv))
> return IRQ_NONE;
>
> - master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> - master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
> + master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> if (!master_ctl)
> return IRQ_NONE;
>
> I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
>
> - /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> - disable_rpm_wakeref_asserts(dev_priv);
> -
> /* Find, clear, then process each source of interrupt */
> - ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> - gen8_gt_irq_handler(dev_priv, gt_iir);
> - ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> + if (master_ctl & GEN8_GT_IRQ_BITS)
> + ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> +
> + if (master_ctl & ~GEN8_GT_IRQ_BITS) {
> + disable_rpm_wakeref_asserts(dev_priv);
Hmm. Why is this needed for DE interrupts but not GT interrupts? Just
the _FW() vs. not in the codepaths? If I'm reading things right we still
have some non _FW() accesses in the RPS handler at least.
BDW+ doesn't suffer from the "hang when accessing the same cacheline from
multiple cpus" issue anymore?
> + ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> + enable_rpm_wakeref_asserts(dev_priv);
> + }
This thing reminds me that I'd still like to split the DE stuff into
ack/handle stuff as well.
>
> I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> - POSTING_READ_FW(GEN8_MASTER_IRQ);
>
> - enable_rpm_wakeref_asserts(dev_priv);
> + if (master_ctl & GEN8_GT_IRQ_BITS)
> + gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>
> return ret;
> }
> --
> 2.14.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler
2017-09-14 15:46 ` Ville Syrjälä
@ 2017-09-14 15:54 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2017-09-14 15:54 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Quoting Ville Syrjälä (2017-09-14 16:46:10)
> On Wed, Sep 13, 2017 at 07:18:44PM +0100, Chris Wilson wrote:
> > The goal here is to trim an excess posting read and keep the predicates
> > tight (reusing the same predicate throughout for GT ack/handling).
> >
> > add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
> > function old new delta
> > gen8_gt_irq_handler 282 301 +19
> > cherryview_irq_handler 450 457 +7
> > gen8_irq_handler 1653 1623 -30
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
> > 1 file changed, 30 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 91a2c5dbf2da..e12321cb7403 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
> > }
> >
> > static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> > - u32 gt_iir[4])
> > + u32 master_ctl, u32 gt_iir[4])
> > {
> > - if (gt_iir[0]) {
> > + if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
> > gen8_cs_irq_handler(dev_priv->engine[RCS],
> > gt_iir[0], GEN8_RCS_IRQ_SHIFT);
> > gen8_cs_irq_handler(dev_priv->engine[BCS],
> > gt_iir[0], GEN8_BCS_IRQ_SHIFT);
> > }
> >
> > - if (gt_iir[1]) {
> > + if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
> > gen8_cs_irq_handler(dev_priv->engine[VCS],
> > gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
> > gen8_cs_irq_handler(dev_priv->engine[VCS2],
> > gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
> > }
> >
> > - if (gt_iir[3])
> > + if (master_ctl & GEN8_GT_VECS_IRQ) {
> > gen8_cs_irq_handler(dev_priv->engine[VECS],
> > gt_iir[3], GEN8_VECS_IRQ_SHIFT);
> > + }
> >
> > - if (gt_iir[2] & dev_priv->pm_rps_events)
> > - gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> > + if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> > + if (gt_iir[2] & dev_priv->pm_rps_events)
> > + gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> >
> > - if (gt_iir[2] & dev_priv->pm_guc_events)
> > - gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> > + if (gt_iir[2] & dev_priv->pm_guc_events)
> > + gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> > + }
> > }
> >
> > static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> > @@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> > I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> > POSTING_READ(GEN8_MASTER_IRQ);
> >
> > - gen8_gt_irq_handler(dev_priv, gt_iir);
> > + gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
> >
> > if (hotplug_status)
> > i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> > @@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> > return ret;
> > }
> >
> > +#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
> > + GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
> > + GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
> > +
> > static irqreturn_t gen8_irq_handler(int irq, void *arg)
> > {
> > - struct drm_device *dev = arg;
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > - u32 master_ctl;
> > - u32 gt_iir[4] = {};
> > - irqreturn_t ret;
> > + struct drm_i915_private *dev_priv = arg;
> > + u32 master_ctl, gt_iir[4];
> > + irqreturn_t ret = IRQ_NONE;
> >
> > if (!intel_irqs_enabled(dev_priv))
> > return IRQ_NONE;
> >
> > - master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> > - master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
> > + master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> > if (!master_ctl)
> > return IRQ_NONE;
> >
> > I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
> >
> > - /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> > - disable_rpm_wakeref_asserts(dev_priv);
> > -
> > /* Find, clear, then process each source of interrupt */
> > - ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> > - gen8_gt_irq_handler(dev_priv, gt_iir);
> > - ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> > + if (master_ctl & GEN8_GT_IRQ_BITS)
> > + ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> > +
> > + if (master_ctl & ~GEN8_GT_IRQ_BITS) {
> > + disable_rpm_wakeref_asserts(dev_priv);
>
> Hmm. Why is this needed for DE interrupts but not GT interrupts? Just
> the _FW() vs. not in the codepaths? If I'm reading things right we still
> have some non _FW() accesses in the RPS handler at least.
The GT interrupts are covered by a wakeref. I left it for DE as I
haven't verified that all should be ok. The atomic in there isn't as
expensive as the mmio, but it certainly isn't free either.
> BDW+ doesn't suffer from the "hang when accessing the same cacheline from
> multiple cpus" issue anymore?
No, that was a gen7 bug.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread