From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH v3 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7) Date: Tue, 17 Jun 2014 23:27:35 +0300 Message-ID: <1403036855.32487.31.camel@intelbox> References: <1402931460-21231-1-git-send-email-oscar.mateo@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0211564579==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 0C68872006 for ; Tue, 17 Jun 2014 13:27:57 -0700 (PDT) In-Reply-To: <1402931460-21231-1-git-send-email-oscar.mateo@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: oscar.mateo@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0211564579== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-R3+LN1/qnGRTlGS5qfQ1" --=-R3+LN1/qnGRTlGS5qfQ1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2014-06-16 at 16:10 +0100, oscar.mateo@intel.com wrote: > From: Oscar Mateo >=20 > Otherwise, we might receive a new interrupt before we have time to ack th= e first > one, eventually missing it. >=20 > According to BSPec, the right order should be: >=20 > 1 - Disable Master Interrupt Control. > 2 - Find the source(s) of the interrupt. > 3 - Clear the Interrupt Identity bits (IIR). > 4 - Process the interrupt(s) that had bits set in the IIRs. > 5 - Re-enable Master Interrupt Control. >=20 > Without an atomic XCHG operation with mmio space, the above merely reduce= s the window > in which we can miss an interrupt (especially when you consider how heavy= weight the > I915_READ/I915_WRITE operations are). I can see how we can miss a second, third etc. back-to-back interrupt, but that's always a problem with edge triggered interrupts. But the rearranging done in this patchset closes the race where we are left with a pending interrupt flag without ever calling its handler. > We maintain the "disable SDE interrupts when handling" hack since apparen= tly it works. >=20 > Spotted by Bob Beckett . >=20 > v2: Add warning to commit message and comments to the code as per Chris W= ilson's request. > v3: Improve the source comments. >=20 > Signed-off-by: Oscar Mateo I couldn't spot any problems, so on all 4 patches: Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_= irq.c > index 5522cbf..a68f68c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2136,6 +2136,14 @@ static void ivb_display_irq_handler(struct drm_dev= ice *dev, u32 de_iir) > } > } > =20 > +/* > + * To handle irqs with the minimum potential races with fresh interrupts= , we: > + * 1 - Disable Master Interrupt Control. > + * 2 - Find the source(s) of the interrupt. > + * 3 - Clear the Interrupt Identity bits (IIR). > + * 4 - Process the interrupt(s) that had bits set in the IIRs. > + * 5 - Re-enable Master Interrupt Control. > + */ > static irqreturn_t ironlake_irq_handler(int irq, void *arg) > { > struct drm_device *dev =3D arg; > @@ -2163,32 +2171,34 @@ static irqreturn_t ironlake_irq_handler(int irq, = void *arg) > POSTING_READ(SDEIER); > } > =20 > + /* Find, clear, then process each source of interrupt */ > + > gt_iir =3D I915_READ(GTIIR); > if (gt_iir) { > + I915_WRITE(GTIIR, gt_iir); > + ret =3D IRQ_HANDLED; > if (INTEL_INFO(dev)->gen >=3D 6) > snb_gt_irq_handler(dev, dev_priv, gt_iir); > else > ilk_gt_irq_handler(dev, dev_priv, gt_iir); > - I915_WRITE(GTIIR, gt_iir); > - ret =3D IRQ_HANDLED; > } > =20 > de_iir =3D I915_READ(DEIIR); > if (de_iir) { > + I915_WRITE(DEIIR, de_iir); > + ret =3D IRQ_HANDLED; > if (INTEL_INFO(dev)->gen >=3D 7) > ivb_display_irq_handler(dev, de_iir); > else > ilk_display_irq_handler(dev, de_iir); > - I915_WRITE(DEIIR, de_iir); > - ret =3D IRQ_HANDLED; > } > =20 > if (INTEL_INFO(dev)->gen >=3D 6) { > u32 pm_iir =3D I915_READ(GEN6_PMIIR); > if (pm_iir) { > - gen6_rps_irq_handler(dev_priv, pm_iir); > I915_WRITE(GEN6_PMIIR, pm_iir); > ret =3D IRQ_HANDLED; > + gen6_rps_irq_handler(dev_priv, pm_iir); > } > } > =20 --=-R3+LN1/qnGRTlGS5qfQ1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJToKS3AAoJEORIIAnNuWDFzSIH/0j53bxRQxpVV1C4/ByqPg5N P4olezfI8TBv6TjCmyEhXb/eNATJ8+OgnwcGlszRo6vx+Xz4W0Z+8wTOjzT3Q3Xm 4m0coPUKuiXIUfxsSZ9+tXsrKyzt6HVfLhZxPdpzbwd8csQ3ZbZx/iXREy3IC0HT LK6ZGjCLg90pyqAeWMvpLAmSLt/lsBWzAyMb9nydJMYXInfPA2AFzWpHvwFnG+6F MZbnrbqXVkBt1q1Vks+G9mh0Bze0Or+jpKN85pRspbGqpcaHIkq9NDMF/NASvUcW ZQNiXTgLP7/ew8fB9Adb7B8y2VVAL3LKtf9A8Agi1JRlrihpj3NAocEjSzJThgs= =A9bk -----END PGP SIGNATURE----- --=-R3+LN1/qnGRTlGS5qfQ1-- --===============0211564579== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0211564579==--