From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: Fix PIPESTATE irq ack on i965/g4x Date: Thu, 14 Jun 2018 21:16:45 +0300 Message-ID: <20180614181645.GK20518@intel.com> References: <20180611200258.27121-1-ville.syrjala@linux.intel.com> <152875043279.11830.11559744773813729035@mail.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <152875043279.11830.11559744773813729035@mail.alporthouse.com> Sender: stable-owner@vger.kernel.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Jun 11, 2018 at 09:53:52PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-06-11 21:02:55) > > From: Ville Syrjälä > > > > On i965/g4x IIR is edge triggered. So in order for IIR to notice that > > there is still a pending interrupt we have to force and edge in ISR. > > For the ISR/IIR pipe event bits we can do that by temporarily > > clearing all the PIPESTAT enable bits when we ack the status bits. > > This will force the ISR pipe event bit low, and it can then go back > > high when we restore the PIPESTAT enable bits. > > > > This avoids the following race: > > 1. stat = read(PIPESTAT) > > 2. an enabled PIPESTAT status bit goes high > > 3. write(PIPESTAT, enable|stat); > > 4. write(IIR, PIPE_EVENT) > > > > The end result is IIR==0 and ISR!=0. This can lead to nasty > > vblank wait/flip_done timeouts if another interrupt source > > doesn't trick us into looking at the PIPESTAT status bits despite > > the IIR PIPE_EVENT bit being low. > > > > Before i965 IIR was level triggered so this problem can't actually > > happen there. And curiously VLV/CHV went back to the level triggered > > scheme as well. But for simplicity we'll use the same i965/g4x > > compatible code for all platforms. > > > > Cc: stable@vger.kernel.org > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106033 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105225 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106030 > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 2fd92a886789..364e1c85315e 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -1893,9 +1893,17 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv, > > > > /* > > * Clear the PIPE*STAT regs before the IIR > > + * > > + * Toggle the enable bits to make sure we get an > > + * edge in the ISR pipe event bit if we don't clear > > + * all the enabled status bits. Otherwise the edge > > + * triggered IIR on i965/g4x wouldn't notice that > > + * an interrupt is still pending. > > */ > > - if (pipe_stats[pipe]) > > - I915_WRITE(reg, enable_mask | pipe_stats[pipe]); > > + if (pipe_stats[pipe]) { > > + I915_WRITE(reg, pipe_stats[pipe]); > > Ack status, disable all. > > > + I915_WRITE(reg, enable_mask); > > Enable, an asserted bit should trigger a new edge. > > It certainly does as you explain, now I just hope the hw feels the same! > > Reviewed-by: Chris Wilson s/PIPESTATE/PIPESTAT/ (can't count how many times I've done that one wrong) and pushed to dinq. Thanks for the review. -- Ville Syrjälä Intel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com ([134.134.136.24]:11544 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754713AbeFNSQs (ORCPT ); Thu, 14 Jun 2018 14:16:48 -0400 Date: Thu, 14 Jun 2018 21:16:45 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: Fix PIPESTATE irq ack on i965/g4x Message-ID: <20180614181645.GK20518@intel.com> References: <20180611200258.27121-1-ville.syrjala@linux.intel.com> <152875043279.11830.11559744773813729035@mail.alporthouse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <152875043279.11830.11559744773813729035@mail.alporthouse.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Jun 11, 2018 at 09:53:52PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-06-11 21:02:55) > > From: Ville Syrj�l� > > > > On i965/g4x IIR is edge triggered. So in order for IIR to notice that > > there is still a pending interrupt we have to force and edge in ISR. > > For the ISR/IIR pipe event bits we can do that by temporarily > > clearing all the PIPESTAT enable bits when we ack the status bits. > > This will force the ISR pipe event bit low, and it can then go back > > high when we restore the PIPESTAT enable bits. > > > > This avoids the following race: > > 1. stat = read(PIPESTAT) > > 2. an enabled PIPESTAT status bit goes high > > 3. write(PIPESTAT, enable|stat); > > 4. write(IIR, PIPE_EVENT) > > > > The end result is IIR==0 and ISR!=0. This can lead to nasty > > vblank wait/flip_done timeouts if another interrupt source > > doesn't trick us into looking at the PIPESTAT status bits despite > > the IIR PIPE_EVENT bit being low. > > > > Before i965 IIR was level triggered so this problem can't actually > > happen there. And curiously VLV/CHV went back to the level triggered > > scheme as well. But for simplicity we'll use the same i965/g4x > > compatible code for all platforms. > > > > Cc: stable@vger.kernel.org > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106033 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105225 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106030 > > Signed-off-by: Ville Syrj�l� > > --- > > drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 2fd92a886789..364e1c85315e 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -1893,9 +1893,17 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv, > > > > /* > > * Clear the PIPE*STAT regs before the IIR > > + * > > + * Toggle the enable bits to make sure we get an > > + * edge in the ISR pipe event bit if we don't clear > > + * all the enabled status bits. Otherwise the edge > > + * triggered IIR on i965/g4x wouldn't notice that > > + * an interrupt is still pending. > > */ > > - if (pipe_stats[pipe]) > > - I915_WRITE(reg, enable_mask | pipe_stats[pipe]); > > + if (pipe_stats[pipe]) { > > + I915_WRITE(reg, pipe_stats[pipe]); > > Ack status, disable all. > > > + I915_WRITE(reg, enable_mask); > > Enable, an asserted bit should trigger a new edge. > > It certainly does as you explain, now I just hope the hw feels the same! > > Reviewed-by: Chris Wilson s/PIPESTATE/PIPESTAT/ (can't count how many times I've done that one wrong) and pushed to dinq. Thanks for the review. -- Ville Syrj�l� Intel