From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits Date: Wed, 5 Feb 2014 16:54:37 +0200 Message-ID: <20140205145437.GK3891@intel.com> References: <1391542551-20239-1-git-send-email-imre.deak@intel.com> <1391542551-20239-7-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 mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 48E90106ED0 for ; Wed, 5 Feb 2014 06:54:42 -0800 (PST) Content-Disposition: inline In-Reply-To: <1391542551-20239-7-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:50PM +0200, Imre Deak wrote: > At least on VLV we can't get at the pipestat status bits by simply right > shifting the corresponding enable bits. The mapping between enable and > status bits for the sprite0,1 flip done and the PSR events don't follow > this rule, so we need to map them separately. > = > The PSR enable for pipe A is DPFLIPSTAT[22], but I haven't added support > for this, since there is no user of it atm. Until support is added WARN > if someone tries to enable PSR interrupts, or tries to enable the same > (1 << 6) bit on pipe B, which MBZ. > = > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > 2 files changed, 38 insertions(+), 1 deletion(-) > = > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_= irq.c > index ec56a70..eea5398 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -471,9 +471,29 @@ done: > return ret; > } > = > +static void set_pipestat_enable_bit(u32 status_mask, u32 status_bit, > + u32 enable_bit, u32 *enable_mask) > +{ > + if (status_mask & status_bit) > + *enable_mask |=3D enable_bit; > + else > + *enable_mask &=3D ~enable_bit; > +} > + > static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_ma= sk) > { > - return status_mask << 16; > + u32 enable_mask =3D status_mask << 16; > + > + if (!IS_VALLEYVIEW(dev)) > + return enable_mask; > + > + enable_mask &=3D ~PIPE_FIFO_UNDERRUN_STATUS; > + set_pipestat_enable_bit(status_mask, SPRITE1_FLIP_DONE_INT_STATUS_VLV, > + SPRITE1_FLIP_DONE_INT_EN_VLV, &enable_mask); > + set_pipestat_enable_bit(enable_mask, SPRITE0_FLIP_DONE_INT_STATUS_VLV, > + SPRITE0_FLIP_DONE_INT_EN_VLV, &enable_mask); This feels a bit subtle to me. Maybe do it in a bit more straightforward way? Eg.: enable_mask &=3D ~(PIPE_FIFO_UNDERRUN_STATUS | SPRITE0_FLIP_DONE_INT_EN_VLV | SPRITE1_FLIP_DONE_INT_EN_VLV); if (status_mask & SPRITE0_FLIP_DONE_INT_STATUS_VLV) enable_mask |=3D SPRITE0_FLIP_DONE_INT_EN_VLV; if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV) enable_mask |=3D SPRITE1_FLIP_DONE_INT_EN_VLV; > + > + return enable_mask; > } > = > void > @@ -489,6 +509,13 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, e= num pipe pipe, > if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK)) > return; > = > + /* > + * On pipe A we don't support the PSR interrupt yet, on pipe B the > + * same bit MBZ. > + */ > + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV)) > + return; Won't this break the BLC interrupt on gen3/4? > + > enable_mask =3D pipestat_to_enable_mask(dev_priv->dev, status_mask); > if ((pipestat & enable_mask) =3D=3D enable_mask) > return; > @@ -512,6 +539,13 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, = enum pipe pipe, > if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK)) > return; > = > + /* > + * On pipe A we don't support the PSR interrupt yet, on pipe B the > + * same bit MBZ. > + */ > + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV)) > + return; > + > enable_mask =3D pipestat_to_enable_mask(dev_priv->dev, status_mask); > if ((pipestat & enable_mask) =3D=3D 0) > return; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_= reg.h > index 599c7f6..c998c4f 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3240,6 +3240,7 @@ > #define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL<<22) > #define PIPE_ODD_FIELD_INTERRUPT_ENABLE (1UL<<21) > #define PIPE_EVEN_FIELD_INTERRUPT_ENABLE (1UL<<20) > +#define PIPE_B_PSR_INTERRUPT_ENABLE_VLV (1UL<<19) > #define PIPE_HOTPLUG_TV_INTERRUPT_ENABLE (1UL<<18) /* pre-965 */ > #define PIPE_START_VBLANK_INTERRUPT_ENABLE (1UL<<18) /* 965 or later */ > #define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17) > @@ -3256,8 +3257,10 @@ > #define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8) > #define PIPE_DPST_EVENT_STATUS (1UL<<7) > #define PIPE_LEGACY_BLC_EVENT_STATUS (1UL<<6) > +#define PIPE_A_PSR_STATUS_VLV (1UL<<6) > #define PIPE_ODD_FIELD_INTERRUPT_STATUS (1UL<<5) > #define PIPE_EVEN_FIELD_INTERRUPT_STATUS (1UL<<4) > +#define PIPE_B_PSR_STATUS_VLV (1UL<<3) > #define PIPE_HOTPLUG_TV_INTERRUPT_STATUS (1UL<<2) /* pre-965 */ > #define PIPE_START_VBLANK_INTERRUPT_STATUS (1UL<<2) /* 965 or later */ > #define PIPE_VBLANK_INTERRUPT_STATUS (1UL<<1) > -- = > 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