From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/2] drm/i915: refactor ilk display interrupt handling Date: Wed, 30 Oct 2013 11:17:31 +0100 Message-ID: <20131030101731.GG4167@phenom.ffwll.local> References: <1382371476-25639-1-git-send-email-daniel.vetter@ffwll.ch> <1382371476-25639-2-git-send-email-daniel.vetter@ffwll.ch> <20131021164858.GC13047@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f52.google.com (mail-ee0-f52.google.com [74.125.83.52]) by gabe.freedesktop.org (Postfix) with ESMTP id D1C0BEEE54 for ; Wed, 30 Oct 2013 03:17:04 -0700 (PDT) Received: by mail-ee0-f52.google.com with SMTP id e49so507502eek.25 for ; Wed, 30 Oct 2013 03:17:03 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20131021164858.GC13047@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: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Mon, Oct 21, 2013 at 07:48:58PM +0300, Ville Syrj=E4l=E4 wrote: > On Mon, Oct 21, 2013 at 06:04:36PM +0200, Daniel Vetter wrote: > > - Use a for_each_loop and add the corresponding #defines. > > - Drop the _ILK postfix on the existing DE_PIPE_VBLANK macro for > > consistency with everything else. > > - Also use macros (and add the missing one for plane flips) for the > > ivb display interrupt handler. > > = > > Cc: Ville Syrj=E4l=E4 > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_irq.c | 50 +++++++++++++++++----------------= -------- > > drivers/gpu/drm/i915/i915_reg.h | 7 ++++-- > > 2 files changed, 26 insertions(+), 31 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i91= 5_irq.c > > index ce94afc..062a6d6 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -1535,6 +1535,7 @@ static void cpt_irq_handler(struct drm_device *de= v, u32 pch_iir) > > static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir) > > { > > struct drm_i915_private *dev_priv =3D dev->dev_private; > > + enum pipe pipe; > > = > > if (de_iir & DE_AUX_CHANNEL_A) > > dp_aux_irq_handler(dev); > > @@ -1542,37 +1543,26 @@ static void ilk_display_irq_handler(struct drm_= device *dev, u32 de_iir) > > if (de_iir & DE_GSE) > > intel_opregion_asle_intr(dev); > > = > > - if (de_iir & DE_PIPEA_VBLANK) > > - drm_handle_vblank(dev, 0); > > - > > - if (de_iir & DE_PIPEB_VBLANK) > > - drm_handle_vblank(dev, 1); > > - > > if (de_iir & DE_POISON) > > DRM_ERROR("Poison interrupt\n"); > > = > > - if (de_iir & DE_PIPEA_FIFO_UNDERRUN) > > - if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_A, false)) > > - DRM_DEBUG_DRIVER("Pipe A FIFO underrun\n"); > > - > > - if (de_iir & DE_PIPEB_FIFO_UNDERRUN) > > - if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_B, false)) > > - DRM_DEBUG_DRIVER("Pipe B FIFO underrun\n"); > > - > > - if (de_iir & DE_PIPEA_CRC_DONE) > > - i9xx_pipe_crc_irq_handler(dev, PIPE_A); > > + for_each_pipe(pipe) { > > + if (de_iir & DE_PIPE_VBLANK(pipe)) > > + drm_handle_vblank(dev, pipe); > > = > > - if (de_iir & DE_PIPEB_CRC_DONE) > > - i9xx_pipe_crc_irq_handler(dev, PIPE_B); > > + if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe)) > > + if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false)) > > + DRM_DEBUG_DRIVER("Pipe %c FIFO underrun\n", > > + pipe_name(pipe)); > > = > > - if (de_iir & DE_PLANEA_FLIP_DONE) { > > - intel_prepare_page_flip(dev, 0); > > - intel_finish_page_flip_plane(dev, 0); > > - } > > + if (de_iir & DE_PIPE_CRC_DONE(pipe)) > > + i9xx_pipe_crc_irq_handler(dev, pipe); > > = > > - if (de_iir & DE_PLANEB_FLIP_DONE) { > > - intel_prepare_page_flip(dev, 1); > > - intel_finish_page_flip_plane(dev, 1); > > + /* plane/pipes map 1:1 on ilk+ */ > > + if (de_iir & DE_PLANE_FLIP_DONE(pipe)) { > > + intel_prepare_page_flip(dev, pipe); > > + intel_finish_page_flip_plane(dev, pipe); > > + } > > } > > = > > /* check event from PCH */ > > @@ -1607,9 +1597,11 @@ static void ivb_display_irq_handler(struct drm_d= evice *dev, u32 de_iir) > > intel_opregion_asle_intr(dev); > > = > > for_each_pipe(i) { > > - if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i))) > > + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) > ^ ^ > = > > drm_handle_vblank(dev, i); > > - if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) { > > + > > + /* plane/pipes map 1:1 on ilk+ */ > > + if (de_iir & (DE_PLANE_FLIP_DONE_IVB(i))) { > ^ ^ > = > Just a minor complaint about the useless parens. Fixed and both merged, thanks for the review. -Daniel > = > But otherwise both patches look good, so: > Reviewed-by: Ville Syrj=E4l=E4 > = > > intel_prepare_page_flip(dev, i); > > intel_finish_page_flip_plane(dev, i); > > } > > @@ -2012,7 +2004,7 @@ static int ironlake_enable_vblank(struct drm_devi= ce *dev, int pipe) > > drm_i915_private_t *dev_priv =3D (drm_i915_private_t *) dev->dev_priv= ate; > > unsigned long irqflags; > > uint32_t bit =3D (INTEL_INFO(dev)->gen >=3D 7) ? DE_PIPE_VBLANK_IVB(p= ipe) : > > - DE_PIPE_VBLANK_ILK(pipe); > > + DE_PIPE_VBLANK(pipe); > > = > > if (!i915_pipe_enabled(dev, pipe)) > > return -EINVAL; > > @@ -2070,7 +2062,7 @@ static void ironlake_disable_vblank(struct drm_de= vice *dev, int pipe) > > drm_i915_private_t *dev_priv =3D (drm_i915_private_t *) dev->dev_priv= ate; > > unsigned long irqflags; > > uint32_t bit =3D (INTEL_INFO(dev)->gen >=3D 7) ? DE_PIPE_VBLANK_IVB(p= ipe) : > > - DE_PIPE_VBLANK_ILK(pipe); > > + DE_PIPE_VBLANK(pipe); > > = > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > ironlake_disable_display_irq(dev_priv, bit); > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i91= 5_reg.h > > index c97fc94..1af080a 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3928,6 +3928,7 @@ > > #define DE_SPRITEA_FLIP_DONE (1 << 28) > > #define DE_PLANEB_FLIP_DONE (1 << 27) > > #define DE_PLANEA_FLIP_DONE (1 << 26) > > +#define DE_PLANE_FLIP_DONE(plane) (1 << (26 + (plane))) > > #define DE_PCU_EVENT (1 << 25) > > #define DE_GTT_FAULT (1 << 24) > > #define DE_POISON (1 << 23) > > @@ -3944,12 +3945,15 @@ > > #define DE_PIPEB_CRC_DONE (1 << 10) > > #define DE_PIPEB_FIFO_UNDERRUN (1 << 8) > > #define DE_PIPEA_VBLANK (1 << 7) > > +#define DE_PIPE_VBLANK(pipe) (1 << (7 + 8*(pipe))) > > #define DE_PIPEA_EVEN_FIELD (1 << 6) > > #define DE_PIPEA_ODD_FIELD (1 << 5) > > #define DE_PIPEA_LINE_COMPARE (1 << 4) > > #define DE_PIPEA_VSYNC (1 << 3) > > #define DE_PIPEA_CRC_DONE (1 << 2) > > +#define DE_PIPE_CRC_DONE(pipe) (1 << (2 + 8*(pipe))) > > #define DE_PIPEA_FIFO_UNDERRUN (1 << 0) > > +#define DE_PIPE_FIFO_UNDERRUN(pipe) (1 << (8*(pipe))) > > = > > /* More Ivybridge lolz */ > > #define DE_ERR_INT_IVB (1<<30) > > @@ -3965,9 +3969,8 @@ > > #define DE_PIPEB_VBLANK_IVB (1<<5) > > #define DE_SPRITEA_FLIP_DONE_IVB (1<<4) > > #define DE_PLANEA_FLIP_DONE_IVB (1<<3) > > +#define DE_PLANE_FLIP_DONE_IVB(plane) (1<< (3 + 5*(plane))) > > #define DE_PIPEA_VBLANK_IVB (1<<0) > > - > > -#define DE_PIPE_VBLANK_ILK(pipe) (1 << ((pipe * 8) + 7)) > > #define DE_PIPE_VBLANK_IVB(pipe) (1 << (pipe * 5)) > > = > > #define VLV_MASTER_IER 0x4400c /* Gunit master IER */ > > -- = > > 1.8.4.rc3 > = > -- = > Ville Syrj=E4l=E4 > Intel OTC -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch