Re: [PATCH 00/15] Unify interrupt register init/reset

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 23, 2013 at 07:33:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@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.

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?

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.

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@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