From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events Date: Wed, 5 Feb 2014 17:11:45 +0200 Message-ID: <20140205151145.GL3891@intel.com> References: <1391542551-20239-1-git-send-email-imre.deak@intel.com> <1391542551-20239-8-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 7FDAE106EFE for ; Wed, 5 Feb 2014 07:11:52 -0800 (PST) Content-Disposition: inline In-Reply-To: <1391542551-20239-8-git-send-email-imre.deak@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Imre Deak Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Feb 04, 2014 at 09:35:51PM +0200, Imre Deak wrote: > Atm we call the handlers for pending pipestat interrupt events even if > they aren't explicitly enabled by i915_enable_pipestat(). This isn't an > issue for events other than the vblank start event, since those are > always enabled anyways. Otoh, we enable the vblank start event > on-demand, so we'll end up calling the vblank handler at times when they > are disabled. > = > I haven't checked if this causes any real problem, but for consistency > and to remove some overhead we should still fix this by clearing / > handling only the enabled interrupt events. Also this is a dependency > for the upcoming VLV power domain patchset where we need to disable all > the pipestat interrupts whenever the display power well is off. > = > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 33 ++++++++++++++++++++++++++++++--- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > 3 files changed, 35 insertions(+), 3 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 43f37ca..faca5b4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1427,6 +1427,7 @@ typedef struct drm_i915_private { > }; > u32 gt_irq_mask; > u32 pm_irq_mask; > + u32 pipestat_irq_mask[I915_MAX_PIPES]; > = > struct work_struct hotplug_work; > bool enable_hotplug_processing; > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_= irq.c > index eea5398..2cac477 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -419,6 +419,16 @@ done: > return ret; > } > = > +static bool __cpu_fifo_underrun_reporting_enabled(struct drm_device *dev, > + enum pipe pipe) > +{ > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct drm_crtc *crtc =3D dev_priv->pipe_to_crtc_mapping[pipe]; > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + > + return !intel_crtc->cpu_fifo_underrun_disabled; > +} > + > /** > * intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun = messages > * @dev: drm device > @@ -520,6 +530,8 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, en= um pipe pipe, > if ((pipestat & enable_mask) =3D=3D enable_mask) > return; > = > + dev_priv->pipestat_irq_mask[pipe] |=3D status_mask; > + > /* Enable the interrupt, clear any pending status */ > pipestat |=3D enable_mask | status_mask; > I915_WRITE(reg, pipestat); > @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, e= num pipe pipe, > if ((pipestat & enable_mask) =3D=3D 0) > return; > = > + dev_priv->pipestat_irq_mask[pipe] &=3D ~status_mask; > + > pipestat &=3D ~enable_mask; > I915_WRITE(reg, pipestat); > POSTING_READ(reg); > @@ -1530,18 +1544,31 @@ static void gen6_rps_irq_handler(struct drm_i915_= private *dev_priv, u32 pm_iir) > static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 = iir) > { > drm_i915_private_t *dev_priv =3D dev->dev_private; > - u32 pipe_stats[I915_MAX_PIPES]; > + u32 pipe_stats[I915_MAX_PIPES] =3D { }; > int pipe; > = > spin_lock(&dev_priv->irq_lock); > for_each_pipe(pipe) { > - int reg =3D PIPESTAT(pipe); > + int reg; > + u32 mask; > + > + if (!dev_priv->pipestat_irq_mask[pipe] && > + !__cpu_fifo_underrun_reporting_enabled(dev, pipe)) > + continue; > + > + reg =3D PIPESTAT(pipe); > pipe_stats[pipe] =3D I915_READ(reg); > = > /* > * Clear the PIPE*STAT regs before the IIR > */ > - if (pipe_stats[pipe] & 0x8000ffff) > + mask =3D PIPESTAT_INT_ENABLE_MASK | PIPE_FIFO_UNDERRUN_STATUS; Maybe we should add PIPE_FIFO_UNDERRUN_STATUS to the mask only when the underrun reporting is enabled. If someone goes and enables underrun reporting just after valleyview_pipestat_irq_handler(), we'd potentially report a stale underrun. You get to blame me for that one though, since the bug is already there in the current code, which I implemented. > + if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe)) > + mask |=3D dev_priv->pipestat_irq_mask[pipe]; > + pipe_stats[pipe] &=3D mask; > + > + if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | > + PIPESTAT_INT_STATUS_MASK)) > I915_WRITE(reg, pipe_stats[pipe]); > } > spin_unlock(&dev_priv->irq_lock); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_= reg.h > index c998c4f..47e0635 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -997,6 +997,10 @@ > #define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT (1<<6) > #define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT (1<<5) > #define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT (1<<4) > +#define I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe) \ > + ((pipe) =3D=3D PIPE_A ? I915_DISPLAY_PIPE_A_EVENT_INTERRUPT : \ > + I915_DISPLAY_PIPE_B_EVENT_INTERRUPT) > + > #define I915_DEBUG_INTERRUPT (1<<2) > #define I915_USER_INTERRUPT (1<<1) > #define I915_ASLE_INTERRUPT (1<<0) > -- = > 1.8.4 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC