On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote: > On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > > > 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@xxxxxxxxxxxx> (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@xxxxxxxxxxxx> > > 8: I'd really like to drop this one. Comment on this and I think with a pimped commit message this is good to go imo. I really think the added self-checks are required to start using this code for runtime pm. > 9-10: Reviewed-by: Ben Widawsky <ben@xxxxxxxxxxxx> > > 12-13: I wouldn't mind cpt_irq_* rename, but either way > Reviewed-by: Ben Widawsky <ben@xxxxxxxxxxxx> > > 14: With the requested change in the mail: > Reviewed-by: Ben Widawsky <ben@xxxxxxxxxxxx> > > 16: Reviewed-by: Ben Widawsky <ben@xxxxxxxxxxxx> > > 20: Should be squashed, but > Reviewed-by: Ben Widawsky <ben@xxxxxxxxxxxx> > > 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@xxxxxxxxxxxx> Paulo's using "reset" functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain "disable" more misleading. I think you raise some good points in your review, and besides the 3 cases I commented on I lack the detailed knowledge to avoid looking like a fool ;-) So I think I'll wait for Paulo's comments before pulling this all in. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
