From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 00/20] ILK+ interrupt improvements, v2 Date: Tue, 18 Mar 2014 13:53:53 -0700 Message-ID: <20140318205353.GA29729@bwidawsk.net> References: <1394233836-3827-1-git-send-email-przanoni@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 4D8916E5AD for ; Tue, 18 Mar 2014 13:54:07 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1394233836-3827-1-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > 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 (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 8: I'd really like to drop this one. 9-10: Reviewed-by: Ben Widawsky 12-13: I wouldn't mind cpt_irq_* rename, but either way Reviewed-by: Ben Widawsky 14: With the requested change in the mail: Reviewed-by: Ben Widawsky 16: Reviewed-by: Ben Widawsky 20: Should be squashed, but Reviewed-by: Ben Widawsky 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 Widawsky, Intel Open Source Technology Center