[Intel-gfx] [PATCH 00/15] Unify interrupt register init/reset

[Paulo Zanoni]

2013/7/24 Daniel Vetter <daniel at ffwll.ch>:
> On Tue, Jul 23, 2013 at 07:33:40PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> Hi
>>
>> This patch series is based on emails I sent a few days ago, with subject
>> "proposals/questions about the IRQ registers". The biggest goal of the series is
>> to have an unified way to initialize/reset IRQ regsiters. After this series,
>> when you add new interrupts to our driver you won't need to think about doing
>> the correct reads, writes and posting_reads, all you'll need to worry about is
>> to call the macros everyone else calls. So now either all the registers are
>> initialized correctly or all the registers are initialized wrongly.
>>
>> The first 10 patches do all the rework on i915+. The changes should be
>> incremental and any non-trivial changes are on separate patches, so bisecting
>> regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and
>> 10 touch VLV-specific code, and I don't have a way to test this code.
>>
>> On my intial patches the i8xx code was changed along with the i915+ code, but
>> Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx
>> work.  I don't know if we really want these patches, but they're here anyway, so
>> feel free to give them a NACK, I won't complain. In case we decide to merge the
>> patches, git bisect should easily spot regressions.
>>
>> Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours
>> of usage) and Atom (only booted and then suspend/resumed once).
>
> My first question was why you didn't use name##IMR, name##IER, name##IIR
> in the macros, since that would match what we have in -internal and imo
> would look more natural. But then I've noticed that for IIR, IER & IMR we
> don't have a prefix ... I still think the I in the name looks a bit funny
> though (e.g. GTI). Maybe we should add an I915_ prefix then we could
> remove the I.

Yeah, you guessed the reason. If we use name##IMR then we could
replace "INTEL_IRQ_REG_INIT(I, false, enable_mask,
dev_priv->irq_mask);" with "INTEL_IRQ_REG_INIT(, false, enable_mask,
dev_priv->irq_mask);", but I guess your suggestion of renaming is
probably better. The problem with renaming to I915_IIR is that the
registers also exists on i8xx, so I'd vote for another name
(GEN2_IIR?).


>
> The other thing I'm unclear about is the do_iir flag, which in the end
> seems to clear IIR bits in uninstall but not in preinstall. Iirc Ben's
> proposal was to do the double IIR clear in preinstall, and I tend to agree
> with him. Can you please elaborate on this?

Please look at the final result of the series, we remove the flag on
later patches (you just made me notice I forgot to remove it on the
reg_init functions, but we don't really use it anymore). At the end of
the series we do the double-IIR clear at both preinstall and
uninstall. I wanted to keep one functional change at each patch.

>
> Maybe one patch on top to add some kerneldoc to the new magic macros would
> be good so that we can explain once what the thinking between the sequence
> is.

Will do.

Thanks Daniel and Chris for the fast reviews!

>
> Cheers, Daniel
>>
>
>> Thanks,
>> Paulo
>>
>> Paulo Zanoni (15):
>>   drm/i915: add INTEL_IRQ_REG_RESET
>>   drm/i915: change how VLV_IIR is reset
>>   drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET
>>   drm/i915: really clear the IIR registers
>>   drm/i915: add INTEL_IRQ_REG_INIT
>>   drm/i915: use INTEL_IRQ_REG_INIT on VLV too
>>   drm/i915: reset the IIR registers at preinstall
>>   drm/i915: WARN if IIR is not zero at irq_postinstall
>>   drm/i915: remove additional zerogin of VLV_IIR at postinstall
>>   drm/i915: remove extra clearing of GTIIR from VLV irq preinstall
>>   drm/i915: add INTEL_IRQ_REG_RESET16
>>   drm/i915: really clear the IIR registers on i8xx
>>   drm/i915: add INTEL_IRQ_REG_INIT16
>>   drm/i915: reset the i8xx IIR registers at preinstall
>>   drm/i915: WARN if IIR is not zero at i8xx irq_postinstall
>>
>>  drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++-----------------------
>>  1 file changed, 62 insertions(+), 82 deletions(-)
>>
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni