Re: [Intel-gfx] [PATCH] drm/i915: Clear PIPE.STAT before IIR on VLV/CHV

2015-10-13 Thread Jani Nikula 
On Tue, 01 Sep 2015, Imre Deak  wrote:
> On pe, 2015-08-14 at 18:24 +0100, Chris Wilson wrote:
>> The PIPE.STAT register contains some interrupt status bits per pipe, and
>> if assert cause the corresponding bit in the IIR to be asserted (thus
>> raising an interrupt). When handling an interrupt, we should clear the
>> PIPE.STAT generator first before clearing the IIR so that we do not miss
>> events or cause spurious work.
>> 
>> This ordering was broken by
>> 
>> commit 27b6c122512ca30399bb1b39cc42eda83901f304
>> Author: Oscar Mateo 
>> Date:   Mon Jun 16 16:11:00 2014 +0100
>> 
>> drm/i915/chv: Ack interrupts before handling them (CHV)
>> 
>> commit 3ff60f89bc4836583f5bd195062f16c563bd97aa
>> Author: Oscar Mateo 
>> Date:   Mon Jun 16 16:10:58 2014 +0100
>> 
>> drm/i915/vlv: Ack interrupts before handling them (VLV)
>> 
>> in attempting to reduce the amount of work done between receiving an
>> interrupt and acknowledging it. In light of this motivation, split the
>> pipestat interrupt handler into two, one to acknowledge and clear the
>> interrupt and a later pass to process it.
>
> Yes, after thinking about hierarchical interrupt clearing/handling this
> makes complete sense. It was even hinted at by Ville in the discussion
> of the above patches, but I missed his point back then.
>
>> Signed-off-by: Chris Wilson 
>> Cc: Oscar Mateo 
>> Cc: Bob Beckett 
>> Cc: Imre Deak 
>> Cc: Daniel Vetter 
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 55 
>> +++--
>>  1 file changed, 31 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 66064511cffb..92bdfe6f91d8 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct 
>> drm_i915_private *dev_priv, u32 pm_iir)
>>  }
>>  }
>>  
>> -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
>> +static inline bool
>> +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
>>  {
>> -if (!drm_handle_vblank(dev, pipe))
>> -return false;
>> -
>> -return true;
>> +return drm_handle_vblank(dev, pipe);
>>  }
>>  
>> -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>> +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv,
>> +u32 iir, u32 *pipe_stats)
>>  {
>> -struct drm_i915_private *dev_priv = dev->dev_private;
>> -u32 pipe_stats[I915_MAX_PIPES] = { };
>>  int pipe;
>>  
>>  spin_lock(_priv->irq_lock);
>>  for_each_pipe(dev_priv, pipe) {
>> +u32 mask, val, iir_bit = 0;
>>  int reg;
>> -u32 mask, iir_bit = 0;
>>  
>>  /*
>>   * PIPESTAT bits get signalled even when the interrupt is
>> @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct 
>> drm_device *dev, u32 iir)
>>  
>>  /* fifo underruns are filterered in the underrun handler. */
>>  mask = PIPE_FIFO_UNDERRUN_STATUS;
>> +mask |= PIPESTAT_INT_ENABLE_MASK;
>>  
>>  switch (pipe) {
>>  case PIPE_A:
>> @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct 
>> drm_device *dev, u32 iir)
>>  if (iir & iir_bit)
>>  mask |= dev_priv->pipestat_irq_mask[pipe];
>>  
>> -if (!mask)
>> -continue;
>> -
>>  reg = PIPESTAT(pipe);
>> -mask |= PIPESTAT_INT_ENABLE_MASK;
>> -pipe_stats[pipe] = I915_READ(reg) & mask;
>> +val = I915_READ(reg);
>> +pipe_stats[pipe] |= val & mask;
>>  
>>  /*
>>   * Clear the PIPE*STAT regs before the IIR
>>   */
>> -if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
>> -PIPESTAT_INT_STATUS_MASK))
>> -I915_WRITE(reg, pipe_stats[pipe]);
>> +if (val & (PIPE_FIFO_UNDERRUN_STATUS |
>> +   PIPESTAT_INT_STATUS_MASK))
>> +I915_WRITE(reg, val);
>>  }
>>  spin_unlock(_priv->irq_lock);
>> +}
>> +
>> +static void valleyview_pipestat_irq_handler(struct drm_i915_private 
>> *dev_priv,
>> +u32 *pipe_stats)
>> +{
>> +struct drm_device *dev = dev_priv->dev;
>> +int pipe;
>>  
>>  for_each_pipe(dev_priv, pipe) {
>>  if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
>> @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, 
>> void *arg)
>>  {
>>  struct drm_device *dev = arg;
>>  struct drm_i915_private *dev_priv =

Re: [Intel-gfx] [PATCH] drm/i915: Clear PIPE.STAT before IIR on VLV/CHV

2015-09-01 Thread Imre Deak 
On pe, 2015-08-14 at 18:24 +0100, Chris Wilson wrote:
> The PIPE.STAT register contains some interrupt status bits per pipe, and
> if assert cause the corresponding bit in the IIR to be asserted (thus
> raising an interrupt). When handling an interrupt, we should clear the
> PIPE.STAT generator first before clearing the IIR so that we do not miss
> events or cause spurious work.
> 
> This ordering was broken by
> 
> commit 27b6c122512ca30399bb1b39cc42eda83901f304
> Author: Oscar Mateo 
> Date:   Mon Jun 16 16:11:00 2014 +0100
> 
> drm/i915/chv: Ack interrupts before handling them (CHV)
> 
> commit 3ff60f89bc4836583f5bd195062f16c563bd97aa
> Author: Oscar Mateo 
> Date:   Mon Jun 16 16:10:58 2014 +0100
> 
> drm/i915/vlv: Ack interrupts before handling them (VLV)
> 
> in attempting to reduce the amount of work done between receiving an
> interrupt and acknowledging it. In light of this motivation, split the
> pipestat interrupt handler into two, one to acknowledge and clear the
> interrupt and a later pass to process it.

Yes, after thinking about hierarchical interrupt clearing/handling this
makes complete sense. It was even hinted at by Ville in the discussion
of the above patches, but I missed his point back then.

> Signed-off-by: Chris Wilson 
> Cc: Oscar Mateo 
> Cc: Bob Beckett 
> Cc: Imre Deak 
> Cc: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 55 
> +++--
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 66064511cffb..92bdfe6f91d8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct 
> drm_i915_private *dev_priv, u32 pm_iir)
>   }
>  }
>  
> -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +static inline bool
> +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
>  {
> - if (!drm_handle_vblank(dev, pipe))
> - return false;
> -
> - return true;
> + return drm_handle_vblank(dev, pipe);
>  }
>  
> -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv,
> + u32 iir, u32 *pipe_stats)
>  {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 pipe_stats[I915_MAX_PIPES] = { };
>   int pipe;
>  
>   spin_lock(_priv->irq_lock);
>   for_each_pipe(dev_priv, pipe) {
> + u32 mask, val, iir_bit = 0;
>   int reg;
> - u32 mask, iir_bit = 0;
>  
>   /*
>* PIPESTAT bits get signalled even when the interrupt is
> @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct 
> drm_device *dev, u32 iir)
>  
>   /* fifo underruns are filterered in the underrun handler. */
>   mask = PIPE_FIFO_UNDERRUN_STATUS;
> + mask |= PIPESTAT_INT_ENABLE_MASK;
>  
>   switch (pipe) {
>   case PIPE_A:
> @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct 
> drm_device *dev, u32 iir)
>   if (iir & iir_bit)
>   mask |= dev_priv->pipestat_irq_mask[pipe];
>  
> - if (!mask)
> - continue;
> -
>   reg = PIPESTAT(pipe);
> - mask |= PIPESTAT_INT_ENABLE_MASK;
> - pipe_stats[pipe] = I915_READ(reg) & mask;
> + val = I915_READ(reg);
> + pipe_stats[pipe] |= val & mask;
>  
>   /*
>* Clear the PIPE*STAT regs before the IIR
>*/
> - if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> - PIPESTAT_INT_STATUS_MASK))
> - I915_WRITE(reg, pipe_stats[pipe]);
> + if (val & (PIPE_FIFO_UNDERRUN_STATUS |
> +PIPESTAT_INT_STATUS_MASK))
> + I915_WRITE(reg, val);
>   }
>   spin_unlock(_priv->irq_lock);
> +}
> +
> +static void valleyview_pipestat_irq_handler(struct drm_i915_private 
> *dev_priv,
> + u32 *pipe_stats)
> +{
> + struct drm_device *dev = dev_priv->dev;
> + int pipe;
>  
>   for_each_pipe(dev_priv, pipe) {
>   if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
> @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
> *arg)
>  {
>   struct drm_device *dev = arg;
>   struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 pipe_stats[I915_MAX_PIPES] = {};
>   u32 iir, gt_iir, pm_iir;
>   irqreturn_t ret = IRQ_NONE;
>  
> @@ -1600,6 +1603,7

Re: [Intel-gfx] [PATCH] drm/i915: Clear PIPE.STAT before IIR on VLV/CHV

2015-08-26 Thread Daniel Vetter 
On Fri, Aug 14, 2015 at 06:24:32PM +0100, Chris Wilson wrote:
 The PIPE.STAT register contains some interrupt status bits per pipe, and
 if assert cause the corresponding bit in the IIR to be asserted (thus
 raising an interrupt). When handling an interrupt, we should clear the
 PIPE.STAT generator first before clearing the IIR so that we do not miss
 events or cause spurious work.
 
 This ordering was broken by
 
 commit 27b6c122512ca30399bb1b39cc42eda83901f304
 Author: Oscar Mateo oscar.ma...@intel.com
 Date:   Mon Jun 16 16:11:00 2014 +0100
 
 drm/i915/chv: Ack interrupts before handling them (CHV)
 
 commit 3ff60f89bc4836583f5bd195062f16c563bd97aa
 Author: Oscar Mateo oscar.ma...@intel.com
 Date:   Mon Jun 16 16:10:58 2014 +0100
 
 drm/i915/vlv: Ack interrupts before handling them (VLV)
 
 in attempting to reduce the amount of work done between receiving an
 interrupt and acknowledging it. In light of this motivation, split the
 pipestat interrupt handler into two, one to acknowledge and clear the
 interrupt and a later pass to process it.
 
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
 Cc: Oscar Mateo oscar.ma...@intel.com
 Cc: Bob Beckett robert.beck...@intel.com
 Cc: Imre Deak imre.d...@intel.com
 Cc: Daniel Vetter daniel.vet...@ffwll.ch

Oscar/Imre, where's your review on this?

Thanks, Daniel
 ---
  drivers/gpu/drm/i915/i915_irq.c | 55 
 +++--
  1 file changed, 31 insertions(+), 24 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
 index 66064511cffb..92bdfe6f91d8 100644
 --- a/drivers/gpu/drm/i915/i915_irq.c
 +++ b/drivers/gpu/drm/i915/i915_irq.c
 @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct 
 drm_i915_private *dev_priv, u32 pm_iir)
   }
  }
  
 -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
 +static inline bool
 +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
  {
 - if (!drm_handle_vblank(dev, pipe))
 - return false;
 -
 - return true;
 + return drm_handle_vblank(dev, pipe);
  }
  
 -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv,
 + u32 iir, u32 *pipe_stats)
  {
 - struct drm_i915_private *dev_priv = dev-dev_private;
 - u32 pipe_stats[I915_MAX_PIPES] = { };
   int pipe;
  
   spin_lock(dev_priv-irq_lock);
   for_each_pipe(dev_priv, pipe) {
 + u32 mask, val, iir_bit = 0;
   int reg;
 - u32 mask, iir_bit = 0;
  
   /*
* PIPESTAT bits get signalled even when the interrupt is
 @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct 
 drm_device *dev, u32 iir)
  
   /* fifo underruns are filterered in the underrun handler. */
   mask = PIPE_FIFO_UNDERRUN_STATUS;
 + mask |= PIPESTAT_INT_ENABLE_MASK;
  
   switch (pipe) {
   case PIPE_A:
 @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct 
 drm_device *dev, u32 iir)
   if (iir  iir_bit)
   mask |= dev_priv-pipestat_irq_mask[pipe];
  
 - if (!mask)
 - continue;
 -
   reg = PIPESTAT(pipe);
 - mask |= PIPESTAT_INT_ENABLE_MASK;
 - pipe_stats[pipe] = I915_READ(reg)  mask;
 + val = I915_READ(reg);
 + pipe_stats[pipe] |= val  mask;
  
   /*
* Clear the PIPE*STAT regs before the IIR
*/
 - if (pipe_stats[pipe]  (PIPE_FIFO_UNDERRUN_STATUS |
 - PIPESTAT_INT_STATUS_MASK))
 - I915_WRITE(reg, pipe_stats[pipe]);
 + if (val  (PIPE_FIFO_UNDERRUN_STATUS |
 +PIPESTAT_INT_STATUS_MASK))
 + I915_WRITE(reg, val);
   }
   spin_unlock(dev_priv-irq_lock);
 +}
 +
 +static void valleyview_pipestat_irq_handler(struct drm_i915_private 
 *dev_priv,
 + u32 *pipe_stats)
 +{
 + struct drm_device *dev = dev_priv-dev;
 + int pipe;
  
   for_each_pipe(dev_priv, pipe) {
   if (pipe_stats[pipe]  PIPE_START_VBLANK_INTERRUPT_STATUS 
 @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
 *arg)
  {
   struct drm_device *dev = arg;
   struct drm_i915_private *dev_priv = dev-dev_private;
 + u32 pipe_stats[I915_MAX_PIPES] = {};
   u32 iir, gt_iir, pm_iir;
   irqreturn_t ret = IRQ_NONE;
  
 @@ -1600,6 +1603,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
 *arg)
   /* Consume port before clearing IIR or we'll miss 
 events */
   if (iir  I915_DISPLAY_PORT_INTERRUPT)
   i9xx_hpd_irq_handler(dev);
 +