[PATCH 0/7] drm/i915: vlv: handle only enabled pipestat interrupts

[PATCH 0/7] drm/i915: vlv: handle only enabled pipestat interrupts

[Imre Deak]

Atm on VLV we handle any pending pipestat interruts, whether or not
these were actually enabled explicitly with i915_enable_pipestat(). This
may or may not cause any real problem, but for consistency it's worth
fixing. See the last patch for more details.

I also need this as a dependency for the VLV power domain support, since
there we have to make sure we properly disable all pipestat interrupts
during the power well is off.

I've already sent the first patch separate from this set, but I include
it here since it's a dependency.

Tested on VLV with igt/kms_flip.

Imre Deak (7):
  drm/i915: vlv: don't unmask IIR[DISPLAY_PIPE_A/B_VBLANK] interrupt
  drm/i915: factor out valleyview_pipestat_irq_handler
  drm/i915: vlv: s/spin_lock_irqsave/spin_lock/ in irq handler
  drm/i915: unify FLIP_DONE macro names
  drm/i915: pass status instead of enable flags to i915_enable_pipestat
  drm/i915: vlv: fix mapping of pipestat enable to status bits
  drm/i915: vlv: handle only enabled pipestat interrupt events

 drivers/gpu/drm/i915/i915_drv.h |   7 +-
 drivers/gpu/drm/i915/i915_irq.c | 218 ++++++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_reg.h |  28 ++++--
 drivers/gpu/drm/i915/intel_tv.c |   8 +-
 4 files changed, 172 insertions(+), 89 deletions(-)

-- 
1.8.4

[PATCH 1/7] drm/i915: vlv: don't unmask IIR[DISPLAY_PIPE_A/B_VBLANK] interrupt

[Imre Deak]

Bspec and the code suggests that the interrupt signaled by IIR[7,5]
(DISPLAY_PIPE_A/B_VBLANK) is a first level IRQ flag for the second
level PIPEA/BSTAT[2] (Start of Vertical Blank) interrupt. Measuring
the relative timings of when IIR[7] and PIPEASTAT[1,2] get set and
checking the effect of unmasking different pipestat and IIR events
shows that this isn't so:

First, ISR/IIR[7] gets set independently of PIPEASTAT[18] (Start of
Vertical Blank Enable) or any other pipestat enable bit, so it isn't
a first level IRQ bit showing the state of PIPEASTAT[2], but is
connected directly to the timing generator.

Second, setting only PIPEASTAT[18] and leaving all other pipestat events
disabled, IIR[6] (DISPLAY_PIPE_A_EVENT) gets set close to the moment when
PIPEASTAT[2] gets set, so the former is a first level interrupt flag for
the latter. The bspec is rather unclear about this, but I also assume
that IIR[6] signals all pipestat A events, except PIPEASTAT[31] (FIFO
Under-run Status).

Third, IIR[7] is set close to the moment when PIPEASTAT[1] (Framestart
Interrupt) gets set, in the mode I used about 12usec after PIPEASTAT[2]
and IIR[6] gets set. This means the IIR[7] isn't marking the start of
vblank, but rather signals the framestart event.

Based on the above, we don't need to unmask IIR[7] when waiting for
start of vblank events, but we can rely on IIR[6] being always unmasked,
which will signal when PIPEASTAT[2] gets set. Doing this will also get
rid of the overhead of getting an interrupt and servicing IIR[7], which
is atm raised always some time after IIR[6]/PIPEASTAT[2] is raised.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b226ae6..137ac65 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2297,18 +2297,11 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	unsigned long irqflags;
-	u32 imr;
 
 	if (!i915_pipe_enabled(dev, pipe))
 		return -EINVAL;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	imr = I915_READ(VLV_IMR);
-	if (pipe == PIPE_A)
-		imr &= ~I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
-	else
-		imr &= ~I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
-	I915_WRITE(VLV_IMR, imr);
 	i915_enable_pipestat(dev_priv, pipe,
 			     PIPE_START_VBLANK_INTERRUPT_ENABLE);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2366,17 +2359,10 @@ static void valleyview_disable_vblank(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	unsigned long irqflags;
-	u32 imr;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_disable_pipestat(dev_priv, pipe,
 			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
-	imr = I915_READ(VLV_IMR);
-	if (pipe == PIPE_A)
-		imr |= I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
-	else
-		imr |= I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
-	I915_WRITE(VLV_IMR, imr);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-- 
1.8.4

Re: [PATCH 1/7] drm/i915: vlv: don't unmask IIR[DISPLAY_PIPE_A/B_VBLANK] interrupt

[Jesse Barnes]

On Tue,  4 Feb 2014 21:35:45 +0200
Imre Deak <imre.deak@intel.com> wrote:

> Bspec and the code suggests that the interrupt signaled by IIR[7,5]
> (DISPLAY_PIPE_A/B_VBLANK) is a first level IRQ flag for the second
> level PIPEA/BSTAT[2] (Start of Vertical Blank) interrupt. Measuring
> the relative timings of when IIR[7] and PIPEASTAT[1,2] get set and
> checking the effect of unmasking different pipestat and IIR events
> shows that this isn't so:
> 
> First, ISR/IIR[7] gets set independently of PIPEASTAT[18] (Start of
> Vertical Blank Enable) or any other pipestat enable bit, so it isn't
> a first level IRQ bit showing the state of PIPEASTAT[2], but is
> connected directly to the timing generator.
> 
> Second, setting only PIPEASTAT[18] and leaving all other pipestat events
> disabled, IIR[6] (DISPLAY_PIPE_A_EVENT) gets set close to the moment when
> PIPEASTAT[2] gets set, so the former is a first level interrupt flag for
> the latter. The bspec is rather unclear about this, but I also assume
> that IIR[6] signals all pipestat A events, except PIPEASTAT[31] (FIFO
> Under-run Status).
> 
> Third, IIR[7] is set close to the moment when PIPEASTAT[1] (Framestart
> Interrupt) gets set, in the mode I used about 12usec after PIPEASTAT[2]
> and IIR[6] gets set. This means the IIR[7] isn't marking the start of
> vblank, but rather signals the framestart event.
> 
> Based on the above, we don't need to unmask IIR[7] when waiting for
> start of vblank events, but we can rely on IIR[6] being always unmasked,
> which will signal when PIPEASTAT[2] gets set. Doing this will also get
> rid of the overhead of getting an interrupt and servicing IIR[7], which
> is atm raised always some time after IIR[6]/PIPEASTAT[2] is raised.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b226ae6..137ac65 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2297,18 +2297,11 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	unsigned long irqflags;
> -	u32 imr;
>  
>  	if (!i915_pipe_enabled(dev, pipe))
>  		return -EINVAL;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	imr = I915_READ(VLV_IMR);
> -	if (pipe == PIPE_A)
> -		imr &= ~I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
> -	else
> -		imr &= ~I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
> -	I915_WRITE(VLV_IMR, imr);
>  	i915_enable_pipestat(dev_priv, pipe,
>  			     PIPE_START_VBLANK_INTERRUPT_ENABLE);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> @@ -2366,17 +2359,10 @@ static void valleyview_disable_vblank(struct drm_device *dev, int pipe)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	unsigned long irqflags;
> -	u32 imr;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_disable_pipestat(dev_priv, pipe,
>  			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
> -	imr = I915_READ(VLV_IMR);
> -	if (pipe == PIPE_A)
> -		imr |= I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
> -	else
> -		imr |= I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
> -	I915_WRITE(VLV_IMR, imr);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

[PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler

[Imre Deak]

This will be used by other platforms too, so factor it out.

The only functional change is the reordeing of gmbus_irq_handler() wrt.
the hotplug handling, but since it only schedules a work, it isn't an
issue.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 137ac65..b5524ea 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1477,15 +1477,53 @@ 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 = dev->dev_private;
+	u32 pipe_stats[I915_MAX_PIPES];
+	unsigned long irqflags;
+	int pipe;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	for_each_pipe(pipe) {
+		int reg = PIPESTAT(pipe);
+		pipe_stats[pipe] = I915_READ(reg);
+
+		/*
+		 * Clear the PIPE*STAT regs before the IIR
+		 */
+		if (pipe_stats[pipe] & 0x8000ffff)
+			I915_WRITE(reg, pipe_stats[pipe]);
+	}
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	for_each_pipe(pipe) {
+		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
+			drm_handle_vblank(dev, pipe);
+
+		if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
+			intel_prepare_page_flip(dev, pipe);
+			intel_finish_page_flip(dev, pipe);
+		}
+
+		if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
+			i9xx_pipe_crc_irq_handler(dev, pipe);
+
+		if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS &&
+		    intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
+			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
+	}
+
+	if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
+		gmbus_irq_handler(dev);
+}
+
 static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 iir, gt_iir, pm_iir;
 	irqreturn_t ret = IRQ_NONE;
-	unsigned long irqflags;
-	int pipe;
-	u32 pipe_stats[I915_MAX_PIPES];
 
 	while (true) {
 		iir = I915_READ(VLV_IIR);
@@ -1499,35 +1537,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 
 		snb_gt_irq_handler(dev, dev_priv, gt_iir);
 
-		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-		for_each_pipe(pipe) {
-			int reg = PIPESTAT(pipe);
-			pipe_stats[pipe] = I915_READ(reg);
-
-			/*
-			 * Clear the PIPE*STAT regs before the IIR
-			 */
-			if (pipe_stats[pipe] & 0x8000ffff)
-				I915_WRITE(reg, pipe_stats[pipe]);
-		}
-		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
-
-		for_each_pipe(pipe) {
-			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-				drm_handle_vblank(dev, pipe);
-
-			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
-				intel_prepare_page_flip(dev, pipe);
-				intel_finish_page_flip(dev, pipe);
-			}
-
-			if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
-				i9xx_pipe_crc_irq_handler(dev, pipe);
-
-			if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS &&
-			    intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
-				DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
-		}
+		valleyview_pipestat_irq_handler(dev, iir);
 
 		/* Consume port.  Then clear IIR or we'll miss events */
 		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
@@ -1543,8 +1553,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			I915_READ(PORT_HOTPLUG_STAT);
 		}
 
-		if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
-			gmbus_irq_handler(dev);
 
 		if (pm_iir)
 			gen6_rps_irq_handler(dev_priv, pm_iir);
-- 
1.8.4

Re: [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler

[Jesse Barnes]

On Tue,  4 Feb 2014 21:35:46 +0200
Imre Deak <imre.deak@intel.com> wrote:

> This will be used by other platforms too, so factor it out.
> 
> The only functional change is the reordeing of gmbus_irq_handler() wrt.
> the hotplug handling, but since it only schedules a work, it isn't an
> issue.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 137ac65..b5524ea 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1477,15 +1477,53 @@ 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 = dev->dev_private;
> +	u32 pipe_stats[I915_MAX_PIPES];
> +	unsigned long irqflags;
> +	int pipe;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	for_each_pipe(pipe) {
> +		int reg = PIPESTAT(pipe);
> +		pipe_stats[pipe] = I915_READ(reg);
> +
> +		/*
> +		 * Clear the PIPE*STAT regs before the IIR
> +		 */
> +		if (pipe_stats[pipe] & 0x8000ffff)
> +			I915_WRITE(reg, pipe_stats[pipe]);
> +	}
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	for_each_pipe(pipe) {
> +		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> +			drm_handle_vblank(dev, pipe);
> +
> +		if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> +			intel_prepare_page_flip(dev, pipe);
> +			intel_finish_page_flip(dev, pipe);
> +		}
> +
> +		if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
> +			i9xx_pipe_crc_irq_handler(dev, pipe);
> +
> +		if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS &&
> +		    intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> +			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
> +	}
> +
> +	if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
> +		gmbus_irq_handler(dev);
> +}
> +
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	u32 iir, gt_iir, pm_iir;
>  	irqreturn_t ret = IRQ_NONE;
> -	unsigned long irqflags;
> -	int pipe;
> -	u32 pipe_stats[I915_MAX_PIPES];
>  
>  	while (true) {
>  		iir = I915_READ(VLV_IIR);
> @@ -1499,35 +1537,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  
>  		snb_gt_irq_handler(dev, dev_priv, gt_iir);
>  
> -		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -		for_each_pipe(pipe) {
> -			int reg = PIPESTAT(pipe);
> -			pipe_stats[pipe] = I915_READ(reg);
> -
> -			/*
> -			 * Clear the PIPE*STAT regs before the IIR
> -			 */
> -			if (pipe_stats[pipe] & 0x8000ffff)
> -				I915_WRITE(reg, pipe_stats[pipe]);
> -		}
> -		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> -
> -		for_each_pipe(pipe) {
> -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> -				drm_handle_vblank(dev, pipe);
> -
> -			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> -				intel_prepare_page_flip(dev, pipe);
> -				intel_finish_page_flip(dev, pipe);
> -			}
> -
> -			if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
> -				i9xx_pipe_crc_irq_handler(dev, pipe);
> -
> -			if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS &&
> -			    intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> -				DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
> -		}
> +		valleyview_pipestat_irq_handler(dev, iir);
>  
>  		/* Consume port.  Then clear IIR or we'll miss events */
>  		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
> @@ -1543,8 +1553,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  			I915_READ(PORT_HOTPLUG_STAT);
>  		}
>  
> -		if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
> -			gmbus_irq_handler(dev);
>  
>  		if (pm_iir)
>  			gen6_rps_irq_handler(dev_priv, pm_iir);

I think we're still missing the asle bits too.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Re: [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler

[Daniel Vetter]

On Tue, Feb 04, 2014 at 09:35:46PM +0200, Imre Deak wrote:
> This will be used by other platforms too, so factor it out.
> 
> The only functional change is the reordeing of gmbus_irq_handler() wrt.
> the hotplug handling, but since it only schedules a work, it isn't an
> issue.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 137ac65..b5524ea 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1477,15 +1477,53 @@ 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 = dev->dev_private;

typedefs for structs are frowned upon. I've fixed this while applying.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler

[Jani Nikula]

On Wed, 05 Feb 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 04, 2014 at 09:35:46PM +0200, Imre Deak wrote:
>> This will be used by other platforms too, so factor it out.
>> 
>> The only functional change is the reordeing of gmbus_irq_handler() wrt.
>> the hotplug handling, but since it only schedules a work, it isn't an
>> issue.
>> 
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++++++++++------------------
>>  1 file changed, 42 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 137ac65..b5524ea 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1477,15 +1477,53 @@ 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 = dev->dev_private;
>
> typedefs for structs are frowned upon. I've fixed this while applying.

sed -i -e 's/drm_i915_private_t/struct drm_i915_private/' *.[ch]

plus a little hand-editing and be done with it...?


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler

[Daniel Vetter]

On Thu, Feb 06, 2014 at 09:14:09AM +0200, Jani Nikula wrote:
> On Wed, 05 Feb 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Feb 04, 2014 at 09:35:46PM +0200, Imre Deak wrote:
> >> This will be used by other platforms too, so factor it out.
> >> 
> >> The only functional change is the reordeing of gmbus_irq_handler() wrt.
> >> the hotplug handling, but since it only schedules a work, it isn't an
> >> issue.
> >> 
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++++++++++------------------
> >>  1 file changed, 42 insertions(+), 34 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 137ac65..b5524ea 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -1477,15 +1477,53 @@ 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 = dev->dev_private;
> >
> > typedefs for structs are frowned upon. I've fixed this while applying.
> 
> sed -i -e 's/drm_i915_private_t/struct drm_i915_private/' *.[ch]
> 
> plus a little hand-editing and be done with it...?

Split as per-file patches I guess we could do. A quick grep indicates that
we've fought down this typedef a lot already, most of the hits are in
areas no one touches anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/7] drm/i915: vlv: s/spin_lock_irqsave/spin_lock/ in irq handler

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b5524ea..e0e5190 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1481,10 +1481,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 pipe_stats[I915_MAX_PIPES];
-	unsigned long irqflags;
 	int pipe;
 
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	spin_lock(&dev_priv->irq_lock);
 	for_each_pipe(pipe) {
 		int reg = PIPESTAT(pipe);
 		pipe_stats[pipe] = I915_READ(reg);
@@ -1495,7 +1494,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 		if (pipe_stats[pipe] & 0x8000ffff)
 			I915_WRITE(reg, pipe_stats[pipe]);
 	}
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+	spin_unlock(&dev_priv->irq_lock);
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-- 
1.8.4

Re: [PATCH 3/7] drm/i915: vlv: s/spin_lock_irqsave/spin_lock/ in irq handler

[Jesse Barnes]

On Tue,  4 Feb 2014 21:35:47 +0200
Imre Deak <imre.deak@intel.com> wrote:

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b5524ea..e0e5190 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1481,10 +1481,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u32 pipe_stats[I915_MAX_PIPES];
> -	unsigned long irqflags;
>  	int pipe;
>  
> -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	spin_lock(&dev_priv->irq_lock);
>  	for_each_pipe(pipe) {
>  		int reg = PIPESTAT(pipe);
>  		pipe_stats[pipe] = I915_READ(reg);
> @@ -1495,7 +1494,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  		if (pipe_stats[pipe] & 0x8000ffff)
>  			I915_WRITE(reg, pipe_stats[pipe]);
>  	}
> -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +	spin_unlock(&dev_priv->irq_lock);
>  
>  	for_each_pipe(pipe) {
>  		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)

I guess we don't have to worry about new interrupts until we ack this
one, so:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

[PATCH 4/7] drm/i915: unify FLIP_DONE macro names

[Imre Deak]

s/FLIPDONE/FLIP_DONE/ to make all FLIP_DONE macro names consistent.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |  2 +-
 drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e0e5190..3b876ee 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1500,7 +1500,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
 			drm_handle_vblank(dev, pipe);
 
-		if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
+		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
 			intel_finish_page_flip(dev, pipe);
 		}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index abd18cd..8eefb26 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3227,7 +3227,7 @@
 #define   PIPECONF_DITHER_TYPE_TEMP (3<<2)
 #define _PIPEASTAT		(dev_priv->info->display_mmio_offset + 0x70024)
 #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL<<31)
-#define   SPRITE1_FLIPDONE_INT_EN_VLV		(1UL<<30)
+#define   SPRITE1_FLIP_DONE_INT_EN_VLV		(1UL<<30)
 #define   PIPE_CRC_ERROR_ENABLE			(1UL<<29)
 #define   PIPE_CRC_DONE_ENABLE			(1UL<<28)
 #define   PIPE_GMBUS_EVENT_ENABLE		(1UL<<27)
@@ -3245,12 +3245,12 @@
 #define   PIPE_VBLANK_INTERRUPT_ENABLE		(1UL<<17)
 #define   PIPEA_HBLANK_INT_EN_VLV		(1UL<<16)
 #define   PIPE_OVERLAY_UPDATED_ENABLE		(1UL<<16)
-#define   SPRITE1_FLIPDONE_INT_STATUS_VLV	(1UL<<15)
-#define   SPRITE0_FLIPDONE_INT_STATUS_VLV	(1UL<<14)
+#define   SPRITE1_FLIP_DONE_INT_STATUS_VLV	(1UL<<15)
+#define   SPRITE0_FLIP_DONE_INT_STATUS_VLV	(1UL<<14)
 #define   PIPE_CRC_ERROR_INTERRUPT_STATUS	(1UL<<13)
 #define   PIPE_CRC_DONE_INTERRUPT_STATUS	(1UL<<12)
 #define   PIPE_GMBUS_INTERRUPT_STATUS		(1UL<<11)
-#define   PLANE_FLIPDONE_INT_STATUS_VLV		(1UL<<10)
+#define   PLANE_FLIP_DONE_INT_STATUS_VLV	(1UL<<10)
 #define   PIPE_HOTPLUG_INTERRUPT_STATUS		(1UL<<10)
 #define   PIPE_VSYNC_INTERRUPT_STATUS		(1UL<<9)
 #define   PIPE_DISPLAY_LINE_COMPARE_STATUS	(1UL<<8)
@@ -3286,14 +3286,14 @@
 #define   PIPEB_LINE_COMPARE_INT_EN		(1<<29)
 #define   PIPEB_HLINE_INT_EN			(1<<28)
 #define   PIPEB_VBLANK_INT_EN			(1<<27)
-#define   SPRITED_FLIPDONE_INT_EN		(1<<26)
-#define   SPRITEC_FLIPDONE_INT_EN		(1<<25)
-#define   PLANEB_FLIPDONE_INT_EN		(1<<24)
+#define   SPRITED_FLIP_DONE_INT_EN		(1<<26)
+#define   SPRITEC_FLIP_DONE_INT_EN		(1<<25)
+#define   PLANEB_FLIP_DONE_INT_EN		(1<<24)
 #define   PIPEA_LINE_COMPARE_INT_EN		(1<<21)
 #define   PIPEA_HLINE_INT_EN			(1<<20)
 #define   PIPEA_VBLANK_INT_EN			(1<<19)
-#define   SPRITEB_FLIPDONE_INT_EN		(1<<18)
-#define   SPRITEA_FLIPDONE_INT_EN		(1<<17)
+#define   SPRITEB_FLIP_DONE_INT_EN		(1<<18)
+#define   SPRITEA_FLIP_DONE_INT_EN		(1<<17)
 #define   PLANEA_FLIPDONE_INT_EN		(1<<16)
 
 #define DPINVGTT				(VLV_DISPLAY_BASE + 0x7002c) /* VLV only */
-- 
1.8.4

Re: [PATCH 4/7] drm/i915: unify FLIP_DONE macro names

[Jesse Barnes]

On Tue,  4 Feb 2014 21:35:48 +0200
Imre Deak <imre.deak@intel.com> wrote:

> s/FLIPDONE/FLIP_DONE/ to make all FLIP_DONE macro names consistent.
> 
> No functional change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |  2 +-
>  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e0e5190..3b876ee 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1500,7 +1500,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
>  			drm_handle_vblank(dev, pipe);
>  
> -		if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> +		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
>  			intel_prepare_page_flip(dev, pipe);
>  			intel_finish_page_flip(dev, pipe);
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index abd18cd..8eefb26 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3227,7 +3227,7 @@
>  #define   PIPECONF_DITHER_TYPE_TEMP (3<<2)
>  #define _PIPEASTAT		(dev_priv->info->display_mmio_offset + 0x70024)
>  #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL<<31)
> -#define   SPRITE1_FLIPDONE_INT_EN_VLV		(1UL<<30)
> +#define   SPRITE1_FLIP_DONE_INT_EN_VLV		(1UL<<30)
>  #define   PIPE_CRC_ERROR_ENABLE			(1UL<<29)
>  #define   PIPE_CRC_DONE_ENABLE			(1UL<<28)
>  #define   PIPE_GMBUS_EVENT_ENABLE		(1UL<<27)
> @@ -3245,12 +3245,12 @@
>  #define   PIPE_VBLANK_INTERRUPT_ENABLE		(1UL<<17)
>  #define   PIPEA_HBLANK_INT_EN_VLV		(1UL<<16)
>  #define   PIPE_OVERLAY_UPDATED_ENABLE		(1UL<<16)
> -#define   SPRITE1_FLIPDONE_INT_STATUS_VLV	(1UL<<15)
> -#define   SPRITE0_FLIPDONE_INT_STATUS_VLV	(1UL<<14)
> +#define   SPRITE1_FLIP_DONE_INT_STATUS_VLV	(1UL<<15)
> +#define   SPRITE0_FLIP_DONE_INT_STATUS_VLV	(1UL<<14)
>  #define   PIPE_CRC_ERROR_INTERRUPT_STATUS	(1UL<<13)
>  #define   PIPE_CRC_DONE_INTERRUPT_STATUS	(1UL<<12)
>  #define   PIPE_GMBUS_INTERRUPT_STATUS		(1UL<<11)
> -#define   PLANE_FLIPDONE_INT_STATUS_VLV		(1UL<<10)
> +#define   PLANE_FLIP_DONE_INT_STATUS_VLV	(1UL<<10)
>  #define   PIPE_HOTPLUG_INTERRUPT_STATUS		(1UL<<10)
>  #define   PIPE_VSYNC_INTERRUPT_STATUS		(1UL<<9)
>  #define   PIPE_DISPLAY_LINE_COMPARE_STATUS	(1UL<<8)
> @@ -3286,14 +3286,14 @@
>  #define   PIPEB_LINE_COMPARE_INT_EN		(1<<29)
>  #define   PIPEB_HLINE_INT_EN			(1<<28)
>  #define   PIPEB_VBLANK_INT_EN			(1<<27)
> -#define   SPRITED_FLIPDONE_INT_EN		(1<<26)
> -#define   SPRITEC_FLIPDONE_INT_EN		(1<<25)
> -#define   PLANEB_FLIPDONE_INT_EN		(1<<24)
> +#define   SPRITED_FLIP_DONE_INT_EN		(1<<26)
> +#define   SPRITEC_FLIP_DONE_INT_EN		(1<<25)
> +#define   PLANEB_FLIP_DONE_INT_EN		(1<<24)
>  #define   PIPEA_LINE_COMPARE_INT_EN		(1<<21)
>  #define   PIPEA_HLINE_INT_EN			(1<<20)
>  #define   PIPEA_VBLANK_INT_EN			(1<<19)
> -#define   SPRITEB_FLIPDONE_INT_EN		(1<<18)
> -#define   SPRITEA_FLIPDONE_INT_EN		(1<<17)
> +#define   SPRITEB_FLIP_DONE_INT_EN		(1<<18)
> +#define   SPRITEA_FLIP_DONE_INT_EN		(1<<17)
>  #define   PLANEA_FLIPDONE_INT_EN		(1<<16)
>  
>  #define DPINVGTT				(VLV_DISPLAY_BASE + 0x7002c) /* VLV only */

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

[PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat

[Imre Deak]

There isn't any PSR interrupt enable bit for pipe A, so we couldn't
enable it through the current API. Passing the corresponding status bits
solves this and also makes the mapping between enable and status bits
simpler on VLV (addressed in an upcoming patch).

Except of checking for invalid status bit arguments, no functional
change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  6 ++--
 drivers/gpu/drm/i915/i915_irq.c | 68 +++++++++++++++++++++++++----------------
 drivers/gpu/drm/i915/i915_reg.h |  3 ++
 drivers/gpu/drm/i915/intel_tv.c |  8 ++---
 4 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fa37dfd..43f37ca 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1994,10 +1994,12 @@ extern void intel_uncore_check_errors(struct drm_device *dev);
 extern void intel_uncore_fini(struct drm_device *dev);
 
 void
-i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
+i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+		     u32 status_mask);
 
 void
-i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
+i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+		      u32 status_mask);
 
 /* i915_gem.c */
 int i915_gem_init_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3b876ee..ec56a70 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -471,36 +471,52 @@ done:
 	return ret;
 }
 
+static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_mask)
+{
+	return status_mask << 16;
+}
 
 void
-i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
+i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+		     u32 status_mask)
 {
 	u32 reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & 0x7fff0000;
+	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
+	u32 enable_mask;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if ((pipestat & mask) == mask)
+	if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
+		return;
+
+	enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
+	if ((pipestat & enable_mask) == enable_mask)
 		return;
 
 	/* Enable the interrupt, clear any pending status */
-	pipestat |= mask | (mask >> 16);
+	pipestat |= enable_mask | status_mask;
 	I915_WRITE(reg, pipestat);
 	POSTING_READ(reg);
 }
 
 void
-i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
+i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+		      u32 status_mask)
 {
 	u32 reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & 0x7fff0000;
+	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
+	u32 enable_mask;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if ((pipestat & mask) == 0)
+	if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
 		return;
 
-	pipestat &= ~mask;
+	enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
+	if ((pipestat & enable_mask) == 0)
+		return;
+
+	pipestat &= ~enable_mask;
 	I915_WRITE(reg, pipestat);
 	POSTING_READ(reg);
 }
@@ -518,10 +534,10 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_STATUS);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, PIPE_A,
-				     PIPE_LEGACY_BLC_EVENT_ENABLE);
+				     PIPE_LEGACY_BLC_EVENT_STATUS);
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
@@ -2270,10 +2286,10 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, pipe,
-				     PIPE_START_VBLANK_INTERRUPT_ENABLE);
+				     PIPE_START_VBLANK_INTERRUPT_STATUS);
 	else
 		i915_enable_pipestat(dev_priv, pipe,
-				     PIPE_VBLANK_INTERRUPT_ENABLE);
+				     PIPE_VBLANK_INTERRUPT_STATUS);
 
 	/* maintain vblank delivery even in deep C-states */
 	if (dev_priv->info->gen == 3)
@@ -2310,7 +2326,7 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe,
-			     PIPE_START_VBLANK_INTERRUPT_ENABLE);
+			     PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -2345,8 +2361,8 @@ static void i915_disable_vblank(struct drm_device *dev, int pipe)
 		I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_DIS));
 
 	i915_disable_pipestat(dev_priv, pipe,
-			      PIPE_VBLANK_INTERRUPT_ENABLE |
-			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
+			      PIPE_VBLANK_INTERRUPT_STATUS |
+			      PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
@@ -2369,7 +2385,7 @@ static void valleyview_disable_vblank(struct drm_device *dev, int pipe)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_disable_pipestat(dev_priv, pipe,
-			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
+			      PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
@@ -2918,8 +2934,8 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 enable_mask;
-	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV |
-		PIPE_CRC_DONE_ENABLE;
+	u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
+		PIPE_CRC_DONE_INTERRUPT_STATUS;
 	unsigned long irqflags;
 
 	enable_mask = I915_DISPLAY_PORT_INTERRUPT;
@@ -2950,7 +2966,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, PIPE_A, pipestat_enable);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_EVENT_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
 	i915_enable_pipestat(dev_priv, PIPE_B, pipestat_enable);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
@@ -3173,8 +3189,8 @@ static int i8xx_irq_postinstall(struct drm_device *dev)
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -3356,8 +3372,8 @@ static int i915_irq_postinstall(struct drm_device *dev)
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -3566,9 +3582,9 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_EVENT_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8eefb26..599c7f6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3263,6 +3263,9 @@
 #define   PIPE_VBLANK_INTERRUPT_STATUS		(1UL<<1)
 #define   PIPE_OVERLAY_UPDATED_STATUS		(1UL<<0)
 
+#define PIPESTAT_INT_ENABLE_MASK		0x7fff0000
+#define PIPESTAT_INT_STATUS_MASK		0x0000ffff
+
 #define PIPESRC(pipe) _PIPE(pipe, _PIPEASRC, _PIPEBSRC)
 #define PIPECONF(tran) _TRANSCODER(tran, _PIPEACONF, _PIPEBCONF)
 #define PIPEDSL(pipe)  _PIPE(pipe, _PIPEADSL, _PIPEBDSL)
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 22cf0f4..ccd02ec 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1189,8 +1189,8 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 		i915_disable_pipestat(dev_priv, 0,
-				      PIPE_HOTPLUG_INTERRUPT_ENABLE |
-				      PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
+				      PIPE_HOTPLUG_INTERRUPT_STATUS |
+				      PIPE_HOTPLUG_TV_INTERRUPT_STATUS);
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 	}
 
@@ -1266,8 +1266,8 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 		i915_enable_pipestat(dev_priv, 0,
-				     PIPE_HOTPLUG_INTERRUPT_ENABLE |
-				     PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
+				     PIPE_HOTPLUG_INTERRUPT_STATUS |
+				     PIPE_HOTPLUG_TV_INTERRUPT_STATUS);
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 	}
 
-- 
1.8.4

Re: [PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat

[Jesse Barnes]

On Tue,  4 Feb 2014 21:35:49 +0200
Imre Deak <imre.deak@intel.com> wrote:

> There isn't any PSR interrupt enable bit for pipe A, so we couldn't
> enable it through the current API. Passing the corresponding status bits
> solves this and also makes the mapping between enable and status bits
> simpler on VLV (addressed in an upcoming patch).
> 
> Except of checking for invalid status bit arguments, no functional
> change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  6 ++--
>  drivers/gpu/drm/i915/i915_irq.c | 68 +++++++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/i915_reg.h |  3 ++
>  drivers/gpu/drm/i915/intel_tv.c |  8 ++---
>  4 files changed, 53 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fa37dfd..43f37ca 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1994,10 +1994,12 @@ extern void intel_uncore_check_errors(struct drm_device *dev);
>  extern void intel_uncore_fini(struct drm_device *dev);
>  
>  void
> -i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
> +i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> +		     u32 status_mask);
>  
>  void
> -i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
> +i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> +		      u32 status_mask);
>  
>  /* i915_gem.c */
>  int i915_gem_init_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3b876ee..ec56a70 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -471,36 +471,52 @@ done:
>  	return ret;
>  }
>  
> +static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_mask)
> +{
> +	return status_mask << 16;
> +}
>  
>  void
> -i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
> +i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> +		     u32 status_mask)
>  {
>  	u32 reg = PIPESTAT(pipe);
> -	u32 pipestat = I915_READ(reg) & 0x7fff0000;
> +	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> +	u32 enable_mask;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if ((pipestat & mask) == mask)
> +	if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> +		return;
> +
> +	enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> +	if ((pipestat & enable_mask) == enable_mask)
>  		return;
>  
>  	/* Enable the interrupt, clear any pending status */
> -	pipestat |= mask | (mask >> 16);
> +	pipestat |= enable_mask | status_mask;
>  	I915_WRITE(reg, pipestat);
>  	POSTING_READ(reg);
>  }
>  
>  void
> -i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
> +i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> +		      u32 status_mask)
>  {
>  	u32 reg = PIPESTAT(pipe);
> -	u32 pipestat = I915_READ(reg) & 0x7fff0000;
> +	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> +	u32 enable_mask;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if ((pipestat & mask) == 0)
> +	if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
>  		return;
>  
> -	pipestat &= ~mask;
> +	enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> +	if ((pipestat & enable_mask) == 0)
> +		return;
> +
> +	pipestat &= ~enable_mask;
>  	I915_WRITE(reg, pipestat);
>  	POSTING_READ(reg);
>  }
> @@ -518,10 +534,10 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  
> -	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_ENABLE);
> +	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_STATUS);
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		i915_enable_pipestat(dev_priv, PIPE_A,
> -				     PIPE_LEGACY_BLC_EVENT_ENABLE);
> +				     PIPE_LEGACY_BLC_EVENT_STATUS);
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
> @@ -2270,10 +2286,10 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		i915_enable_pipestat(dev_priv, pipe,
> -				     PIPE_START_VBLANK_INTERRUPT_ENABLE);
> +				     PIPE_START_VBLANK_INTERRUPT_STATUS);
>  	else
>  		i915_enable_pipestat(dev_priv, pipe,
> -				     PIPE_VBLANK_INTERRUPT_ENABLE);
> +				     PIPE_VBLANK_INTERRUPT_STATUS);
>  
>  	/* maintain vblank delivery even in deep C-states */
>  	if (dev_priv->info->gen == 3)
> @@ -2310,7 +2326,7 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_enable_pipestat(dev_priv, pipe,
> -			     PIPE_START_VBLANK_INTERRUPT_ENABLE);
> +			     PIPE_START_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return 0;
> @@ -2345,8 +2361,8 @@ static void i915_disable_vblank(struct drm_device *dev, int pipe)
>  		I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_DIS));
>  
>  	i915_disable_pipestat(dev_priv, pipe,
> -			      PIPE_VBLANK_INTERRUPT_ENABLE |
> -			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
> +			      PIPE_VBLANK_INTERRUPT_STATUS |
> +			      PIPE_START_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> @@ -2369,7 +2385,7 @@ static void valleyview_disable_vblank(struct drm_device *dev, int pipe)
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_disable_pipestat(dev_priv, pipe,
> -			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
> +			      PIPE_START_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> @@ -2918,8 +2934,8 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	u32 enable_mask;
> -	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV |
> -		PIPE_CRC_DONE_ENABLE;
> +	u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
> +		PIPE_CRC_DONE_INTERRUPT_STATUS;
>  	unsigned long irqflags;
>  
>  	enable_mask = I915_DISPLAY_PORT_INTERRUPT;
> @@ -2950,7 +2966,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
>  	 * just to make the assert_spin_locked check happy. */
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_enable_pipestat(dev_priv, PIPE_A, pipestat_enable);
> -	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_EVENT_ENABLE);
> +	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
>  	i915_enable_pipestat(dev_priv, PIPE_B, pipestat_enable);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> @@ -3173,8 +3189,8 @@ static int i8xx_irq_postinstall(struct drm_device *dev)
>  	/* Interrupt setup is already guaranteed to be single-threaded, this is
>  	 * just to make the assert_spin_locked check happy. */
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
> -	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
> +	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
> +	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return 0;
> @@ -3356,8 +3372,8 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  	/* Interrupt setup is already guaranteed to be single-threaded, this is
>  	 * just to make the assert_spin_locked check happy. */
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
> -	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
> +	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
> +	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return 0;
> @@ -3566,9 +3582,9 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  	/* Interrupt setup is already guaranteed to be single-threaded, this is
>  	 * just to make the assert_spin_locked check happy. */
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_EVENT_ENABLE);
> -	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
> -	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
> +	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> +	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
> +	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8eefb26..599c7f6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3263,6 +3263,9 @@
>  #define   PIPE_VBLANK_INTERRUPT_STATUS		(1UL<<1)
>  #define   PIPE_OVERLAY_UPDATED_STATUS		(1UL<<0)
>  
> +#define PIPESTAT_INT_ENABLE_MASK		0x7fff0000
> +#define PIPESTAT_INT_STATUS_MASK		0x0000ffff
> +
>  #define PIPESRC(pipe) _PIPE(pipe, _PIPEASRC, _PIPEBSRC)
>  #define PIPECONF(tran) _TRANSCODER(tran, _PIPEACONF, _PIPEBCONF)
>  #define PIPEDSL(pipe)  _PIPE(pipe, _PIPEADSL, _PIPEBDSL)
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 22cf0f4..ccd02ec 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1189,8 +1189,8 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
>  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  		i915_disable_pipestat(dev_priv, 0,
> -				      PIPE_HOTPLUG_INTERRUPT_ENABLE |
> -				      PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
> +				      PIPE_HOTPLUG_INTERRUPT_STATUS |
> +				      PIPE_HOTPLUG_TV_INTERRUPT_STATUS);
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  	}
>  
> @@ -1266,8 +1266,8 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
>  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  		i915_enable_pipestat(dev_priv, 0,
> -				     PIPE_HOTPLUG_INTERRUPT_ENABLE |
> -				     PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
> +				     PIPE_HOTPLUG_INTERRUPT_STATUS |
> +				     PIPE_HOTPLUG_TV_INTERRUPT_STATUS);
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  	}
>  

I almost think we should just separate enable vs status entirely.  As
long as the bits are named consistently it may be easier to follow (as
Ville found in your next patch with the subtle remapping of status
bits).

But if everyone else is comfortable with this, it looks fine.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Re: [PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat

[Daniel Vetter]

On Wed, Feb 05, 2014 at 03:35:15PM +0000, Jesse Barnes wrote:
> I almost think we should just separate enable vs status entirely.  As
> long as the bits are named consistently it may be easier to follow (as
> Ville found in your next patch with the subtle remapping of status
> bits).

Yeah, I think for cases where the hw engineers just made a mess of it it's
better to be explicit. So what about keeping the current pipestat
enable/disable functions as wrappers which assume a regular mapping
betweeen status and mask bit, and then add a low-level function which
takes both mask and status explicitly?

That way we have less churn in the code, mostly pipestat enable/disable
still looks sane but the irregular cases will really stick out. For a name
I'd just go with __i915_enable_pipestat for lack of better ideas. Or maybe
i915_enable_pipestat_irregular.

Merged the patches thus far in this series to dinq.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat

[Ville Syrjälä]

On Wed, Feb 05, 2014 at 05:12:39PM +0100, Daniel Vetter wrote:
> On Wed, Feb 05, 2014 at 03:35:15PM +0000, Jesse Barnes wrote:
> > I almost think we should just separate enable vs status entirely.  As
> > long as the bits are named consistently it may be easier to follow (as
> > Ville found in your next patch with the subtle remapping of status
> > bits).
> 
> Yeah, I think for cases where the hw engineers just made a mess of it it's
> better to be explicit. So what about keeping the current pipestat
> enable/disable functions as wrappers which assume a regular mapping
> betweeen status and mask bit, and then add a low-level function which
> takes both mask and status explicitly?
> 
> That way we have less churn in the code, mostly pipestat enable/disable
> still looks sane but the irregular cases will really stick out. For a name
> I'd just go with __i915_enable_pipestat for lack of better ideas. Or maybe
> i915_enable_pipestat_irregular.

That could lead to someone accidentally using the regular function when
they should be using the irregular one and then we have some weird bug
on our hands. I rather like keeping the mess in one central place.

-- 
Ville Syrjälä
Intel OTC

[PATCH v2 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat

[Imre Deak]

There isn't any PSR interrupt enable bit for pipe A, so we couldn't
enable it through the current API. Passing the corresponding status bits
solves this and also makes the mapping between enable and status bits
simpler on VLV (addressed in an upcoming patch).

Except of checking for invalid status bit arguments, no functional
change.

v2: split out the low level parts of i915_enable_pipestat accepting
    separate enabled and status masks, to make the non-standard mapping
    between those masks stand out more (added in the next patch)
    (Jesse,Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  6 ++-
 drivers/gpu/drm/i915/i915_irq.c | 82 ++++++++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_reg.h |  3 ++
 drivers/gpu/drm/i915/intel_tv.c |  8 ++--
 4 files changed, 67 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e908c99..c8225ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1996,10 +1996,12 @@ extern void intel_uncore_check_errors(struct drm_device *dev);
 extern void intel_uncore_fini(struct drm_device *dev);
 
 void
-i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
+i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+		     u32 status_mask);
 
 void
-i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
+i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+		      u32 status_mask);
 
 /* i915_gem.c */
 int i915_gem_init_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6135976..37fe12d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -473,38 +473,68 @@ done:
 
 
 void
-i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
+__i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+		       u32 enable_mask, u32 status_mask)
 {
 	u32 reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & 0x7fff0000;
+	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if ((pipestat & mask) == mask)
+	if (WARN_ON_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
+	                 status_mask & ~PIPESTAT_INT_STATUS_MASK))
+		return;
+
+	if ((pipestat & enable_mask) == enable_mask)
 		return;
 
 	/* Enable the interrupt, clear any pending status */
-	pipestat |= mask | (mask >> 16);
+	pipestat |= enable_mask | status_mask;
 	I915_WRITE(reg, pipestat);
 	POSTING_READ(reg);
 }
 
 void
-i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
+__i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+		        u32 enable_mask, u32 status_mask)
 {
 	u32 reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & 0x7fff0000;
+	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if ((pipestat & mask) == 0)
+	if (WARN_ON_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
+	                 status_mask & ~PIPESTAT_INT_STATUS_MASK))
 		return;
 
-	pipestat &= ~mask;
+	if ((pipestat & enable_mask) == 0)
+		return;
+
+	pipestat &= ~enable_mask;
 	I915_WRITE(reg, pipestat);
 	POSTING_READ(reg);
 }
 
+void
+i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+		     u32 status_mask)
+{
+	u32 enable_mask;
+
+	enable_mask = status_mask << 16;
+	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
+}
+
+void
+i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+		      u32 status_mask)
+{
+	u32 enable_mask;
+
+	enable_mask = status_mask << 16;
+	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
+}
+
 /**
  * i915_enable_asle_pipestat - enable ASLE pipestat for OpRegion
  */
@@ -518,10 +548,10 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_STATUS);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, PIPE_A,
-				     PIPE_LEGACY_BLC_EVENT_ENABLE);
+				     PIPE_LEGACY_BLC_EVENT_STATUS);
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
@@ -2270,10 +2300,10 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, pipe,
-				     PIPE_START_VBLANK_INTERRUPT_ENABLE);
+				     PIPE_START_VBLANK_INTERRUPT_STATUS);
 	else
 		i915_enable_pipestat(dev_priv, pipe,
-				     PIPE_VBLANK_INTERRUPT_ENABLE);
+				     PIPE_VBLANK_INTERRUPT_STATUS);
 
 	/* maintain vblank delivery even in deep C-states */
 	if (dev_priv->info->gen == 3)
@@ -2310,7 +2340,7 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe,
-			     PIPE_START_VBLANK_INTERRUPT_ENABLE);
+			     PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -2345,8 +2375,8 @@ static void i915_disable_vblank(struct drm_device *dev, int pipe)
 		I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_DIS));
 
 	i915_disable_pipestat(dev_priv, pipe,
-			      PIPE_VBLANK_INTERRUPT_ENABLE |
-			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
+			      PIPE_VBLANK_INTERRUPT_STATUS |
+			      PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
@@ -2369,7 +2399,7 @@ static void valleyview_disable_vblank(struct drm_device *dev, int pipe)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_disable_pipestat(dev_priv, pipe,
-			      PIPE_START_VBLANK_INTERRUPT_ENABLE);
+			      PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
@@ -2917,8 +2947,8 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 enable_mask;
-	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV |
-		PIPE_CRC_DONE_ENABLE;
+	u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
+		PIPE_CRC_DONE_INTERRUPT_STATUS;
 	unsigned long irqflags;
 
 	enable_mask = I915_DISPLAY_PORT_INTERRUPT;
@@ -2949,7 +2979,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, PIPE_A, pipestat_enable);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_EVENT_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
 	i915_enable_pipestat(dev_priv, PIPE_B, pipestat_enable);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
@@ -3172,8 +3202,8 @@ static int i8xx_irq_postinstall(struct drm_device *dev)
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -3355,8 +3385,8 @@ static int i915_irq_postinstall(struct drm_device *dev)
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -3565,9 +3595,9 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_EVENT_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_ENABLE);
-	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_ENABLE);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b33349e..0e69c5c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3263,6 +3263,9 @@
 #define   PIPE_VBLANK_INTERRUPT_STATUS		(1UL<<1)
 #define   PIPE_OVERLAY_UPDATED_STATUS		(1UL<<0)
 
+#define PIPESTAT_INT_ENABLE_MASK		0x7fff0000
+#define PIPESTAT_INT_STATUS_MASK		0x0000ffff
+
 #define PIPESRC(pipe) _PIPE(pipe, _PIPEASRC, _PIPEBSRC)
 #define PIPECONF(tran) _TRANSCODER(tran, _PIPEACONF, _PIPEBCONF)
 #define PIPEDSL(pipe)  _PIPE(pipe, _PIPEADSL, _PIPEBDSL)
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 22cf0f4..ccd02ec 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1189,8 +1189,8 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 		i915_disable_pipestat(dev_priv, 0,
-				      PIPE_HOTPLUG_INTERRUPT_ENABLE |
-				      PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
+				      PIPE_HOTPLUG_INTERRUPT_STATUS |
+				      PIPE_HOTPLUG_TV_INTERRUPT_STATUS);
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 	}
 
@@ -1266,8 +1266,8 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 		i915_enable_pipestat(dev_priv, 0,
-				     PIPE_HOTPLUG_INTERRUPT_ENABLE |
-				     PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
+				     PIPE_HOTPLUG_INTERRUPT_STATUS |
+				     PIPE_HOTPLUG_TV_INTERRUPT_STATUS);
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 	}
 
-- 
1.8.4

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

[Imre Deak]

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);
+
+	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;
+
 	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

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

[Ville Syrjälä]

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;

> +
> +	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?

> +
>  	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

-- 
Ville Syrjälä
Intel OTC

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

[Imre Deak]

[-- 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

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

[Imre Deak]

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.

v2:
- inline the status->enable mask mapping (Ville)
- don't check for invalid PSR bit on platforms other than VLV (Ville)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h |  3 +++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 37fe12d..6166dd9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -515,13 +515,39 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 	POSTING_READ(reg);
 }
 
+static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
+{
+	u32 enable_mask = status_mask << 16;
+
+	/*
+	 * 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 0;
+
+	enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
+			 SPRITE0_FLIP_DONE_INT_STATUS_VLV |
+			 SPRITE1_FLIP_DONE_INT_STATUS_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;
+
+	return enable_mask;
+}
+
 void
 i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 		     u32 status_mask)
 {
 	u32 enable_mask;
 
-	enable_mask = status_mask << 16;
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+							   status_mask);
+	else
+		enable_mask = status_mask << 16;
 	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
 }
 
@@ -531,7 +557,11 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 {
 	u32 enable_mask;
 
-	enable_mask = status_mask << 16;
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+							   status_mask);
+	else
+		enable_mask = status_mask << 16;
 	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0e69c5c..c5e301e 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

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

[Ville Syrjälä]

On Wed, Feb 05, 2014 at 08:55:07PM +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.
> 
> v2:
> - inline the status->enable mask mapping (Ville)
> - don't check for invalid PSR bit on platforms other than VLV (Ville)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h |  3 +++
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 37fe12d..6166dd9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -515,13 +515,39 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  	POSTING_READ(reg);
>  }
>  
> +static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
> +{
> +	u32 enable_mask = status_mask << 16;
> +
> +	/*
> +	 * 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 0;
> +
> +	enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
> +			 SPRITE0_FLIP_DONE_INT_STATUS_VLV |
> +			 SPRITE1_FLIP_DONE_INT_STATUS_VLV);

Those two should have been ENABLE bits.

The rest looks fine. So fix that, and you can add (for the entire series):
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	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;
> +
> +	return enable_mask;
> +}
> +
>  void
>  i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  		     u32 status_mask)
>  {
>  	u32 enable_mask;
>  
> -	enable_mask = status_mask << 16;
> +	if (IS_VALLEYVIEW(dev_priv->dev))
> +		enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
> +							   status_mask);
> +	else
> +		enable_mask = status_mask << 16;
>  	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
>  }
>  
> @@ -531,7 +557,11 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  {
>  	u32 enable_mask;
>  
> -	enable_mask = status_mask << 16;
> +	if (IS_VALLEYVIEW(dev_priv->dev))
> +		enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
> +							   status_mask);
> +	else
> +		enable_mask = status_mask << 16;
>  	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0e69c5c..c5e301e 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älä
Intel OTC

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

[Imre Deak]

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.

v2:
- inline the status->enable mask mapping (Ville)
- don't check for invalid PSR bit on platforms other than VLV (Ville)
v3:
- fix bogus use of status bits in enable mask (Ville)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h |  3 +++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 37fe12d..68c942f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -515,13 +515,39 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 	POSTING_READ(reg);
 }
 
+static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
+{
+	u32 enable_mask = status_mask << 16;
+
+	/*
+	 * 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 0;
+
+	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;
+
+	return enable_mask;
+}
+
 void
 i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 		     u32 status_mask)
 {
 	u32 enable_mask;
 
-	enable_mask = status_mask << 16;
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+							   status_mask);
+	else
+		enable_mask = status_mask << 16;
 	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
 }
 
@@ -531,7 +557,11 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 {
 	u32 enable_mask;
 
-	enable_mask = status_mask << 16;
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+							   status_mask);
+	else
+		enable_mask = status_mask << 16;
 	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0e69c5c..c5e301e 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

[PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events

[Imre Deak]

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 <imre.deak@intel.com>
---
 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 = dev->dev_private;
+	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	struct intel_crtc *intel_crtc = 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, enum pipe pipe,
 	if ((pipestat & enable_mask) == enable_mask)
 		return;
 
+	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
+
 	/* Enable the interrupt, clear any pending status */
 	pipestat |= enable_mask | status_mask;
 	I915_WRITE(reg, pipestat);
@@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
 	if ((pipestat & enable_mask) == 0)
 		return;
 
+	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
+
 	pipestat &= ~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 = dev->dev_private;
-	u32 pipe_stats[I915_MAX_PIPES];
+	u32 pipe_stats[I915_MAX_PIPES] = { };
 	int pipe;
 
 	spin_lock(&dev_priv->irq_lock);
 	for_each_pipe(pipe) {
-		int reg = PIPESTAT(pipe);
+		int reg;
+		u32 mask;
+
+		if (!dev_priv->pipestat_irq_mask[pipe] &&
+		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
+			continue;
+
+		reg = PIPESTAT(pipe);
 		pipe_stats[pipe] = I915_READ(reg);
 
 		/*
 		 * Clear the PIPE*STAT regs before the IIR
 		 */
-		if (pipe_stats[pipe] & 0x8000ffff)
+		mask = PIPESTAT_INT_ENABLE_MASK | PIPE_FIFO_UNDERRUN_STATUS;
+		if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
+			mask |= dev_priv->pipestat_irq_mask[pipe];
+		pipe_stats[pipe] &= 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) == 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

Re: [PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events

[Ville Syrjälä]

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 <imre.deak@intel.com>
> ---
>  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 = dev->dev_private;
> +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +	struct intel_crtc *intel_crtc = 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, enum pipe pipe,
>  	if ((pipestat & enable_mask) == enable_mask)
>  		return;
>  
> +	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> +
>  	/* Enable the interrupt, clear any pending status */
>  	pipestat |= enable_mask | status_mask;
>  	I915_WRITE(reg, pipestat);
> @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
>  	if ((pipestat & enable_mask) == 0)
>  		return;
>  
> +	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> +
>  	pipestat &= ~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 = dev->dev_private;
> -	u32 pipe_stats[I915_MAX_PIPES];
> +	u32 pipe_stats[I915_MAX_PIPES] = { };
>  	int pipe;
>  
>  	spin_lock(&dev_priv->irq_lock);
>  	for_each_pipe(pipe) {
> -		int reg = PIPESTAT(pipe);
> +		int reg;
> +		u32 mask;
> +
> +		if (!dev_priv->pipestat_irq_mask[pipe] &&
> +		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> +			continue;
> +
> +		reg = PIPESTAT(pipe);
>  		pipe_stats[pipe] = I915_READ(reg);
>  
>  		/*
>  		 * Clear the PIPE*STAT regs before the IIR
>  		 */
> -		if (pipe_stats[pipe] & 0x8000ffff)
> +		mask = 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 |= dev_priv->pipestat_irq_mask[pipe];
> +		pipe_stats[pipe] &= 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) == 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älä
Intel OTC

Re: [PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events

[Imre Deak]

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

On Wed, 2014-02-05 at 17:11 +0200, Ville Syrjälä wrote:
> 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 <imre.deak@intel.com>
> > ---
> >  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 = dev->dev_private;
> > +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > +	struct intel_crtc *intel_crtc = 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, enum pipe pipe,
> >  	if ((pipestat & enable_mask) == enable_mask)
> >  		return;
> >  
> > +	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > +
> >  	/* Enable the interrupt, clear any pending status */
> >  	pipestat |= enable_mask | status_mask;
> >  	I915_WRITE(reg, pipestat);
> > @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> >  	if ((pipestat & enable_mask) == 0)
> >  		return;
> >  
> > +	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > +
> >  	pipestat &= ~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 = dev->dev_private;
> > -	u32 pipe_stats[I915_MAX_PIPES];
> > +	u32 pipe_stats[I915_MAX_PIPES] = { };
> >  	int pipe;
> >  
> >  	spin_lock(&dev_priv->irq_lock);
> >  	for_each_pipe(pipe) {
> > -		int reg = PIPESTAT(pipe);
> > +		int reg;
> > +		u32 mask;
> > +
> > +		if (!dev_priv->pipestat_irq_mask[pipe] &&
> > +		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> > +			continue;
> > +
> > +		reg = PIPESTAT(pipe);
> >  		pipe_stats[pipe] = I915_READ(reg);
> >  
> >  		/*
> >  		 * Clear the PIPE*STAT regs before the IIR
> >  		 */
> > -		if (pipe_stats[pipe] & 0x8000ffff)
> > +		mask = 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.

Ok, will fix it.

--Imre

> 
> 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 |= dev_priv->pipestat_irq_mask[pipe];
> > +		pipe_stats[pipe] &= 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) == 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
> 


[-- 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

[PATCH v2 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events

[Imre Deak]

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.

v2: make sure the underrun status is always ignored if its reporting is
    disabled (Ville)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 35 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h |  4 ++++
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8225ac..2751490a 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 6166dd9..b456356 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 = dev->dev_private;
+	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	struct intel_crtc *intel_crtc = 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
@@ -488,6 +498,8 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 	if ((pipestat & enable_mask) == enable_mask)
 		return;
 
+	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
+
 	/* Enable the interrupt, clear any pending status */
 	pipestat |= enable_mask | status_mask;
 	I915_WRITE(reg, pipestat);
@@ -510,6 +522,8 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 	if ((pipestat & enable_mask) == 0)
 		return;
 
+	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
+
 	pipestat &= ~enable_mask;
 	I915_WRITE(reg, pipestat);
 	POSTING_READ(reg);
@@ -1540,18 +1554,33 @@ 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 = dev->dev_private;
-	u32 pipe_stats[I915_MAX_PIPES];
+	u32 pipe_stats[I915_MAX_PIPES] = { };
 	int pipe;
 
 	spin_lock(&dev_priv->irq_lock);
 	for_each_pipe(pipe) {
-		int reg = PIPESTAT(pipe);
+		int reg;
+		u32 mask;
+
+		if (!dev_priv->pipestat_irq_mask[pipe] &&
+		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
+			continue;
+
+		reg = PIPESTAT(pipe);
 		pipe_stats[pipe] = I915_READ(reg);
 
 		/*
 		 * Clear the PIPE*STAT regs before the IIR
 		 */
-		if (pipe_stats[pipe] & 0x8000ffff)
+		mask = PIPESTAT_INT_ENABLE_MASK;
+		if (__cpu_fifo_underrun_reporting_enabled(dev, pipe))
+			mask |= PIPE_FIFO_UNDERRUN_STATUS;
+		if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
+			mask |= dev_priv->pipestat_irq_mask[pipe];
+		pipe_stats[pipe] &= 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 c5e301e..81b4edb 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) == 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