Re: [Intel-gfx] [PATCH 2/2] drm/i915: Prune gen8_gt_irq_handler

2018-02-15 Thread Mika Kuoppala 
Chris Wilson  writes:

> Quoting Chris Wilson (2018-02-15 07:37:13)
>> 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])
>> -   I915_WRITE_FW(GEN8_GT_IIR(0), gt_iir[0]);
>> +   gt_iir[0] = readl(regs + 
>> i915_mmio_reg_offset(GEN8_GT_IIR(0)));
>> +   if (likely(gt_iir[0]))
>> +   writel(gt_iir[0], regs + 
>> i915_mmio_reg_offset(GEN8_GT_IIR(0)));
>
> What do we feel about
>
> #define raw_reg_read32(mmio, reg) readl(mmio + i915_mmio_reg_offset(reg))
> #define raw_reg_write32(mmio, reg, value) writel(value, mmio + 
> i915_mmio_reg_offset(reg))

I am in favour. Shorter versions would be even more pleasing.
reg_read and reg_write.
-Mika

>
> for transitioning away from the implicit argument in I915_READ?
> -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

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Prune gen8_gt_irq_handler

2018-02-15 Thread Chris Wilson 
Quoting Chris Wilson (2018-02-15 07:37:13)
> 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])
> -   I915_WRITE_FW(GEN8_GT_IIR(0), gt_iir[0]);
> +   gt_iir[0] = readl(regs + 
> i915_mmio_reg_offset(GEN8_GT_IIR(0)));
> +   if (likely(gt_iir[0]))
> +   writel(gt_iir[0], regs + 
> i915_mmio_reg_offset(GEN8_GT_IIR(0)));

What do we feel about

#define raw_reg_read32(mmio, reg) readl(mmio + i915_mmio_reg_offset(reg))
#define raw_reg_write32(mmio, reg, value) writel(value, mmio + 
i915_mmio_reg_offset(reg))

for transitioning away from the implicit argument in I915_READ?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Prune gen8_gt_irq_handler

2018-02-14 Thread Chris Wilson 
The compiler is not automatically caching the i915->regs address inside
a register and emitting a load for every mmio access. For simple
functions like gen8_gt_irq_handler that are already using the raw
accessors, we can open-code them for substantial savings:

add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-83 (-83)
Function old new   delta
gen8_gt_irq_handler  290 266 -24
gen8_gt_irq_ack  181 122 -59
Total: Before=954637, After=954554, chg -0.01%

Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
---
And so begins the long haul of de-I915_READ/WRITE-ing the driver...
---
 drivers/gpu/drm/i915/i915_irq.c | 57 +++--
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b7b377ba7b6e..10d62b33cb13 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1413,9 +1413,11 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir, int test_shift)
tasklet_hi_schedule(&execlists->tasklet);
 }
 
-static void gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
+static void gen8_gt_irq_ack(struct drm_i915_private *i915,
u32 master_ctl, u32 gt_iir[4])
 {
+   void __iomem * const regs = i915->regs;
+
 #define GEN8_GT_IRQS (GEN8_GT_RCS_IRQ | \
  GEN8_GT_BCS_IRQ | \
  GEN8_GT_VCS1_IRQ | \
@@ -1425,62 +1427,57 @@ static void gen8_gt_irq_ack(struct drm_i915_private 
*dev_priv,
  GEN8_GT_GUC_IRQ)
 
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])
-   I915_WRITE_FW(GEN8_GT_IIR(0), gt_iir[0]);
+   gt_iir[0] = readl(regs + i915_mmio_reg_offset(GEN8_GT_IIR(0)));
+   if (likely(gt_iir[0]))
+   writel(gt_iir[0], regs + 
i915_mmio_reg_offset(GEN8_GT_IIR(0)));
}
 
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])
-   I915_WRITE_FW(GEN8_GT_IIR(1), gt_iir[1]);
+   gt_iir[1] = readl(regs + i915_mmio_reg_offset(GEN8_GT_IIR(1)));
+   if (likely(gt_iir[1]))
+   writel(gt_iir[1], regs + 
i915_mmio_reg_offset(GEN8_GT_IIR(1)));
}
 
-   if (master_ctl & GEN8_GT_VECS_IRQ) {
-   gt_iir[3] = I915_READ_FW(GEN8_GT_IIR(3));
-   if (gt_iir[3])
-   I915_WRITE_FW(GEN8_GT_IIR(3), gt_iir[3]);
+   if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
+   gt_iir[2] = readl(regs + i915_mmio_reg_offset(GEN8_GT_IIR(2)));
+   if (likely(gt_iir[2] & (i915->pm_rps_events | 
i915->pm_guc_events)))
+   writel(gt_iir[2] & (i915->pm_rps_events |
+   i915->pm_guc_events),
+  regs + i915_mmio_reg_offset(GEN8_GT_IIR(2)));
}
 
-   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)) {
-   I915_WRITE_FW(GEN8_GT_IIR(2),
- gt_iir[2] & (dev_priv->pm_rps_events |
-  dev_priv->pm_guc_events));
-   }
+   if (master_ctl & GEN8_GT_VECS_IRQ) {
+   gt_iir[3] = readl(regs + i915_mmio_reg_offset(GEN8_GT_IIR(3)));
+   if (likely(gt_iir[3]))
+   writel(gt_iir[3], regs + 
i915_mmio_reg_offset(GEN8_GT_IIR(3)));
}
 }
 
-static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
+static void gen8_gt_irq_handler(struct drm_i915_private *i915,
u32 master_ctl, u32 gt_iir[4])
 {
if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
-   gen8_cs_irq_handler(dev_priv->engine[RCS],
+   gen8_cs_irq_handler(i915->engine[RCS],
gt_iir[0], GEN8_RCS_IRQ_SHIFT);
-   gen8_cs_irq_handler(dev_priv->engine[BCS],
+   gen8_cs_irq_handler(i915->engine[BCS],
gt_iir[0], GEN8_BCS_IRQ_SHIFT);
}
 
if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
-   gen8_cs_irq_handler(dev_priv->engine[VCS],
+   gen8_cs_irq_handler(i915->engine[VCS],
gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
-   gen8_cs_irq_handler(dev_priv->engine[VCS2],
+   gen8_cs_irq_handler(i915->engine[VCS2],
gt_iir[1], GEN8_VCS2_IRQ_S