Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
> was sent to the mailing list on January 22. There are no real differences,
> except for the last patch, which is new.
> 
> Original cover letter:
> http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
> 
> The idea behind this series is that at some point our runtime PM code will just
> call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
> using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
> to our code before we do that change. We gotta be in shape if we want to be
> exposed to runtime!
> 
> Thanks,
> Paulo
> 
> Paulo Zanoni (20):
>   drm/i915: add GEN5_IRQ_INIT macro
>   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
>   drm/i915: use GEN8_IRQ_INIT on GEN5
>   drm/i915: add GEN5_IRQ_FINI
>   drm/i915: don't forget to uninstall the PM IRQs
>   drm/i915: properly clear IIR at irq_uninstall on Gen5+
>   drm/i915: add GEN5_IRQ_INIT
>   drm/i915: check if IIR is still zero at postinstall on Gen5+
>   drm/i915: fix SERR_INT init/reset code
>   drm/i915: fix GEN7_ERR_INT init/reset code
>   drm/i915: fix open coded gen5_gt_irq_preinstall
>   drm/i915: extract ibx_irq_uninstall
>   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
>   drm/i915: enable SDEIER later
>   drm/i915: remove ibx_irq_uninstall
>   drm/i915: add missing intel_hpd_irq_uninstall
>   drm/i915: add ironlake_irq_reset
>   drm/i915: add gen8_irq_reset
>   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
>   drm/i915: add POSTING_READs to the IRQ init/reset macros
> 
>  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
>  1 file changed, 121 insertions(+), 149 deletions(-)
> 

Okay, here is the summary of my review. At first I was complaining to
myself about how many patches you used to do a simple thing. But, I must
admit it made reviewing the thing a lot easier, and when I look back at
how much stuff you combined, I'm really glad you did it this way. I'm
sure I've missed something silly though, since every patch looks so
similar :P

1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
improvement on #3)

7: I don't like. Can we drop? I guess doing this would make a decent
amount of churn, so if you don't want to drop it, that's fine, and it's
functionally correct:
     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

8: I'd really like to drop this one.

9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

12-13: I wouldn't mind cpt_irq_* rename, but either way
       Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

14: With the requested change in the mail:
    Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

20: Should be squashed, but
    Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
seems to always mean "disable." I think disable makes the code so much
clearer, and would really love if you can apply this simple rename. With
the rename, they're:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center