Re: [PATCH 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5499 bytes --]

On Wed, 2014-02-05 at 16:54 +0200, Ville Syrjälä wrote:
> 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 <imre.deak@intel.com>
> > ---
> >  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 |= enable_bit;
> > +	else
> > +		*enable_mask &= ~enable_bit;
> > +}
> > +
> >  static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_mask)
> >  {
> > -	return status_mask << 16;
> > +	u32 enable_mask = status_mask << 16;
> > +
> > +	if (!IS_VALLEYVIEW(dev))
> > +		return enable_mask;
> > +
> > +	enable_mask &= ~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 &= ~(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 |= SPRITE0_FLIP_DONE_INT_EN_VLV;
> if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
> 	enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;

I added the helpers in case more remappings are needed. But since we
have only the above two (and hopefully won't have more) I agree this is
simpler, so I'll rewrite it.

> > +
> > +	return enable_mask;
> >  }
> >  
> >  void
> > @@ -489,6 +509,13 @@ i915_enable_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;
> 
> Won't this break the BLC interrupt on gen3/4?

Good catch and me not testing this on those gens.. Will use instead here
and in i915_disable_pipestat:

if (WARN_ON_ONCE(IS_VALLEYVIEW(dev) && (status_mask &
PIPE_A_PSR_STATUS_VLV)))
	return;

--Imre

> > +
> >  	enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> >  	if ((pipestat & enable_mask) == 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 = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> >  	if ((pipestat & enable_mask) == 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
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx