[Intel-gfx] [PATCH 03/20] drm/i915: use GEN8_IRQ_INIT on GEN5

[Paulo Zanoni]

2014-03-18 14:11 GMT-03:00 Ben Widawsky <ben at bwidawsk.net>:
> On Fri, Mar 07, 2014 at 08:10:19PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> And rename is to GEN5_IRQ_INIT.
>>
>> We have discussed doing equivalent changes on July 2013, and I even
>> sent a patch series for this: "[PATCH 00/15] Unify interrupt register
>> init/reset". Now that the BDW code was merged, I have one more
>> argument in favor of these changes.
>>
>> Here's what really changes with the Gen 5 IRQ init code:
>>   - We now clear the IIR registers at preinstall (they are also
>>     cleared at postinstall, but we will change that later).
>>   - We have an additional POSTING_READ at the IMR register.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++----------------------
>>  1 file changed, 23 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 852844d..7be7da1 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>>       [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>>  };
>>
>> +/*
>> + * IIR can theoretically queue up two events. Be paranoid.
>> + * Also, make sure callers of these macros have something equivalent to a
>> + * POSTING_READ on the IIR register.
>> + * */
>
> I don't understand what you mean in this comment. If you're always going
> to sending a posting read after the second IIR write, why not just put
> it in the macro?

Because if the next piece of the code is still reading registers, you
don't need it, so you can save it. This is exactly what is done by the
Gen8 code, which I copied. So the comment is telling the user to not
forget to do what you have done, for example, with the call to
POSTING_READ(GEN8_PCU_IIR) at gen8_irq_preinstall and
gen8_irq_uninstall. In other words, the comment is supposed to mean
"hey, the macros don't do the posting read, so please remember to do
them if you need". If we leave the POSTING_READs out of the macro, we
can save a few calls, just like we do on the Gen8 code.

Anyway, we have discussed this and it seems everybody prefers to have
the POSTING_READ calls on the macro, so I'll just change the patches
to do this, and then the comment will disappear.

>
> The reason it wasn't in my original macro is because we've done the
> posting read on IER, and IMR - so we're not going to get new interrupts.
> When the second IIR write lands is irrelevant. The POSTING_READ in
> between is to prevent the [probably impossible] case of the writes
> getting collapsed into one write.

I thought the reason it wasn't in your original macro was because you
always had I915_READ calls after the macro calls. And on the case you
don't have this, you have that that POSTING_READ(GEN8_PCU_IIR) which I
mentioned.

>
>> +#define GEN8_IRQ_INIT_NDX(type, which) do { \
>> +     I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
>> +     POSTING_READ(GEN8_##type##_IMR(which)); \
>> +     I915_WRITE(GEN8_##type##_IER(which), 0); \
>> +     I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
>> +     POSTING_READ(GEN8_##type##_IIR(which)); \
>> +     I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
>> +} while (0)
>> +
>>  #define GEN5_IRQ_INIT(type) do { \
>>       I915_WRITE(type##IMR, 0xffffffff); \
>> +     POSTING_READ(type##IMR); \
>>       I915_WRITE(type##IER, 0); \
>> -     POSTING_READ(type##IER); \
>> +     I915_WRITE(type##IIR, 0xffffffff); \
>> +     POSTING_READ(type##IIR); \
>> +     I915_WRITE(type##IIR, 0xffffffff); \
>>  } while (0)
>>
>> +
>>  /* For display hotplug interrupt */
>>  static void
>>  ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>> @@ -2789,6 +2807,7 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
>>       GEN5_IRQ_INIT(GT);
>>       if (INTEL_INFO(dev)->gen >= 6)
>>               GEN5_IRQ_INIT(GEN6_PM);
>> +     POSTING_READ(GTIIR);
>>  }
>>
>>  /* drm_dma.h hooks
>> @@ -2843,25 +2862,6 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>>       I915_WRITE(GEN8_MASTER_IRQ, 0);
>>       POSTING_READ(GEN8_MASTER_IRQ);
>>
>> -     /* IIR can theoretically queue up two events. Be paranoid */
>> -#define GEN8_IRQ_INIT_NDX(type, which) do { \
>> -             I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
>> -             POSTING_READ(GEN8_##type##_IMR(which)); \
>> -             I915_WRITE(GEN8_##type##_IER(which), 0); \
>> -             I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
>> -             POSTING_READ(GEN8_##type##_IIR(which)); \
>> -             I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
>> -     } while (0)
>> -
>> -#define GEN8_IRQ_INIT(type) do { \
>> -             I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \
>> -             POSTING_READ(GEN8_##type##_IMR); \
>> -             I915_WRITE(GEN8_##type##_IER, 0); \
>> -             I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
>> -             POSTING_READ(GEN8_##type##_IIR); \
>> -             I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
>> -     } while (0)
>> -
>>       GEN8_IRQ_INIT_NDX(GT, 0);
>>       GEN8_IRQ_INIT_NDX(GT, 1);
>>       GEN8_IRQ_INIT_NDX(GT, 2);
>> @@ -2871,12 +2871,9 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>>               GEN8_IRQ_INIT_NDX(DE_PIPE, pipe);
>>       }
>>
>> -     GEN8_IRQ_INIT(DE_PORT);
>> -     GEN8_IRQ_INIT(DE_MISC);
>> -     GEN8_IRQ_INIT(PCU);
>> -#undef GEN8_IRQ_INIT
>> -#undef GEN8_IRQ_INIT_NDX
>> -
>> +     GEN5_IRQ_INIT(GEN8_DE_PORT_);
>> +     GEN5_IRQ_INIT(GEN8_DE_MISC_);
>> +     GEN5_IRQ_INIT(GEN8_PCU_);
>>       POSTING_READ(GEN8_PCU_IIR);
>>
>>       ibx_irq_preinstall(dev);
>> --
>> 1.8.5.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni