From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+ Date: Wed, 19 Mar 2014 10:50:08 -0700 Message-ID: <20140319175007.GD26156@bwidawsk.net> References: <1394233836-3827-1-git-send-email-przanoni@gmail.com> <1394233836-3827-9-git-send-email-przanoni@gmail.com> <20140318182009.GB28915@bwidawsk.net> <20140319082832.GK30571@phenom.ffwll.local> 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 BFFE86E1F3 for ; Wed, 19 Mar 2014 10:50:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140319082832.GK30571@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Wed, Mar 19, 2014 at 09:28:32AM +0100, Daniel Vetter wrote: > On Tue, Mar 18, 2014 at 11:20:09AM -0700, Ben Widawsky wrote: > > On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote: > > > From: Paulo Zanoni > > > > > > Instead of trying to clear it again. It should already be masked and > > > disabled and zeroed at preinstall/uninstall. > > > > > > Signed-off-by: Paulo Zanoni > > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++----------------- > > > 1 file changed, 15 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index 6d4daf2..4d0a8b1 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ > > > I915_WRITE(type##IIR, 0xffffffff); \ > > > } while (0) > > > > > > +/* > > > + * We should clear IMR at preinstall/uninstall, and just check at postinstall. > > > + */ > > > +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \ > > > + u32 val = I915_READ(reg); \ > > > + if (val) \ > > > + DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \ > > > + (reg), val); \ > > > +} while (0) > > > + > > > #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \ > > > + GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \ > > > I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \ > > > I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \ > > > } while (0) > > > > > > #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \ > > > + GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \ > > > I915_WRITE(type##IMR, (imr_val)); \ > > > I915_WRITE(type##IER, (ier_val)); \ > > > } while (0) > > > > Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ > > which gives an error that we can do nothing about other than clear it > > anyway. > > > > I'd be in favor of dropping this patch. > > The point of the assert is to make sure that the new IIR clearing logic > with blocking everything+clearing in the preinstall hook actually does > what it's supposed to do. > > Since the point of this exercise is to reuse this code for runtime > suspend/resume where races are much easier to hit I think this is a good > self-check of the code. > -Daniel > Okay, I am feeling somewhat pressured to stick a reviewed-by on this since Daniel likes it. Change the macro to WARN instead of DRM_ERROR, and, clear the IIR if it's non-zero. With that change, it's: Reviewed-by-with-reservations: Ben Widawsky -- Ben Widawsky, Intel Open Source Technology Center