[PATCH 00/14] irq locking review v2

[PATCH 00/14] irq locking review v2

[Daniel Vetter]

Hi all,

So I've hopefully taken all review feedback into account, fixed a few other
things on top (accessing gen6+ PM registers on ilk mostly) and polished the turd
otherwise.

I've also tried to come up with some infrastructure to reproduce fifo underruns
for testing so that we can make sure we don't break stuff. But it looks like
that's a bit too hard to easily do, or I'm just not good enough at creating bad
watermarks ;-)

Anyway a few patches still need an r-b, so comments and review highly welcome.

Cheers, Daniel

Daniel Vetter (14):
  drm/i915: extract ibx_display_interrupt_update
  drm/i915: improve SERR_INT clearing for fifo underrun reporting
  drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting
  drm/i915: kill lpt pch transcoder->crtc mapping code for fifo
    underruns
  drm/i915: irq handlers don't need interrupt-safe spinlocks
  drm/i915: streamline hsw_pm_irq_handler
  drm/i915: queue work outside spinlock in hsw_pm_irq_handler
  drm/i915: kill dev_priv->rps.lock
  drm/i915: unify ring irq refcounts (again)
  drm/i915: don't enable PM_VEBOX_CS_ERROR_INTERRUPT
  drm/i915: unify PM interrupt preinstall sequence
  drm/i915: unify GT/PM irq postinstall code
  drm/i915: extract rps interrupt enable/disable helpers
  drm/i915: simplify rps interrupt enabling/disabling sequence

 drivers/gpu/drm/i915/i915_dma.c         |   1 -
 drivers/gpu/drm/i915/i915_drv.h         |   8 +-
 drivers/gpu/drm/i915/i915_irq.c         | 328 ++++++++++++++++----------------
 drivers/gpu/drm/i915/i915_reg.h         |   2 +
 drivers/gpu/drm/i915/intel_pm.c         |  65 +++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |  31 ++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |   5 +-
 7 files changed, 220 insertions(+), 220 deletions(-)

-- 
1.8.1.4

[PATCH 01/14] drm/i915: extract ibx_display_interrupt_update

[Daniel Vetter]

This way all changes to SDEIMR all go through the same function, with
the exception of the (single-threaded) setup/teardown code.

For paranoia again add an assert_spin_locked.

v2: For even more paranoia also sprinkle a spinlock assert over
cpt_can_enable_serr_int since we need to have that one there, too.

v3: Fix the logic of interrupt enabling, add enable/disable macros for
the simple cases in the fifo code and add a comment. All requested by
Paulo.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 51 +++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4aedd38..80b88c8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -128,6 +128,8 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
 	enum pipe pipe;
 	struct intel_crtc *crtc;
 
+	assert_spin_locked(&dev_priv->irq_lock);
+
 	for_each_pipe(pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 
@@ -170,6 +172,30 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 	}
 }
 
+/**
+ * ibx_display_interrupt_update - update SDEIMR
+ * @dev_priv: driver private
+ * @interrupt_mask: mask of interrupt bits to update
+ * @enabled_irq_mask: mask of interrupt bits to enable
+ */
+static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
+					 uint32_t interrupt_mask,
+					 uint32_t enabled_irq_mask)
+{
+	uint32_t sdeimr = I915_READ(SDEIMR);
+	sdeimr &= ~interrupt_mask;
+	sdeimr |= (~enabled_irq_mask & interrupt_mask);
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	I915_WRITE(SDEIMR, sdeimr);
+	POSTING_READ(SDEIMR);
+}
+#define ibx_enable_display_interrupt(dev_priv, bits) \
+	ibx_display_interrupt_update((dev_priv), (bits), (bits))
+#define ibx_disable_display_interrupt(dev_priv, bits) \
+	ibx_display_interrupt_update((dev_priv), (bits), 0)
+
 static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
 					    bool enable)
 {
@@ -179,11 +205,9 @@ static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
 						SDE_TRANSB_FIFO_UNDER;
 
 	if (enable)
-		I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
+		ibx_enable_display_interrupt(dev_priv, bit);
 	else
-		I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
-
-	POSTING_READ(SDEIMR);
+		ibx_disable_display_interrupt(dev_priv, bit);
 }
 
 static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
@@ -200,12 +224,10 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
 				     SERR_INT_TRANS_C_FIFO_UNDERRUN);
 
-		I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
+		ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	} else {
-		I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
+		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	}
-
-	POSTING_READ(SDEIMR);
 }
 
 /**
@@ -2652,22 +2674,21 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_encoder *intel_encoder;
-	u32 mask = ~I915_READ(SDEIMR);
-	u32 hotplug;
+	u32 hotplug_irqs, hotplug, enabled_irqs = 0;
 
 	if (HAS_PCH_IBX(dev)) {
-		mask &= ~SDE_HOTPLUG_MASK;
+		hotplug_irqs = SDE_HOTPLUG_MASK;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
 			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
-				mask |= hpd_ibx[intel_encoder->hpd_pin];
+				enabled_irqs |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
-		mask &= ~SDE_HOTPLUG_MASK_CPT;
+		hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
 			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
-				mask |= hpd_cpt[intel_encoder->hpd_pin];
+				enabled_irqs |= hpd_cpt[intel_encoder->hpd_pin];
 	}
 
-	I915_WRITE(SDEIMR, ~mask);
+	ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs);
 
 	/*
 	 * Enable digital hotplug on the PCH, and configure the DP short pulse
-- 
1.8.1.4

Re: [PATCH 01/14] drm/i915: extract ibx_display_interrupt_update

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This way all changes to SDEIMR all go through the same function, with
> the exception of the (single-threaded) setup/teardown code.
>
> For paranoia again add an assert_spin_locked.
>
> v2: For even more paranoia also sprinkle a spinlock assert over
> cpt_can_enable_serr_int since we need to have that one there, too.
>
> v3: Fix the logic of interrupt enabling, add enable/disable macros for
> the simple cases in the fifo code and add a comment. All requested by
> Paulo.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

How about also converting ironlake_{enable,disable}_display_irq to the
same template on a separate patch? Perhaps we could merge everything
into an even more generic update_imr(dev_priv, to_update_mask,
to_enable_mask, register, &dev_priv->xxx_mask).

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 51 +++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4aedd38..80b88c8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -128,6 +128,8 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
>         enum pipe pipe;
>         struct intel_crtc *crtc;
>
> +       assert_spin_locked(&dev_priv->irq_lock);
> +
>         for_each_pipe(pipe) {
>                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>
> @@ -170,6 +172,30 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
>         }
>  }
>
> +/**
> + * ibx_display_interrupt_update - update SDEIMR
> + * @dev_priv: driver private
> + * @interrupt_mask: mask of interrupt bits to update
> + * @enabled_irq_mask: mask of interrupt bits to enable

Optional bikeshedding: I'd call the variables  to_update_mask and
to_enable_mask since IMHO it's much more easier to understand (and
would also remove the need for documentation).

With or without that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> + */
> +static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
> +                                        uint32_t interrupt_mask,
> +                                        uint32_t enabled_irq_mask)
> +{
> +       uint32_t sdeimr = I915_READ(SDEIMR);
> +       sdeimr &= ~interrupt_mask;
> +       sdeimr |= (~enabled_irq_mask & interrupt_mask);
> +
> +       assert_spin_locked(&dev_priv->irq_lock);
> +
> +       I915_WRITE(SDEIMR, sdeimr);
> +       POSTING_READ(SDEIMR);
> +}
> +#define ibx_enable_display_interrupt(dev_priv, bits) \
> +       ibx_display_interrupt_update((dev_priv), (bits), (bits))
> +#define ibx_disable_display_interrupt(dev_priv, bits) \
> +       ibx_display_interrupt_update((dev_priv), (bits), 0)
> +
>  static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
>                                             bool enable)
>  {
> @@ -179,11 +205,9 @@ static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
>                                                 SDE_TRANSB_FIFO_UNDER;
>
>         if (enable)
> -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
> +               ibx_enable_display_interrupt(dev_priv, bit);
>         else
> -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
> -
> -       POSTING_READ(SDEIMR);
> +               ibx_disable_display_interrupt(dev_priv, bit);
>  }
>
>  static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> @@ -200,12 +224,10 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>                                      SERR_INT_TRANS_B_FIFO_UNDERRUN |
>                                      SERR_INT_TRANS_C_FIFO_UNDERRUN);
>
> -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
> +               ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
>         } else {
> -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
> +               ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
>         }
> -
> -       POSTING_READ(SDEIMR);
>  }
>
>  /**
> @@ -2652,22 +2674,21 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>         drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>         struct drm_mode_config *mode_config = &dev->mode_config;
>         struct intel_encoder *intel_encoder;
> -       u32 mask = ~I915_READ(SDEIMR);
> -       u32 hotplug;
> +       u32 hotplug_irqs, hotplug, enabled_irqs = 0;
>
>         if (HAS_PCH_IBX(dev)) {
> -               mask &= ~SDE_HOTPLUG_MASK;
> +               hotplug_irqs = SDE_HOTPLUG_MASK;
>                 list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
>                         if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> -                               mask |= hpd_ibx[intel_encoder->hpd_pin];
> +                               enabled_irqs |= hpd_ibx[intel_encoder->hpd_pin];
>         } else {
> -               mask &= ~SDE_HOTPLUG_MASK_CPT;
> +               hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
>                 list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
>                         if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> -                               mask |= hpd_cpt[intel_encoder->hpd_pin];
> +                               enabled_irqs |= hpd_cpt[intel_encoder->hpd_pin];
>         }
>
> -       I915_WRITE(SDEIMR, ~mask);
> +       ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs);
>
>         /*
>          * Enable digital hotplug on the PCH, and configure the DP short pulse
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 01/14] drm/i915: extract ibx_display_interrupt_update

[Daniel Vetter]

On Mon, Jul 08, 2013 at 11:38:15AM -0300, Paulo Zanoni wrote:
> 2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > This way all changes to SDEIMR all go through the same function, with
> > the exception of the (single-threaded) setup/teardown code.
> >
> > For paranoia again add an assert_spin_locked.
> >
> > v2: For even more paranoia also sprinkle a spinlock assert over
> > cpt_can_enable_serr_int since we need to have that one there, too.
> >
> > v3: Fix the logic of interrupt enabling, add enable/disable macros for
> > the simple cases in the fifo code and add a comment. All requested by
> > Paulo.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> How about also converting ironlake_{enable,disable}_display_irq to the
> same template on a separate patch? Perhaps we could merge everything
> into an even more generic update_imr(dev_priv, to_update_mask,
> to_enable_mask, register, &dev_priv->xxx_mask).

The reason I've opted for the more flexible scheme here for IBX is that
the hpd code updates all the hpd bits at once. On the cpu side we don't
(yet) have such a use-case. So as long as we only enable/disable one bit
we can keep the current code.

> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 51 +++++++++++++++++++++++++++++------------
> >  1 file changed, 36 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4aedd38..80b88c8 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -128,6 +128,8 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
> >         enum pipe pipe;
> >         struct intel_crtc *crtc;
> >
> > +       assert_spin_locked(&dev_priv->irq_lock);
> > +
> >         for_each_pipe(pipe) {
> >                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >
> > @@ -170,6 +172,30 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
> >         }
> >  }
> >
> > +/**
> > + * ibx_display_interrupt_update - update SDEIMR
> > + * @dev_priv: driver private
> > + * @interrupt_mask: mask of interrupt bits to update
> > + * @enabled_irq_mask: mask of interrupt bits to enable
> 
> Optional bikeshedding: I'd call the variables  to_update_mask and
> to_enable_mask since IMHO it's much more easier to understand (and
> would also remove the need for documentation).

Hm, in my code reading English that doesn't improve things ... not a
native speaker either so I think I'll just leave it as-is.

> With or without that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks, I'll go through the other comments and then merge.
-Daniel

> 
> 
> > + */
> > +static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
> > +                                        uint32_t interrupt_mask,
> > +                                        uint32_t enabled_irq_mask)
> > +{
> > +       uint32_t sdeimr = I915_READ(SDEIMR);
> > +       sdeimr &= ~interrupt_mask;
> > +       sdeimr |= (~enabled_irq_mask & interrupt_mask);
> > +
> > +       assert_spin_locked(&dev_priv->irq_lock);
> > +
> > +       I915_WRITE(SDEIMR, sdeimr);
> > +       POSTING_READ(SDEIMR);
> > +}
> > +#define ibx_enable_display_interrupt(dev_priv, bits) \
> > +       ibx_display_interrupt_update((dev_priv), (bits), (bits))
> > +#define ibx_disable_display_interrupt(dev_priv, bits) \
> > +       ibx_display_interrupt_update((dev_priv), (bits), 0)
> > +
> >  static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
> >                                             bool enable)
> >  {
> > @@ -179,11 +205,9 @@ static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
> >                                                 SDE_TRANSB_FIFO_UNDER;
> >
> >         if (enable)
> > -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
> > +               ibx_enable_display_interrupt(dev_priv, bit);
> >         else
> > -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
> > -
> > -       POSTING_READ(SDEIMR);
> > +               ibx_disable_display_interrupt(dev_priv, bit);
> >  }
> >
> >  static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> > @@ -200,12 +224,10 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> >                                      SERR_INT_TRANS_B_FIFO_UNDERRUN |
> >                                      SERR_INT_TRANS_C_FIFO_UNDERRUN);
> >
> > -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
> > +               ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> >         } else {
> > -               I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
> > +               ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> >         }
> > -
> > -       POSTING_READ(SDEIMR);
> >  }
> >
> >  /**
> > @@ -2652,22 +2674,21 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
> >         drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> >         struct drm_mode_config *mode_config = &dev->mode_config;
> >         struct intel_encoder *intel_encoder;
> > -       u32 mask = ~I915_READ(SDEIMR);
> > -       u32 hotplug;
> > +       u32 hotplug_irqs, hotplug, enabled_irqs = 0;
> >
> >         if (HAS_PCH_IBX(dev)) {
> > -               mask &= ~SDE_HOTPLUG_MASK;
> > +               hotplug_irqs = SDE_HOTPLUG_MASK;
> >                 list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> >                         if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> > -                               mask |= hpd_ibx[intel_encoder->hpd_pin];
> > +                               enabled_irqs |= hpd_ibx[intel_encoder->hpd_pin];
> >         } else {
> > -               mask &= ~SDE_HOTPLUG_MASK_CPT;
> > +               hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
> >                 list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> >                         if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> > -                               mask |= hpd_cpt[intel_encoder->hpd_pin];
> > +                               enabled_irqs |= hpd_cpt[intel_encoder->hpd_pin];
> >         }
> >
> > -       I915_WRITE(SDEIMR, ~mask);
> > +       ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs);
> >
> >         /*
> >          * Enable digital hotplug on the PCH, and configure the DP short pulse
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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

[PATCH 02/14] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Daniel Vetter]

The current code won't report any fifo underruns on cpt if just one
pipe has fifo underrun reporting disabled. We can't enable the
interrupts, but we can still check the per-transcoder bits and so
report the underrun delayed if:
- We always clear the transcoder's bit (and none of the other bits)
  when enabling.
- We check the transcoder's bit after disabling (to avoid racing with
  the interrupt handler).

v2: I've forgotten to actually remove the old SERR_INT clearing.

v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
noticed a logic bug: When an underrun interrupt fires we report it
both in the interrupt handler and when checking for underruns when
disabling it in cpt_set_fifo_underrun_reporting. But that second check
is only required if the interrupt is disabled and we're switching of
underrun reporting (e.g. because we're disabling the crtc). Hence
check for that condition.

At first I wanted to rework the code to pass that bit of information
from the uppper functions down to cpt_set_fifo_underrun_reporting. But
that turned out too messy. Hence the quick&dirty check whether the
south error interrupt source is masked off or not.

v4: Streamline the control flow a bit.

v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 80b88c8..bbf4df8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -215,18 +215,26 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 					    bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
 
 	if (enable) {
+		I915_WRITE(SERR_INT,
+			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
+
 		if (!cpt_can_enable_serr_int(dev))
 			return;
 
-		I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
-				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
-				     SERR_INT_TRANS_C_FIFO_UNDERRUN);
-
 		ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	} else {
+		uint32_t tmp = I915_READ(SERR_INT);
+
 		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
+
+		if (!was_enabled &&
+		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
+			DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %i\n",
+				      transcoder_name(pch_transcoder));
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9b51be8..5bba39a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3881,6 +3881,7 @@
 #define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
 #define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
 #define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
+#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)	(1<< (pipe*3))
 
 /* digital port hotplug */
 #define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
-- 
1.8.1.4

Re: [PATCH 02/14] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current code won't report any fifo underruns on cpt if just one
> pipe has fifo underrun reporting disabled. We can't enable the
> interrupts, but we can still check the per-transcoder bits and so
> report the underrun delayed if:
> - We always clear the transcoder's bit (and none of the other bits)
>   when enabling.
> - We check the transcoder's bit after disabling (to avoid racing with
>   the interrupt handler).
>
> v2: I've forgotten to actually remove the old SERR_INT clearing.
>
> v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
> noticed a logic bug: When an underrun interrupt fires we report it
> both in the interrupt handler and when checking for underruns when
> disabling it in cpt_set_fifo_underrun_reporting. But that second check
> is only required if the interrupt is disabled and we're switching of
> underrun reporting (e.g. because we're disabling the crtc). Hence
> check for that condition.
>
> At first I wanted to rework the code to pass that bit of information
> from the uppper functions down to cpt_set_fifo_underrun_reporting. But
> that turned out too messy. Hence the quick&dirty check whether the
> south error interrupt source is masked off or not.
>
> v4: Streamline the control flow a bit.
>
> v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++----
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 80b88c8..bbf4df8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -215,18 +215,26 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>                                             bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);

You could move this declaration down to the "else" case so we don't
read the register when enable is true.


>
>         if (enable) {
> +               I915_WRITE(SERR_INT,
> +                          SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> +
>                 if (!cpt_can_enable_serr_int(dev))
>                         return;
>
> -               I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
> -                                    SERR_INT_TRANS_B_FIFO_UNDERRUN |
> -                                    SERR_INT_TRANS_C_FIFO_UNDERRUN);
> -
>                 ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
>         } else {
> +               uint32_t tmp = I915_READ(SERR_INT);
> +
>                 ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> +
> +               if (!was_enabled &&
> +                   (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
> +                       DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %i\n",

Shouldn't this be %c ?

> +                                     transcoder_name(pch_transcoder));
> +               }
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9b51be8..5bba39a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3881,6 +3881,7 @@
>  #define  SERR_INT_TRANS_C_FIFO_UNDERRUN        (1<<6)
>  #define  SERR_INT_TRANS_B_FIFO_UNDERRUN        (1<<3)
>  #define  SERR_INT_TRANS_A_FIFO_UNDERRUN        (1<<0)
> +#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)    (1<< (pipe*3))

Both checkpatch.pl and Paulo complain: ERROR: need consistent spacing
around '<<' (ctx:VxW)

The logic of the patch looks correct now, I think I finally understood it.

>
>  /* digital port hotplug */
>  #define PCH_PORT_HOTPLUG        0xc4030                /* SHOTPLUG_CTL */
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Daniel Vetter]

The current code won't report any fifo underruns on cpt if just one
pipe has fifo underrun reporting disabled. We can't enable the
interrupts, but we can still check the per-transcoder bits and so
report the underrun delayed if:
- We always clear the transcoder's bit (and none of the other bits)
  when enabling.
- We check the transcoder's bit after disabling (to avoid racing with
  the interrupt handler).

v2: I've forgotten to actually remove the old SERR_INT clearing.

v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
noticed a logic bug: When an underrun interrupt fires we report it
both in the interrupt handler and when checking for underruns when
disabling it in cpt_set_fifo_underrun_reporting. But that second check
is only required if the interrupt is disabled and we're switching of
underrun reporting (e.g. because we're disabling the crtc). Hence
check for that condition.

At first I wanted to rework the code to pass that bit of information
from the uppper functions down to cpt_set_fifo_underrun_reporting. But
that turned out too messy. Hence the quick&dirty check whether the
south error interrupt source is masked off or not.

v4: Streamline the control flow a bit.

v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.

v6: Review from Paulo:
- Reorder the was_enabled assignment to only read the register when we
  need it. Also add a comment that we need to do that before updating
  the register.
- s/%i/%c/ fix for the debug output.
- Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN
  #define.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 13 +++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d7d9afd..dd9d999 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -217,12 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (enable) {
+		I915_WRITE(SERR_INT,
+			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
+
 		if (!cpt_can_enable_serr_int(dev))
 			return;
 
 		ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	} else {
+		uint32_t tmp = I915_READ(SERR_INT);
+		bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
+
+		/* Change the state _after_ we've read out the current one. */
 		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
+
+		if (!was_enabled &&
+		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
+			DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
+				      transcoder_name(pch_transcoder));
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e9c50fa..7e2684f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3882,6 +3882,7 @@
 #define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
 #define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
 #define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
+#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)	(1<<(pipe*3))
 
 /* digital port hotplug */
 #define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
-- 
1.8.1.4

Re: [PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Paulo Zanoni]

2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current code won't report any fifo underruns on cpt if just one
> pipe has fifo underrun reporting disabled. We can't enable the
> interrupts, but we can still check the per-transcoder bits and so
> report the underrun delayed if:
> - We always clear the transcoder's bit (and none of the other bits)
>   when enabling.
> - We check the transcoder's bit after disabling (to avoid racing with
>   the interrupt handler).
>
> v2: I've forgotten to actually remove the old SERR_INT clearing.
>
> v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
> noticed a logic bug: When an underrun interrupt fires we report it
> both in the interrupt handler and when checking for underruns when
> disabling it in cpt_set_fifo_underrun_reporting. But that second check
> is only required if the interrupt is disabled and we're switching of
> underrun reporting (e.g. because we're disabling the crtc). Hence
> check for that condition.
>
> At first I wanted to rework the code to pass that bit of information
> from the uppper functions down to cpt_set_fifo_underrun_reporting. But
> that turned out too messy. Hence the quick&dirty check whether the
> south error interrupt source is masked off or not.
>
> v4: Streamline the control flow a bit.
>
> v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.
>
> v6: Review from Paulo:
> - Reorder the was_enabled assignment to only read the register when we
>   need it. Also add a comment that we need to do that before updating
>   the register.
> - s/%i/%c/ fix for the debug output.
> - Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN
>   #define.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d7d9afd..dd9d999 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -217,12 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         if (enable) {
> +               I915_WRITE(SERR_INT,
> +                          SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> +
>                 if (!cpt_can_enable_serr_int(dev))
>                         return;
>
>                 ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);

At this point, you should have the following lines:

		I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
				     SERR_INT_TRANS_C_FIFO_UNDERRUN);

Previous versions of this patch were removing these lines, but on this
patch these lines just don't exist. There's probably something wrong
with your branches.

>         } else {
> +               uint32_t tmp = I915_READ(SERR_INT);
> +               bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
> +
> +               /* Change the state _after_ we've read out the current one. */
>                 ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> +
> +               if (!was_enabled &&
> +                   (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
> +                       DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
> +                                     transcoder_name(pch_transcoder));
> +               }
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e9c50fa..7e2684f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3882,6 +3882,7 @@
>  #define  SERR_INT_TRANS_C_FIFO_UNDERRUN        (1<<6)
>  #define  SERR_INT_TRANS_B_FIFO_UNDERRUN        (1<<3)
>  #define  SERR_INT_TRANS_A_FIFO_UNDERRUN        (1<<0)
> +#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)    (1<<(pipe*3))
>
>  /* digital port hotplug */
>  #define PCH_PORT_HOTPLUG        0xc4030                /* SHOTPLUG_CTL */
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

[PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Daniel Vetter]

The current code won't report any fifo underruns on cpt if just one
pipe has fifo underrun reporting disabled. We can't enable the
interrupts, but we can still check the per-transcoder bits and so
report the underrun delayed if:
- We always clear the transcoder's bit (and none of the other bits)
  when enabling.
- We check the transcoder's bit after disabling (to avoid racing with
  the interrupt handler).

v2: I've forgotten to actually remove the old SERR_INT clearing.

v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
noticed a logic bug: When an underrun interrupt fires we report it
both in the interrupt handler and when checking for underruns when
disabling it in cpt_set_fifo_underrun_reporting. But that second check
is only required if the interrupt is disabled and we're switching of
underrun reporting (e.g. because we're disabling the crtc). Hence
check for that condition.

At first I wanted to rework the code to pass that bit of information
from the uppper functions down to cpt_set_fifo_underrun_reporting. But
that turned out too messy. Hence the quick&dirty check whether the
south error interrupt source is masked off or not.

v4: Streamline the control flow a bit.

v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.

v6: Review from Paulo:
- Reorder the was_enabled assignment to only read the register when we
  need it. Also add a comment that we need to do that before updating
  the register.
- s/%i/%c/ fix for the debug output.
- Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN
  #define.

v7: Hopefully put that elusive SERR hunk back into this patch, spotted
by Paulo.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 80b88c8..dd9d999 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -217,16 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (enable) {
+		I915_WRITE(SERR_INT,
+			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
+
 		if (!cpt_can_enable_serr_int(dev))
 			return;
 
-		I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
-				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
-				     SERR_INT_TRANS_C_FIFO_UNDERRUN);
-
 		ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	} else {
+		uint32_t tmp = I915_READ(SERR_INT);
+		bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
+
+		/* Change the state _after_ we've read out the current one. */
 		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
+
+		if (!was_enabled &&
+		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
+			DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
+				      transcoder_name(pch_transcoder));
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e9c50fa..7e2684f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3882,6 +3882,7 @@
 #define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
 #define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
 #define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
+#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)	(1<<(pipe*3))
 
 /* digital port hotplug */
 #define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
-- 
1.8.1.4

Re: [PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Paulo Zanoni]

2013/7/10 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current code won't report any fifo underruns on cpt if just one
> pipe has fifo underrun reporting disabled. We can't enable the
> interrupts, but we can still check the per-transcoder bits and so
> report the underrun delayed if:
> - We always clear the transcoder's bit (and none of the other bits)
>   when enabling.
> - We check the transcoder's bit after disabling (to avoid racing with
>   the interrupt handler).
>
> v2: I've forgotten to actually remove the old SERR_INT clearing.
>
> v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
> noticed a logic bug: When an underrun interrupt fires we report it
> both in the interrupt handler and when checking for underruns when
> disabling it in cpt_set_fifo_underrun_reporting. But that second check
> is only required if the interrupt is disabled and we're switching of
> underrun reporting (e.g. because we're disabling the crtc). Hence
> check for that condition.
>
> At first I wanted to rework the code to pass that bit of information
> from the uppper functions down to cpt_set_fifo_underrun_reporting. But
> that turned out too messy. Hence the quick&dirty check whether the
> south error interrupt source is masked off or not.
>
> v4: Streamline the control flow a bit.
>
> v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.
>
> v6: Review from Paulo:
> - Reorder the was_enabled assignment to only read the register when we
>   need it. Also add a comment that we need to do that before updating
>   the register.
> - s/%i/%c/ fix for the debug output.
> - Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN
>   #define.
>
> v7: Hopefully put that elusive SERR hunk back into this patch, spotted
> by Paulo.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++++++----
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 80b88c8..dd9d999 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -217,16 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         if (enable) {
> +               I915_WRITE(SERR_INT,
> +                          SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> +
>                 if (!cpt_can_enable_serr_int(dev))
>                         return;
>
> -               I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
> -                                    SERR_INT_TRANS_B_FIFO_UNDERRUN |
> -                                    SERR_INT_TRANS_C_FIFO_UNDERRUN);
> -
>                 ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
>         } else {
> +               uint32_t tmp = I915_READ(SERR_INT);
> +               bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
> +
> +               /* Change the state _after_ we've read out the current one. */
>                 ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> +
> +               if (!was_enabled &&
> +                   (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
> +                       DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
> +                                     transcoder_name(pch_transcoder));
> +               }
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e9c50fa..7e2684f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3882,6 +3882,7 @@
>  #define  SERR_INT_TRANS_C_FIFO_UNDERRUN        (1<<6)
>  #define  SERR_INT_TRANS_B_FIFO_UNDERRUN        (1<<3)
>  #define  SERR_INT_TRANS_A_FIFO_UNDERRUN        (1<<0)
> +#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)    (1<<(pipe*3))
>
>  /* digital port hotplug */
>  #define PCH_PORT_HOTPLUG        0xc4030                /* SHOTPLUG_CTL */
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

[PATCH 03/14] drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting

[Daniel Vetter]

Same treatment as for SERR_INT: If we clear only the bit for the pipe
we're enabling (but unconditionally) then we can always check for
possible underruns after having disabled the interrupt. That way pipe
underruns won't be lost, but at worst only get reported in a delayed
fashion.

v2: The same logic bug as in the SERR handling change also existed
here. The same bugfix of only reporting missed underruns when the
error interrupt was masked applies, too.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++++------
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bbf4df8..88e6e47 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -154,21 +154,26 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
 }
 
 static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
-						  bool enable)
+						  enum pipe pipe, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
 
 	if (enable) {
+		I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
+
 		if (!ivb_can_enable_err_int(dev))
 			return;
 
-		I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
-					 ERR_INT_FIFO_UNDERRUN_B |
-					 ERR_INT_FIFO_UNDERRUN_C);
-
 		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
 	} else {
 		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+
+		if (!was_enabled &&
+		    (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
+			DRM_DEBUG_KMS("uncleared fifo underrun on pipe %i\n",
+				      pipe_name(pipe));
+		}
 	}
 }
 
@@ -273,7 +278,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 	if (IS_GEN5(dev) || IS_GEN6(dev))
 		ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
 	else if (IS_GEN7(dev))
-		ivybridge_set_fifo_underrun_reporting(dev, enable);
+		ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
 
 done:
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5bba39a..58ca1d0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -681,6 +681,7 @@
 #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
 #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
 #define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
+#define   ERR_INT_FIFO_UNDERRUN(pipe)	(1<< (pipe*3))
 
 #define FPGA_DBG		0x42300
 #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
-- 
1.8.1.4

[PATCH] drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting

[Daniel Vetter]

Same treatment as for SERR_INT: If we clear only the bit for the pipe
we're enabling (but unconditionally) then we can always check for
possible underruns after having disabled the interrupt. That way pipe
underruns won't be lost, but at worst only get reported in a delayed
fashion.

v2: The same logic bug as in the SERR handling change also existed
here. The same bugfix of only reporting missed underruns when the
error interrupt was masked applies, too.

v3: Do the same fixes as for the SERR handling that Paulo suggested in
his review:
- s/%i/%c/ fix in the debug output
- move the DE_ERR_INT_IVB read into the respective if block

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++-------
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index dd9d999..76e977b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -154,21 +154,27 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
 }
 
 static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
-						  bool enable)
+						  enum pipe pipe, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	if (enable) {
+		I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
+
 		if (!ivb_can_enable_err_int(dev))
 			return;
 
-		I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
-					 ERR_INT_FIFO_UNDERRUN_B |
-					 ERR_INT_FIFO_UNDERRUN_C);
-
 		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
 	} else {
+		bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
+
+		/* Change the state _after_ we've read out the current one. */
 		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+
+		if (!was_enabled &&
+		    (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
+			DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
+				      pipe_name(pipe));
+		}
 	}
 }
 
@@ -274,7 +280,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 	if (IS_GEN5(dev) || IS_GEN6(dev))
 		ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
 	else if (IS_GEN7(dev))
-		ivybridge_set_fifo_underrun_reporting(dev, enable);
+		ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
 
 done:
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7e2684f..43e81c1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -681,6 +681,7 @@
 #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
 #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
 #define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
+#define   ERR_INT_FIFO_UNDERRUN(pipe)	(1<< (pipe*3))
 
 #define FPGA_DBG		0x42300
 #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
-- 
1.8.1.4

Re: [PATCH] drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting

[Paulo Zanoni]

2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Same treatment as for SERR_INT: If we clear only the bit for the pipe
> we're enabling (but unconditionally) then we can always check for
> possible underruns after having disabled the interrupt. That way pipe
> underruns won't be lost, but at worst only get reported in a delayed
> fashion.
>
> v2: The same logic bug as in the SERR handling change also existed
> here. The same bugfix of only reporting missed underruns when the
> error interrupt was masked applies, too.
>
> v3: Do the same fixes as for the SERR handling that Paulo suggested in
> his review:
> - s/%i/%c/ fix in the debug output
> - move the DE_ERR_INT_IVB read into the respective if block
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++-------
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index dd9d999..76e977b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -154,21 +154,27 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
>  }
>
>  static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
> -                                                 bool enable)
> +                                                 enum pipe pipe, bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -
>         if (enable) {
> +               I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
> +
>                 if (!ivb_can_enable_err_int(dev))
>                         return;
>
> -               I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
> -                                        ERR_INT_FIFO_UNDERRUN_B |
> -                                        ERR_INT_FIFO_UNDERRUN_C);
> -
>                 ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>         } else {
> +               bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
> +
> +               /* Change the state _after_ we've read out the current one. */
>                 ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +
> +               if (!was_enabled &&
> +                   (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
> +                       DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
> +                                     pipe_name(pipe));
> +               }
>         }
>  }
>
> @@ -274,7 +280,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>         if (IS_GEN5(dev) || IS_GEN6(dev))
>                 ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
>         else if (IS_GEN7(dev))
> -               ivybridge_set_fifo_underrun_reporting(dev, enable);
> +               ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
>
>  done:
>         spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7e2684f..43e81c1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -681,6 +681,7 @@
>  #define   ERR_INT_FIFO_UNDERRUN_C      (1<<6)
>  #define   ERR_INT_FIFO_UNDERRUN_B      (1<<3)
>  #define   ERR_INT_FIFO_UNDERRUN_A      (1<<0)
> +#define   ERR_INT_FIFO_UNDERRUN(pipe)  (1<< (pipe*3))

Checkpatch complains about inconsistent error spacing here.

With or without that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
>  #define FPGA_DBG               0x42300
>  #define   FPGA_DBG_RM_NOCLAIM  (1<<31)
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting

[Daniel Vetter]

On Wed, Jul 10, 2013 at 04:47:08PM -0300, Paulo Zanoni wrote:
> 2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Same treatment as for SERR_INT: If we clear only the bit for the pipe
> > we're enabling (but unconditionally) then we can always check for
> > possible underruns after having disabled the interrupt. That way pipe
> > underruns won't be lost, but at worst only get reported in a delayed
> > fashion.
> >
> > v2: The same logic bug as in the SERR handling change also existed
> > here. The same bugfix of only reporting missed underruns when the
> > error interrupt was masked applies, too.
> >
> > v3: Do the same fixes as for the SERR handling that Paulo suggested in
> > his review:
> > - s/%i/%c/ fix in the debug output
> > - move the DE_ERR_INT_IVB read into the respective if block
> >
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++-------
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index dd9d999..76e977b 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -154,21 +154,27 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
> >  }
> >
> >  static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
> > -                                                 bool enable)
> > +                                                 enum pipe pipe, bool enable)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> >         if (enable) {
> > +               I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
> > +
> >                 if (!ivb_can_enable_err_int(dev))
> >                         return;
> >
> > -               I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
> > -                                        ERR_INT_FIFO_UNDERRUN_B |
> > -                                        ERR_INT_FIFO_UNDERRUN_C);
> > -
> >                 ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> >         } else {
> > +               bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
> > +
> > +               /* Change the state _after_ we've read out the current one. */
> >                 ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> > +
> > +               if (!was_enabled &&
> > +                   (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
> > +                       DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
> > +                                     pipe_name(pipe));
> > +               }
> >         }
> >  }
> >
> > @@ -274,7 +280,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> >         if (IS_GEN5(dev) || IS_GEN6(dev))
> >                 ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
> >         else if (IS_GEN7(dev))
> > -               ivybridge_set_fifo_underrun_reporting(dev, enable);
> > +               ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
> >
> >  done:
> >         spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 7e2684f..43e81c1 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -681,6 +681,7 @@
> >  #define   ERR_INT_FIFO_UNDERRUN_C      (1<<6)
> >  #define   ERR_INT_FIFO_UNDERRUN_B      (1<<3)
> >  #define   ERR_INT_FIFO_UNDERRUN_A      (1<<0)
> > +#define   ERR_INT_FIFO_UNDERRUN(pipe)  (1<< (pipe*3))
> 
> Checkpatch complains about inconsistent error spacing here.

Oops, I've missed to apply this change from your review on the previous
patch. Will bikeshed when I apply the first part of this series tomorrow.
> 
> With or without that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for your review,
Daniel

> 
> >
> >  #define FPGA_DBG               0x42300
> >  #define   FPGA_DBG_RM_NOCLAIM  (1<<31)
> > --
> > 1.8.1.4
> >
> 
> 
> 
> -- 
> Paulo Zanoni

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

[PATCH 04/14] drm/i915: kill lpt pch transcoder->crtc mapping code for fifo underruns

[Daniel Vetter]

It's racy: There's no guarantee that we won't walk this code (due to a
pch fifo underrun interrupt) while someone is changing the pointers
around.

The only reason we do this is to use the righ crtc for the pch fifo
underrun accounting. But we never expose this to userspace, so
essentially no one really cares if we use the "wrong" crtc.

So let's just rip it out.

With this patch fifo underrun code will always use crtc A for tracking
underruns on the (only) pch transcoder on LPT.

v2: Add a big comment explaining what's going on. Requested by Paulo.

v3: Fixup spelling in comment as spotted by Paulo.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 88e6e47..0c09ae9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -201,13 +201,13 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 #define ibx_disable_display_interrupt(dev_priv, bits) \
 	ibx_display_interrupt_update((dev_priv), (bits), 0)
 
-static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
+static void ibx_set_fifo_underrun_reporting(struct drm_device *dev,
+					    enum transcoder pch_transcoder,
 					    bool enable)
 {
-	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER :
-						SDE_TRANSB_FIFO_UNDER;
+	uint32_t bit = (pch_transcoder == TRANSCODER_A) ?
+		       SDE_TRANSA_FIFO_UNDER : SDE_TRANSB_FIFO_UNDER;
 
 	if (enable)
 		ibx_enable_display_interrupt(dev_priv, bit);
@@ -304,29 +304,19 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 					   bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe p;
-	struct drm_crtc *crtc;
-	struct intel_crtc *intel_crtc;
+	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	unsigned long flags;
 	bool ret;
 
-	if (HAS_PCH_LPT(dev)) {
-		crtc = NULL;
-		for_each_pipe(p) {
-			struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p];
-			if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) {
-				crtc = c;
-				break;
-			}
-		}
-		if (!crtc) {
-			DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n");
-			return false;
-		}
-	} else {
-		crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
-	}
-	intel_crtc = to_intel_crtc(crtc);
+	/*
+	 * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT
+	 * has only one pch transcoder A that all pipes can use. To avoid racy
+	 * pch transcoder -> pipe lookups from interrupt code simply store the
+	 * underrun statistics in crtc A. Since we never expose this anywhere
+	 * nor use it outside of the fifo underrun code here using the "wrong"
+	 * crtc on LPT won't cause issues.
+	 */
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 
@@ -338,7 +328,7 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 	intel_crtc->pch_fifo_underrun_disabled = !enable;
 
 	if (HAS_PCH_IBX(dev))
-		ibx_set_fifo_underrun_reporting(intel_crtc, enable);
+		ibx_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
 	else
 		cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
 
-- 
1.8.1.4

Re: [PATCH 04/14] drm/i915: kill lpt pch transcoder->crtc mapping code for fifo underruns

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> It's racy: There's no guarantee that we won't walk this code (due to a
> pch fifo underrun interrupt) while someone is changing the pointers
> around.
>
> The only reason we do this is to use the righ crtc for the pch fifo
> underrun accounting. But we never expose this to userspace, so
> essentially no one really cares if we use the "wrong" crtc.
>
> So let's just rip it out.
>
> With this patch fifo underrun code will always use crtc A for tracking
> underruns on the (only) pch transcoder on LPT.
>
> v2: Add a big comment explaining what's going on. Requested by Paulo.
>
> v3: Fixup spelling in comment as spotted by Paulo.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++-------------------------
>  1 file changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 88e6e47..0c09ae9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -201,13 +201,13 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>  #define ibx_disable_display_interrupt(dev_priv, bits) \
>         ibx_display_interrupt_update((dev_priv), (bits), 0)
>
> -static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
> +static void ibx_set_fifo_underrun_reporting(struct drm_device *dev,
> +                                           enum transcoder pch_transcoder,
>                                             bool enable)
>  {
> -       struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER :
> -                                               SDE_TRANSB_FIFO_UNDER;
> +       uint32_t bit = (pch_transcoder == TRANSCODER_A) ?
> +                      SDE_TRANSA_FIFO_UNDER : SDE_TRANSB_FIFO_UNDER;
>
>         if (enable)
>                 ibx_enable_display_interrupt(dev_priv, bit);
> @@ -304,29 +304,19 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>                                            bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       enum pipe p;
> -       struct drm_crtc *crtc;
> -       struct intel_crtc *intel_crtc;
> +       struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
> +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         unsigned long flags;
>         bool ret;
>
> -       if (HAS_PCH_LPT(dev)) {
> -               crtc = NULL;
> -               for_each_pipe(p) {
> -                       struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p];
> -                       if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) {
> -                               crtc = c;
> -                               break;
> -                       }
> -               }
> -               if (!crtc) {
> -                       DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n");
> -                       return false;
> -               }
> -       } else {
> -               crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
> -       }
> -       intel_crtc = to_intel_crtc(crtc);
> +       /*
> +        * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT
> +        * has only one pch transcoder A that all pipes can use. To avoid racy
> +        * pch transcoder -> pipe lookups from interrupt code simply store the
> +        * underrun statistics in crtc A. Since we never expose this anywhere
> +        * nor use it outside of the fifo underrun code here using the "wrong"
> +        * crtc on LPT won't cause issues.
> +        */
>
>         spin_lock_irqsave(&dev_priv->irq_lock, flags);
>
> @@ -338,7 +328,7 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>         intel_crtc->pch_fifo_underrun_disabled = !enable;
>
>         if (HAS_PCH_IBX(dev))
> -               ibx_set_fifo_underrun_reporting(intel_crtc, enable);
> +               ibx_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
>         else
>                 cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
>
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

[PATCH 05/14] drm/i915: irq handlers don't need interrupt-safe spinlocks

[Daniel Vetter]

Since we only have one interrupt handler and interrupt handlers are
non-reentrant.

To drive the point really home give them all an _irq_handler suffix.

This is a tiny micro-optimization but even more important it makes it
clearer what locking we actually need. And in case someone screws this
up: lockdep will catch hardirq vs. other context deadlocks.

v2: Fix up compile fail.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 42 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0c09ae9..0e2663c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -654,14 +654,13 @@ static void i915_hotplug_work_func(struct work_struct *work)
 		drm_kms_helper_hotplug_event(dev);
 }
 
-static void ironlake_handle_rps_change(struct drm_device *dev)
+static void ironlake_rps_change_irq_handler(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 busy_up, busy_down, max_avg, min_avg;
 	u8 new_delay;
-	unsigned long flags;
 
-	spin_lock_irqsave(&mchdev_lock, flags);
+	spin_lock(&mchdev_lock);
 
 	I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
 
@@ -689,7 +688,7 @@ static void ironlake_handle_rps_change(struct drm_device *dev)
 	if (ironlake_set_drps(dev, new_delay))
 		dev_priv->ips.cur_delay = new_delay;
 
-	spin_unlock_irqrestore(&mchdev_lock, flags);
+	spin_unlock(&mchdev_lock);
 
 	return;
 }
@@ -833,18 +832,17 @@ static void ivybridge_parity_work(struct work_struct *work)
 	kfree(parity_event[1]);
 }
 
-static void ivybridge_handle_parity_error(struct drm_device *dev)
+static void ivybridge_parity_error_irq_handler(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	unsigned long flags;
 
 	if (!HAS_L3_GPU_CACHE(dev))
 		return;
 
-	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+	spin_lock(&dev_priv->irq_lock);
 	dev_priv->gt_irq_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
 	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+	spin_unlock(&dev_priv->irq_lock);
 
 	queue_work(dev_priv->wq, &dev_priv->l3_parity.error_work);
 }
@@ -870,15 +868,13 @@ static void snb_gt_irq_handler(struct drm_device *dev,
 	}
 
 	if (gt_iir & GT_RENDER_L3_PARITY_ERROR_INTERRUPT)
-		ivybridge_handle_parity_error(dev);
+		ivybridge_parity_error_irq_handler(dev);
 }
 
 /* Legacy way of handling PM interrupts */
-static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
-				u32 pm_iir)
+static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
+				 u32 pm_iir)
 {
-	unsigned long flags;
-
 	/*
 	 * IIR bits should never already be set because IMR should
 	 * prevent an interrupt from being shown in IIR. The warning
@@ -889,11 +885,11 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 	 * The mask bit in IMR is cleared by dev_priv->rps.work.
 	 */
 
-	spin_lock_irqsave(&dev_priv->rps.lock, flags);
+	spin_lock(&dev_priv->rps.lock);
 	dev_priv->rps.pm_iir |= pm_iir;
 	I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 	POSTING_READ(GEN6_PMIMR);
-	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
+	spin_unlock(&dev_priv->rps.lock);
 
 	queue_work(dev_priv->wq, &dev_priv->rps.work);
 }
@@ -957,7 +953,7 @@ static void dp_aux_irq_handler(struct drm_device *dev)
 	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
-/* Unlike gen6_queue_rps_work() from which this function is originally derived,
+/* Unlike gen6_rps_irq_handler() from which this function is originally derived,
  * we must be able to deal with other PM interrupts. This is complicated because
  * of the way in which we use the masks to defer the RPS work (which for
  * posterity is necessary because of forcewake).
@@ -965,9 +961,7 @@ static void dp_aux_irq_handler(struct drm_device *dev)
 static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 			       u32 pm_iir)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev_priv->rps.lock, flags);
+	spin_lock(&dev_priv->rps.lock);
 	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
 	if (dev_priv->rps.pm_iir) {
 		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
@@ -976,7 +970,7 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 		/* TODO: if queue_work is slow, move it out of the spinlock */
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
 	}
-	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
+	spin_unlock(&dev_priv->rps.lock);
 
 	if (pm_iir & ~GEN6_PM_RPS_EVENTS) {
 		if (pm_iir & PM_VEBOX_USER_INTERRUPT)
@@ -1058,7 +1052,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			gmbus_irq_handler(dev);
 
 		if (pm_iir & GEN6_PM_RPS_EVENTS)
-			gen6_queue_rps_work(dev_priv, pm_iir);
+			gen6_rps_irq_handler(dev_priv, pm_iir);
 
 		I915_WRITE(GTIIR, gt_iir);
 		I915_WRITE(GEN6_PMIIR, pm_iir);
@@ -1296,7 +1290,7 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 		if (IS_HASWELL(dev))
 			hsw_pm_irq_handler(dev_priv, pm_iir);
 		else if (pm_iir & GEN6_PM_RPS_EVENTS)
-			gen6_queue_rps_work(dev_priv, pm_iir);
+			gen6_rps_irq_handler(dev_priv, pm_iir);
 		I915_WRITE(GEN6_PMIIR, pm_iir);
 		ret = IRQ_HANDLED;
 	}
@@ -1413,10 +1407,10 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	}
 
 	if (IS_GEN5(dev) &&  de_iir & DE_PCU_EVENT)
-		ironlake_handle_rps_change(dev);
+		ironlake_rps_change_irq_handler(dev);
 
 	if (IS_GEN6(dev) && pm_iir & GEN6_PM_RPS_EVENTS)
-		gen6_queue_rps_work(dev_priv, pm_iir);
+		gen6_rps_irq_handler(dev_priv, pm_iir);
 
 	I915_WRITE(GTIIR, gt_iir);
 	I915_WRITE(DEIIR, de_iir);
-- 
1.8.1.4

[PATCH 06/14] drm/i915: streamline hsw_pm_irq_handler

[Daniel Vetter]

The if (pm_iir & ~GEN6_PM_RPS_EVENTS) check was redunandant. Otoh
adding a check for rps events allows us to avoid the spinlock grabbing
for VECS interrupts.

v2: Drop misplaced hunk which now moved to the right patch.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0e2663c..b1185e2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -961,25 +961,23 @@ static void dp_aux_irq_handler(struct drm_device *dev)
 static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 			       u32 pm_iir)
 {
-	spin_lock(&dev_priv->rps.lock);
-	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
-	if (dev_priv->rps.pm_iir) {
+	if (pm_iir & GEN6_PM_RPS_EVENTS) {
+		spin_lock(&dev_priv->rps.lock);
+		dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
 		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 		/* never want to mask useful interrupts. (also posting read) */
 		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
 		/* TODO: if queue_work is slow, move it out of the spinlock */
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
+		spin_unlock(&dev_priv->rps.lock);
 	}
-	spin_unlock(&dev_priv->rps.lock);
 
-	if (pm_iir & ~GEN6_PM_RPS_EVENTS) {
-		if (pm_iir & PM_VEBOX_USER_INTERRUPT)
-			notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
+	if (pm_iir & PM_VEBOX_USER_INTERRUPT)
+		notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
 
-		if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
-			DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
-			i915_handle_error(dev_priv->dev, false);
-		}
+	if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
+		DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
+		i915_handle_error(dev_priv->dev, false);
 	}
 }
 
-- 
1.8.1.4

[PATCH 07/14] drm/i915: queue work outside spinlock in hsw_pm_irq_handler

[Daniel Vetter]

And kill the comment about it. Queueing work is a barrier type event,
no amount of locking will help in ordering things (as long as we queue
the work after having updated all relevant data structures). Also, the
queue_work works itself as a sufficient memory barrier.

Again on the surface this is just a tiny micro-optimization to reduce
the hold-time of dev_priv->irq_lock. But the better reason is that it
reduces superficial locking and so makes it clearer what we actually
need for correctness.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1185e2..e70c5df 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -967,9 +967,9 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 		/* never want to mask useful interrupts. (also posting read) */
 		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
-		/* TODO: if queue_work is slow, move it out of the spinlock */
-		queue_work(dev_priv->wq, &dev_priv->rps.work);
 		spin_unlock(&dev_priv->rps.lock);
+
+		queue_work(dev_priv->wq, &dev_priv->rps.work);
 	}
 
 	if (pm_iir & PM_VEBOX_USER_INTERRUPT)
-- 
1.8.1.4

[PATCH 08/14] drm/i915: kill dev_priv->rps.lock

[Daniel Vetter]

Now that the rps interrupt locking isn't clearly separated (at elast
conceptually) from all the other interrupt locking having a different
lock stopped making sense: It protects much more than just the rps
workqueue it started out with. But with the addition of VECS the
separation started to blurr and resulted in some more complex locking
for the ring interrupt refcount.

With this we can (again) unifiy the ringbuffer irq refcounts without
causing a massive confusion, but that's for the next patch.

v2: Explain better why the rps.lock once made sense and why no longer,
requested by Ben.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c         |  1 -
 drivers/gpu/drm/i915/i915_drv.h         |  8 ++++----
 drivers/gpu/drm/i915/i915_irq.c         | 12 ++++++------
 drivers/gpu/drm/i915/intel_pm.c         | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++++----
 drivers/gpu/drm/i915/intel_ringbuffer.h |  2 +-
 6 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..d874dab 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1612,7 +1612,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->gpu_error.lock);
-	spin_lock_init(&dev_priv->rps.lock);
 	spin_lock_init(&dev_priv->backlight.lock);
 	mutex_init(&dev_priv->dpio_lock);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea86ad6..2179478 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -743,12 +743,12 @@ struct i915_suspend_saved_registers {
 };
 
 struct intel_gen6_power_mgmt {
+	/* work and pm_iir are protected by dev_priv->irq_lock */
 	struct work_struct work;
-	struct delayed_work vlv_work;
 	u32 pm_iir;
-	/* lock - irqsave spinlock that protectects the work_struct and
-	 * pm_iir. */
-	spinlock_t lock;
+
+	/* On vlv we need to manually drop to Vmin with a delayed work. */
+	struct delayed_work vlv_work;
 
 	/* The below variables an all the rps hw state are protected by
 	 * dev->struct mutext. */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e70c5df..27bf7c1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -717,13 +717,13 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	u32 pm_iir, pm_imr;
 	u8 new_delay;
 
-	spin_lock_irq(&dev_priv->rps.lock);
+	spin_lock_irq(&dev_priv->irq_lock);
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
 	pm_imr = I915_READ(GEN6_PMIMR);
 	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
 	I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS);
-	spin_unlock_irq(&dev_priv->rps.lock);
+	spin_unlock_irq(&dev_priv->irq_lock);
 
 	if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
 		return;
@@ -885,11 +885,11 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
 	 * The mask bit in IMR is cleared by dev_priv->rps.work.
 	 */
 
-	spin_lock(&dev_priv->rps.lock);
+	spin_lock(&dev_priv->irq_lock);
 	dev_priv->rps.pm_iir |= pm_iir;
 	I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 	POSTING_READ(GEN6_PMIMR);
-	spin_unlock(&dev_priv->rps.lock);
+	spin_unlock(&dev_priv->irq_lock);
 
 	queue_work(dev_priv->wq, &dev_priv->rps.work);
 }
@@ -962,12 +962,12 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 			       u32 pm_iir)
 {
 	if (pm_iir & GEN6_PM_RPS_EVENTS) {
-		spin_lock(&dev_priv->rps.lock);
+		spin_lock(&dev_priv->irq_lock);
 		dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
 		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 		/* never want to mask useful interrupts. (also posting read) */
 		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
-		spin_unlock(&dev_priv->rps.lock);
+		spin_unlock(&dev_priv->irq_lock);
 
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 125a741..a9be0d1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3135,9 +3135,9 @@ static void gen6_disable_rps(struct drm_device *dev)
 	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
 	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
 
-	spin_lock_irq(&dev_priv->rps.lock);
+	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->rps.pm_iir = 0;
-	spin_unlock_irq(&dev_priv->rps.lock);
+	spin_unlock_irq(&dev_priv->irq_lock);
 
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
 }
@@ -3154,9 +3154,9 @@ static void valleyview_disable_rps(struct drm_device *dev)
 	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
 	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
 
-	spin_lock_irq(&dev_priv->rps.lock);
+	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->rps.pm_iir = 0;
-	spin_unlock_irq(&dev_priv->rps.lock);
+	spin_unlock_irq(&dev_priv->irq_lock);
 
 	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
 
@@ -3321,13 +3321,13 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	/* requires MSI enabled */
 	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
-	spin_lock_irq(&dev_priv->rps.lock);
+	spin_lock_irq(&dev_priv->irq_lock);
 	/* FIXME: Our interrupt enabling sequence is bonghits.
 	 * dev_priv->rps.pm_iir really should be 0 here. */
 	dev_priv->rps.pm_iir = 0;
 	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
-	spin_unlock_irq(&dev_priv->rps.lock);
+	spin_unlock_irq(&dev_priv->irq_lock);
 	/* unmask all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
@@ -3601,10 +3601,10 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	/* requires MSI enabled */
 	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
-	spin_lock_irq(&dev_priv->rps.lock);
+	spin_lock_irq(&dev_priv->irq_lock);
 	WARN_ON(dev_priv->rps.pm_iir != 0);
 	I915_WRITE(GEN6_PMIMR, 0);
-	spin_unlock_irq(&dev_priv->rps.lock);
+	spin_unlock_irq(&dev_priv->irq_lock);
 	/* enable all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5b647eb..8a5874b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1071,14 +1071,14 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring)
 	if (!dev->irq_enabled)
 		return false;
 
-	spin_lock_irqsave(&dev_priv->rps.lock, flags);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount.pm++ == 0) {
 		u32 pm_imr = I915_READ(GEN6_PMIMR);
 		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
 		I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask);
 		POSTING_READ(GEN6_PMIMR);
 	}
-	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	return true;
 }
@@ -1093,14 +1093,14 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring)
 	if (!dev->irq_enabled)
 		return;
 
-	spin_lock_irqsave(&dev_priv->rps.lock, flags);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount.pm == 0) {
 		u32 pm_imr = I915_READ(GEN6_PMIMR);
 		I915_WRITE_IMR(ring, ~0);
 		I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask);
 		POSTING_READ(GEN6_PMIMR);
 	}
-	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 799f04c..8a87b3f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -80,7 +80,7 @@ struct  intel_ring_buffer {
 
 	struct {
 		u32	gt; /*  protected by dev_priv->irq_lock */
-		u32	pm; /*  protected by dev_priv->rps.lock (sucks) */
+		u32	pm; /*  protected by dev_priv->irq_lock */
 	} irq_refcount;
 	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
 	u32		trace_irq_seqno;
-- 
1.8.1.4

[PATCH 09/14] drm/i915: unify ring irq refcounts (again)

[Daniel Vetter]

With the simplified locking there's no reason any more to keep the
refcounts seperate.

v2: Readd the lost comment that ring->irq_refcount is protected by
dev_priv->irq_lock.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++++++----------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +----
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8a5874b..99d119c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -837,7 +837,7 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (ring->irq_refcount.gt++ == 0) {
+	if (ring->irq_refcount++ == 0) {
 		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
@@ -855,7 +855,7 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (--ring->irq_refcount.gt == 0) {
+	if (--ring->irq_refcount == 0) {
 		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
@@ -874,7 +874,7 @@ i9xx_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (ring->irq_refcount.gt++ == 0) {
+	if (ring->irq_refcount++ == 0) {
 		dev_priv->irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE(IMR, dev_priv->irq_mask);
 		POSTING_READ(IMR);
@@ -892,7 +892,7 @@ i9xx_ring_put_irq(struct intel_ring_buffer *ring)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (--ring->irq_refcount.gt == 0) {
+	if (--ring->irq_refcount == 0) {
 		dev_priv->irq_mask |= ring->irq_enable_mask;
 		I915_WRITE(IMR, dev_priv->irq_mask);
 		POSTING_READ(IMR);
@@ -911,7 +911,7 @@ i8xx_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (ring->irq_refcount.gt++ == 0) {
+	if (ring->irq_refcount++ == 0) {
 		dev_priv->irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE16(IMR, dev_priv->irq_mask);
 		POSTING_READ16(IMR);
@@ -929,7 +929,7 @@ i8xx_ring_put_irq(struct intel_ring_buffer *ring)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (--ring->irq_refcount.gt == 0) {
+	if (--ring->irq_refcount == 0) {
 		dev_priv->irq_mask |= ring->irq_enable_mask;
 		I915_WRITE16(IMR, dev_priv->irq_mask);
 		POSTING_READ16(IMR);
@@ -1022,7 +1022,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 	gen6_gt_force_wake_get(dev_priv);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (ring->irq_refcount.gt++ == 0) {
+	if (ring->irq_refcount++ == 0) {
 		if (HAS_L3_GPU_CACHE(dev) && ring->id == RCS)
 			I915_WRITE_IMR(ring,
 				       ~(ring->irq_enable_mask |
@@ -1046,7 +1046,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (--ring->irq_refcount.gt == 0) {
+	if (--ring->irq_refcount == 0) {
 		if (HAS_L3_GPU_CACHE(dev) && ring->id == RCS)
 			I915_WRITE_IMR(ring,
 				       ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
@@ -1072,7 +1072,7 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (ring->irq_refcount.pm++ == 0) {
+	if (ring->irq_refcount++ == 0) {
 		u32 pm_imr = I915_READ(GEN6_PMIMR);
 		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
 		I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask);
@@ -1094,7 +1094,7 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring)
 		return;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (--ring->irq_refcount.pm == 0) {
+	if (--ring->irq_refcount == 0) {
 		u32 pm_imr = I915_READ(GEN6_PMIMR);
 		I915_WRITE_IMR(ring, ~0);
 		I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 8a87b3f..6e38256 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -78,10 +78,7 @@ struct  intel_ring_buffer {
 	 */
 	u32		last_retired_head;
 
-	struct {
-		u32	gt; /*  protected by dev_priv->irq_lock */
-		u32	pm; /*  protected by dev_priv->irq_lock */
-	} irq_refcount;
+	unsigned irq_refcount; /* protected by dev_priv->irq_lock */
 	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
 	u32		trace_irq_seqno;
 	u32		sync_seqno[I915_NUM_RINGS-1];
-- 
1.8.1.4

[PATCH 10/14] drm/i915: don't enable PM_VEBOX_CS_ERROR_INTERRUPT

[Daniel Vetter]

The code to handle it is broken - there's simply no code to clear CS
parser errors on gen5+. And behold, for all the other rings we also
don't enable it!

Leave the handling code itself in place just to be consistent with the
existing mess though. And in case someone feels like fixing it all up.

This has been errornously enabled in

commit 12638c57f31952127c734c26315e1348fa1334c2
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue May 28 19:22:31 2013 -0700

    drm/i915: Enable vebox interrupts

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c         | 3 +--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 27bf7c1..910912b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2812,8 +2812,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 
 	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
 	if (HAS_VEBOX(dev))
-		pm_irqs |= PM_VEBOX_USER_INTERRUPT |
-			PM_VEBOX_CS_ERROR_INTERRUPT;
+		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
 	/* Our enable/disable rps functions may touch these registers so
 	 * make sure to set a known state for only the non-RPS bits.
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 99d119c..4a0d171 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2009,8 +2009,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 	ring->add_request = gen6_add_request;
 	ring->get_seqno = gen6_ring_get_seqno;
 	ring->set_seqno = ring_set_seqno;
-	ring->irq_enable_mask = PM_VEBOX_USER_INTERRUPT |
-		PM_VEBOX_CS_ERROR_INTERRUPT;
+	ring->irq_enable_mask = PM_VEBOX_USER_INTERRUPT;
 	ring->irq_get = hsw_vebox_get_irq;
 	ring->irq_put = hsw_vebox_put_irq;
 	ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
-- 
1.8.1.4

Re: [PATCH 10/14] drm/i915: don't enable PM_VEBOX_CS_ERROR_INTERRUPT

[Daniel Vetter]

On Thu, Jul 04, 2013 at 11:35:30PM +0200, Daniel Vetter wrote:
> The code to handle it is broken - there's simply no code to clear CS
> parser errors on gen5+. And behold, for all the other rings we also
> don't enable it!
> 
> Leave the handling code itself in place just to be consistent with the
> existing mess though. And in case someone feels like fixing it all up.
> 
> This has been errornously enabled in
> 
> commit 12638c57f31952127c734c26315e1348fa1334c2
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Tue May 28 19:22:31 2013 -0700
> 
>     drm/i915: Enable vebox interrupts
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok, I've merged the series up to this patch. Thanks to Ben and Paulo for
the review.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c         | 3 +--
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 27bf7c1..910912b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2812,8 +2812,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  
>  	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
>  	if (HAS_VEBOX(dev))
> -		pm_irqs |= PM_VEBOX_USER_INTERRUPT |
> -			PM_VEBOX_CS_ERROR_INTERRUPT;
> +		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
>  
>  	/* Our enable/disable rps functions may touch these registers so
>  	 * make sure to set a known state for only the non-RPS bits.
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 99d119c..4a0d171 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2009,8 +2009,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  	ring->add_request = gen6_add_request;
>  	ring->get_seqno = gen6_ring_get_seqno;
>  	ring->set_seqno = ring_set_seqno;
> -	ring->irq_enable_mask = PM_VEBOX_USER_INTERRUPT |
> -		PM_VEBOX_CS_ERROR_INTERRUPT;
> +	ring->irq_enable_mask = PM_VEBOX_USER_INTERRUPT;
>  	ring->irq_get = hsw_vebox_get_irq;
>  	ring->irq_put = hsw_vebox_put_irq;
>  	ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
> -- 
> 1.8.1.4
> 

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

[PATCH 11/14] drm/i915: unify PM interrupt preinstall sequence

[Daniel Vetter]

Since the addition of VECS we have a slightly different enable
sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
will end up in hard to track down surprises.

Hence unifiy things and since we have copies of this code in 3 places
now, extract it into its own little helper.

v3: Rebase on top of the retained double-GTIIR clearing. Also
resurrect the masking/disabling of the gen6+ PM interrupts as spotted
by Ben Widaswky.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 47 ++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 910912b..f4babaa 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2581,17 +2581,9 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
-/* drm_dma.h hooks
-*/
-static void ironlake_irq_preinstall(struct drm_device *dev)
+static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-
-	atomic_set(&dev_priv->irq_received, 0);
-
-	I915_WRITE(HWSTAM, 0xeffe);
-
-	/* XXX hotplug from PCH */
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	I915_WRITE(DEIMR, 0xffffffff);
 	I915_WRITE(DEIER, 0x0);
@@ -2602,6 +2594,26 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GTIER, 0x0);
 	POSTING_READ(GTIER);
 
+	if (INTEL_INFO(dev)->gen >= 6) {
+		/* and GT */
+		I915_WRITE(GEN6_PMIMR, 0xffffffff);
+		I915_WRITE(GEN6_PMIER, 0x0);
+		POSTING_READ(GEN6_PMIER);
+	}
+}
+
+/* drm_dma.h hooks
+*/
+static void ironlake_irq_preinstall(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
+	atomic_set(&dev_priv->irq_received, 0);
+
+	I915_WRITE(HWSTAM, 0xeffe);
+
+	gen5_gt_irq_preinstall(dev);
+
 	ibx_irq_preinstall(dev);
 }
 
@@ -2619,15 +2631,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(DEIER, 0x0);
 	POSTING_READ(DEIER);
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
-
-	/* Power management */
-	I915_WRITE(GEN6_PMIMR, 0xffffffff);
-	I915_WRITE(GEN6_PMIER, 0x0);
-	POSTING_READ(GEN6_PMIER);
+	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);
 }
@@ -2648,9 +2652,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	/* and GT */
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+
+	gen5_gt_irq_preinstall(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);
 
-- 
1.8.1.4

Re: [PATCH 11/14] drm/i915: unify PM interrupt preinstall sequence

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Since the addition of VECS we have a slightly different enable
> sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
> will end up in hard to track down surprises.
>
> Hence unifiy things and since we have copies of this code in 3 places
> now, extract it into its own little helper.
>
> v3: Rebase on top of the retained double-GTIIR clearing. Also
> resurrect the masking/disabling of the gen6+ PM interrupts as spotted
> by Ben Widaswky.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 47 ++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 910912b..f4babaa 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2581,17 +2581,9 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>         POSTING_READ(SDEIER);
>  }
>
> -/* drm_dma.h hooks
> -*/
> -static void ironlake_irq_preinstall(struct drm_device *dev)
> +static void gen5_gt_irq_preinstall(struct drm_device *dev)
>  {
> -       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -
> -       atomic_set(&dev_priv->irq_received, 0);
> -
> -       I915_WRITE(HWSTAM, 0xeffe);
> -
> -       /* XXX hotplug from PCH */
> +       struct drm_i915_private *dev_priv = dev->dev_private;
>
>         I915_WRITE(DEIMR, 0xffffffff);
>         I915_WRITE(DEIER, 0x0);

So now with your patch, gen5_gt_irq_preinstall is also responsible for
setting DEIMR+DEIER, but ivybridge_irq_preinstall still sets them, and
valleyview_irq_preinstall should not touch them. I guess this was an
accident.


> @@ -2602,6 +2594,26 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>         I915_WRITE(GTIER, 0x0);
>         POSTING_READ(GTIER);
>
> +       if (INTEL_INFO(dev)->gen >= 6) {
> +               /* and GT */
> +               I915_WRITE(GEN6_PMIMR, 0xffffffff);
> +               I915_WRITE(GEN6_PMIER, 0x0);
> +               POSTING_READ(GEN6_PMIER);
> +       }
> +}
> +
> +/* drm_dma.h hooks
> +*/
> +static void ironlake_irq_preinstall(struct drm_device *dev)
> +{
> +       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +
> +       atomic_set(&dev_priv->irq_received, 0);
> +
> +       I915_WRITE(HWSTAM, 0xeffe);
> +
> +       gen5_gt_irq_preinstall(dev);
> +
>         ibx_irq_preinstall(dev);
>  }
>
> @@ -2619,15 +2631,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
>         I915_WRITE(DEIER, 0x0);
>         POSTING_READ(DEIER);
>
> -       /* and GT */
> -       I915_WRITE(GTIMR, 0xffffffff);
> -       I915_WRITE(GTIER, 0x0);
> -       POSTING_READ(GTIER);
> -
> -       /* Power management */
> -       I915_WRITE(GEN6_PMIMR, 0xffffffff);
> -       I915_WRITE(GEN6_PMIER, 0x0);
> -       POSTING_READ(GEN6_PMIER);
> +       gen5_gt_irq_preinstall(dev);
>
>         ibx_irq_preinstall(dev);
>  }
> @@ -2648,9 +2652,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>         /* and GT */
>         I915_WRITE(GTIIR, I915_READ(GTIIR));
>         I915_WRITE(GTIIR, I915_READ(GTIIR));
> -       I915_WRITE(GTIMR, 0xffffffff);
> -       I915_WRITE(GTIER, 0x0);
> -       POSTING_READ(GTIER);
> +
> +       gen5_gt_irq_preinstall(dev);
>
>         I915_WRITE(DPINVGTT, 0xff);
>
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 11/14] drm/i915: unify PM interrupt preinstall sequence

[Daniel Vetter]

On Mon, Jul 08, 2013 at 02:06:50PM -0300, Paulo Zanoni wrote:
> 2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Since the addition of VECS we have a slightly different enable
> > sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
> > will end up in hard to track down surprises.
> >
> > Hence unifiy things and since we have copies of this code in 3 places
> > now, extract it into its own little helper.
> >
> > v3: Rebase on top of the retained double-GTIIR clearing. Also
> > resurrect the masking/disabling of the gen6+ PM interrupts as spotted
> > by Ben Widaswky.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 47 ++++++++++++++++++++++-------------------
> >  1 file changed, 25 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 910912b..f4babaa 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2581,17 +2581,9 @@ static void ibx_irq_preinstall(struct drm_device *dev)
> >         POSTING_READ(SDEIER);
> >  }
> >
> > -/* drm_dma.h hooks
> > -*/
> > -static void ironlake_irq_preinstall(struct drm_device *dev)
> > +static void gen5_gt_irq_preinstall(struct drm_device *dev)
> >  {
> > -       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > -
> > -       atomic_set(&dev_priv->irq_received, 0);
> > -
> > -       I915_WRITE(HWSTAM, 0xeffe);
> > -
> > -       /* XXX hotplug from PCH */
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> >
> >         I915_WRITE(DEIMR, 0xffffffff);
> >         I915_WRITE(DEIER, 0x0);
> 
> So now with your patch, gen5_gt_irq_preinstall is also responsible for
> setting DEIMR+DEIER, but ivybridge_irq_preinstall still sets them, and
> valleyview_irq_preinstall should not touch them. I guess this was an
> accident.

Oops, good catch. I've fixed things up now, will resend the patches after
a bit of testing shortly.

Thanks for your detailed review,
-Daniel

> 
> 
> > @@ -2602,6 +2594,26 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(GTIER, 0x0);
> >         POSTING_READ(GTIER);
> >
> > +       if (INTEL_INFO(dev)->gen >= 6) {
> > +               /* and GT */
> > +               I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > +               I915_WRITE(GEN6_PMIER, 0x0);
> > +               POSTING_READ(GEN6_PMIER);
> > +       }
> > +}
> > +
> > +/* drm_dma.h hooks
> > +*/
> > +static void ironlake_irq_preinstall(struct drm_device *dev)
> > +{
> > +       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > +
> > +       atomic_set(&dev_priv->irq_received, 0);
> > +
> > +       I915_WRITE(HWSTAM, 0xeffe);
> > +
> > +       gen5_gt_irq_preinstall(dev);
> > +
> >         ibx_irq_preinstall(dev);
> >  }
> >
> > @@ -2619,15 +2631,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(DEIER, 0x0);
> >         POSTING_READ(DEIER);
> >
> > -       /* and GT */
> > -       I915_WRITE(GTIMR, 0xffffffff);
> > -       I915_WRITE(GTIER, 0x0);
> > -       POSTING_READ(GTIER);
> > -
> > -       /* Power management */
> > -       I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > -       I915_WRITE(GEN6_PMIER, 0x0);
> > -       POSTING_READ(GEN6_PMIER);
> > +       gen5_gt_irq_preinstall(dev);
> >
> >         ibx_irq_preinstall(dev);
> >  }
> > @@ -2648,9 +2652,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
> >         /* and GT */
> >         I915_WRITE(GTIIR, I915_READ(GTIIR));
> >         I915_WRITE(GTIIR, I915_READ(GTIIR));
> > -       I915_WRITE(GTIMR, 0xffffffff);
> > -       I915_WRITE(GTIER, 0x0);
> > -       POSTING_READ(GTIER);
> > +
> > +       gen5_gt_irq_preinstall(dev);
> >
> >         I915_WRITE(DPINVGTT, 0xff);
> >
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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

[PATCH] drm/i915: unify PM interrupt preinstall sequence

[Daniel Vetter]

Since the addition of VECS we have a slightly different enable
sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
will end up in hard to track down surprises.

Hence unifiy things and since we have copies of this code in 3 places
now, extract it into its own little helper.

v3: Rebase on top of the retained double-GTIIR clearing. Also
resurrect the masking/disabling of the gen6+ PM interrupts as spotted
by Ben Widaswky.

v4: Move the DE interrupt reset code out of gen5_gt_irq_preinstall
back to ironlake_irq_preinstall where it really belongs. Spotted by
Paulo.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index cf1a21a..d5c3bef 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2583,6 +2583,23 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
+static void gen5_gt_irq_preinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* and GT */
+	I915_WRITE(GTIMR, 0xffffffff);
+	I915_WRITE(GTIER, 0x0);
+	POSTING_READ(GTIER);
+
+	if (INTEL_INFO(dev)->gen >= 6) {
+		/* and GT */
+		I915_WRITE(GEN6_PMIMR, 0xffffffff);
+		I915_WRITE(GEN6_PMIER, 0x0);
+		POSTING_READ(GEN6_PMIER);
+	}
+}
+
 /* drm_dma.h hooks
 */
 static void ironlake_irq_preinstall(struct drm_device *dev)
@@ -2593,16 +2610,11 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	/* XXX hotplug from PCH */
-
 	I915_WRITE(DEIMR, 0xffffffff);
 	I915_WRITE(DEIER, 0x0);
 	POSTING_READ(DEIER);
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);
 }
@@ -2621,15 +2633,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(DEIER, 0x0);
 	POSTING_READ(DEIER);
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
-
-	/* Power management */
-	I915_WRITE(GEN6_PMIMR, 0xffffffff);
-	I915_WRITE(GEN6_PMIER, 0x0);
-	POSTING_READ(GEN6_PMIER);
+	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);
 }
@@ -2650,9 +2654,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	/* and GT */
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+
+	gen5_gt_irq_preinstall(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);
 
-- 
1.8.1.4

Re: [PATCH] drm/i915: unify PM interrupt preinstall sequence

[Paulo Zanoni]

2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Since the addition of VECS we have a slightly different enable
> sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
> will end up in hard to track down surprises.
>
> Hence unifiy things and since we have copies of this code in 3 places
> now, extract it into its own little helper.
>
> v3: Rebase on top of the retained double-GTIIR clearing. Also
> resurrect the masking/disabling of the gen6+ PM interrupts as spotted
> by Ben Widaswky.
>
> v4: Move the DE interrupt reset code out of gen5_gt_irq_preinstall
> back to ironlake_irq_preinstall where it really belongs. Spotted by
> Paulo.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cf1a21a..d5c3bef 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2583,6 +2583,23 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>         POSTING_READ(SDEIER);
>  }
>
> +static void gen5_gt_irq_preinstall(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       /* and GT */
> +       I915_WRITE(GTIMR, 0xffffffff);
> +       I915_WRITE(GTIER, 0x0);
> +       POSTING_READ(GTIER);
> +
> +       if (INTEL_INFO(dev)->gen >= 6) {
> +               /* and GT */

This comment should be /* Power management */, but it's completely
useless, so you could kill it and also the /* and GT */ above.


> +               I915_WRITE(GEN6_PMIMR, 0xffffffff);
> +               I915_WRITE(GEN6_PMIER, 0x0);
> +               POSTING_READ(GEN6_PMIER);
> +       }
> +}
> +
>  /* drm_dma.h hooks
>  */
>  static void ironlake_irq_preinstall(struct drm_device *dev)
> @@ -2593,16 +2610,11 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>
>         I915_WRITE(HWSTAM, 0xeffe);
>
> -       /* XXX hotplug from PCH */
> -
>         I915_WRITE(DEIMR, 0xffffffff);
>         I915_WRITE(DEIER, 0x0);
>         POSTING_READ(DEIER);
>
> -       /* and GT */
> -       I915_WRITE(GTIMR, 0xffffffff);
> -       I915_WRITE(GTIER, 0x0);
> -       POSTING_READ(GTIER);
> +       gen5_gt_irq_preinstall(dev);

With this, we're now initializing GEN6_PM* on SandyBridge since it
uses ironlake_irq_preinstall. That sounds like a good thing. We could
either mention this in the commit message or do it in a separate
patch.


>
>         ibx_irq_preinstall(dev);
>  }
> @@ -2621,15 +2633,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
>         I915_WRITE(DEIER, 0x0);
>         POSTING_READ(DEIER);
>
> -       /* and GT */
> -       I915_WRITE(GTIMR, 0xffffffff);
> -       I915_WRITE(GTIER, 0x0);
> -       POSTING_READ(GTIER);
> -
> -       /* Power management */
> -       I915_WRITE(GEN6_PMIMR, 0xffffffff);
> -       I915_WRITE(GEN6_PMIER, 0x0);
> -       POSTING_READ(GEN6_PMIER);
> +       gen5_gt_irq_preinstall(dev);
>
>         ibx_irq_preinstall(dev);
>  }
> @@ -2650,9 +2654,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>         /* and GT */
>         I915_WRITE(GTIIR, I915_READ(GTIIR));
>         I915_WRITE(GTIIR, I915_READ(GTIIR));
> -       I915_WRITE(GTIMR, 0xffffffff);
> -       I915_WRITE(GTIER, 0x0);
> -       POSTING_READ(GTIER);
> +
> +       gen5_gt_irq_preinstall(dev);

Valleyview is Gen 7, right? So now we're also initializing GEN6_PMI*
on it. So same comments from SNB apply here. Maybe you should change
the commit title to "Initialize the PM interrupts on SNB and VLV".


>
>         I915_WRITE(DPINVGTT, 0xff);
>
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: unify PM interrupt preinstall sequence

[Daniel Vetter]

On Wed, Jul 10, 2013 at 05:05:07PM -0300, Paulo Zanoni wrote:
> 2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Since the addition of VECS we have a slightly different enable
> > sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
> > will end up in hard to track down surprises.
> >
> > Hence unifiy things and since we have copies of this code in 3 places
> > now, extract it into its own little helper.
> >
> > v3: Rebase on top of the retained double-GTIIR clearing. Also
> > resurrect the masking/disabling of the gen6+ PM interrupts as spotted
> > by Ben Widaswky.
> >
> > v4: Move the DE interrupt reset code out of gen5_gt_irq_preinstall
> > back to ironlake_irq_preinstall where it really belongs. Spotted by
> > Paulo.
> >
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++------------------
> >  1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index cf1a21a..d5c3bef 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2583,6 +2583,23 @@ static void ibx_irq_preinstall(struct drm_device *dev)
> >         POSTING_READ(SDEIER);
> >  }
> >
> > +static void gen5_gt_irq_preinstall(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       /* and GT */
> > +       I915_WRITE(GTIMR, 0xffffffff);
> > +       I915_WRITE(GTIER, 0x0);
> > +       POSTING_READ(GTIER);
> > +
> > +       if (INTEL_INFO(dev)->gen >= 6) {
> > +               /* and GT */
> 
> This comment should be /* Power management */, but it's completely
> useless, so you could kill it and also the /* and GT */ above.

Oh, I tend to ignore old comments freely if the contradict reality. I'll
update the patch and rip the all out (there's also similarly misleading
comments in the upper-level preinstall functions).

> > +               I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > +               I915_WRITE(GEN6_PMIER, 0x0);
> > +               POSTING_READ(GEN6_PMIER);
> > +       }
> > +}
> > +
> >  /* drm_dma.h hooks
> >  */
> >  static void ironlake_irq_preinstall(struct drm_device *dev)
> > @@ -2593,16 +2610,11 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
> >
> >         I915_WRITE(HWSTAM, 0xeffe);
> >
> > -       /* XXX hotplug from PCH */
> > -
> >         I915_WRITE(DEIMR, 0xffffffff);
> >         I915_WRITE(DEIER, 0x0);
> >         POSTING_READ(DEIER);
> >
> > -       /* and GT */
> > -       I915_WRITE(GTIMR, 0xffffffff);
> > -       I915_WRITE(GTIER, 0x0);
> > -       POSTING_READ(GTIER);
> > +       gen5_gt_irq_preinstall(dev);
> 
> With this, we're now initializing GEN6_PM* on SandyBridge since it
> uses ironlake_irq_preinstall. That sounds like a good thing. We could
> either mention this in the commit message or do it in a separate
> patch.
> 
> 
> >
> >         ibx_irq_preinstall(dev);
> >  }
> > @@ -2621,15 +2633,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(DEIER, 0x0);
> >         POSTING_READ(DEIER);
> >
> > -       /* and GT */
> > -       I915_WRITE(GTIMR, 0xffffffff);
> > -       I915_WRITE(GTIER, 0x0);
> > -       POSTING_READ(GTIER);
> > -
> > -       /* Power management */
> > -       I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > -       I915_WRITE(GEN6_PMIER, 0x0);
> > -       POSTING_READ(GEN6_PMIER);
> > +       gen5_gt_irq_preinstall(dev);
> >
> >         ibx_irq_preinstall(dev);
> >  }
> > @@ -2650,9 +2654,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
> >         /* and GT */
> >         I915_WRITE(GTIIR, I915_READ(GTIIR));
> >         I915_WRITE(GTIIR, I915_READ(GTIIR));
> > -       I915_WRITE(GTIMR, 0xffffffff);
> > -       I915_WRITE(GTIER, 0x0);
> > -       POSTING_READ(GTIER);
> > +
> > +       gen5_gt_irq_preinstall(dev);
> 
> Valleyview is Gen 7, right? So now we're also initializing GEN6_PMI*
> on it. So same comments from SNB apply here. Maybe you should change
> the commit title to "Initialize the PM interrupts on SNB and VLV".

Hm, that's actually the change I wanted to do to. Note that on SNB/VLV we
already initialize those interrupts in the gen6_enable_rps/vlv_enable_rps
functions. But unifying things completely takes a bit more than just one
patch.

Do you have a proposal for an improved commit message? I kinda run low on
ideas atm ..

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

Re: [PATCH] drm/i915: unify PM interrupt preinstall sequence

[Paulo Zanoni]

2013/7/10 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jul 10, 2013 at 05:05:07PM -0300, Paulo Zanoni wrote:
>> 2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> > Since the addition of VECS we have a slightly different enable
>> > sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
>> > will end up in hard to track down surprises.
>> >
>> > Hence unifiy things and since we have copies of this code in 3 places
>> > now, extract it into its own little helper.
>> >
>> > v3: Rebase on top of the retained double-GTIIR clearing. Also
>> > resurrect the masking/disabling of the gen6+ PM interrupts as spotted
>> > by Ben Widaswky.
>> >
>> > v4: Move the DE interrupt reset code out of gen5_gt_irq_preinstall
>> > back to ironlake_irq_preinstall where it really belongs. Spotted by
>> > Paulo.
>> >
>> > Cc: Paulo Zanoni <przanoni@gmail.com>
>> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > ---
>> >  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++------------------
>> >  1 file changed, 21 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index cf1a21a..d5c3bef 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -2583,6 +2583,23 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>> >         POSTING_READ(SDEIER);
>> >  }
>> >
>> > +static void gen5_gt_irq_preinstall(struct drm_device *dev)
>> > +{
>> > +       struct drm_i915_private *dev_priv = dev->dev_private;
>> > +
>> > +       /* and GT */
>> > +       I915_WRITE(GTIMR, 0xffffffff);
>> > +       I915_WRITE(GTIER, 0x0);
>> > +       POSTING_READ(GTIER);
>> > +
>> > +       if (INTEL_INFO(dev)->gen >= 6) {
>> > +               /* and GT */
>>
>> This comment should be /* Power management */, but it's completely
>> useless, so you could kill it and also the /* and GT */ above.
>
> Oh, I tend to ignore old comments freely if the contradict reality. I'll
> update the patch and rip the all out (there's also similarly misleading
> comments in the upper-level preinstall functions).
>
>> > +               I915_WRITE(GEN6_PMIMR, 0xffffffff);
>> > +               I915_WRITE(GEN6_PMIER, 0x0);
>> > +               POSTING_READ(GEN6_PMIER);
>> > +       }
>> > +}
>> > +
>> >  /* drm_dma.h hooks
>> >  */
>> >  static void ironlake_irq_preinstall(struct drm_device *dev)
>> > @@ -2593,16 +2610,11 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>> >
>> >         I915_WRITE(HWSTAM, 0xeffe);
>> >
>> > -       /* XXX hotplug from PCH */
>> > -
>> >         I915_WRITE(DEIMR, 0xffffffff);
>> >         I915_WRITE(DEIER, 0x0);
>> >         POSTING_READ(DEIER);
>> >
>> > -       /* and GT */
>> > -       I915_WRITE(GTIMR, 0xffffffff);
>> > -       I915_WRITE(GTIER, 0x0);
>> > -       POSTING_READ(GTIER);
>> > +       gen5_gt_irq_preinstall(dev);
>>
>> With this, we're now initializing GEN6_PM* on SandyBridge since it
>> uses ironlake_irq_preinstall. That sounds like a good thing. We could
>> either mention this in the commit message or do it in a separate
>> patch.
>>
>>
>> >
>> >         ibx_irq_preinstall(dev);
>> >  }
>> > @@ -2621,15 +2633,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
>> >         I915_WRITE(DEIER, 0x0);
>> >         POSTING_READ(DEIER);
>> >
>> > -       /* and GT */
>> > -       I915_WRITE(GTIMR, 0xffffffff);
>> > -       I915_WRITE(GTIER, 0x0);
>> > -       POSTING_READ(GTIER);
>> > -
>> > -       /* Power management */
>> > -       I915_WRITE(GEN6_PMIMR, 0xffffffff);
>> > -       I915_WRITE(GEN6_PMIER, 0x0);
>> > -       POSTING_READ(GEN6_PMIER);
>> > +       gen5_gt_irq_preinstall(dev);
>> >
>> >         ibx_irq_preinstall(dev);
>> >  }
>> > @@ -2650,9 +2654,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>> >         /* and GT */
>> >         I915_WRITE(GTIIR, I915_READ(GTIIR));
>> >         I915_WRITE(GTIIR, I915_READ(GTIIR));
>> > -       I915_WRITE(GTIMR, 0xffffffff);
>> > -       I915_WRITE(GTIER, 0x0);
>> > -       POSTING_READ(GTIER);
>> > +
>> > +       gen5_gt_irq_preinstall(dev);
>>
>> Valleyview is Gen 7, right? So now we're also initializing GEN6_PMI*
>> on it. So same comments from SNB apply here. Maybe you should change
>> the commit title to "Initialize the PM interrupts on SNB and VLV".
>
> Hm, that's actually the change I wanted to do to. Note that on SNB/VLV we
> already initialize those interrupts in the gen6_enable_rps/vlv_enable_rps
> functions. But unifying things completely takes a bit more than just one
> patch.
>
> Do you have a proposal for an improved commit message? I kinda run low on
> ideas atm ..

If just you read the commit message you might think that this patch
only moves code around without any bug-fixing. I guess you could just
say: "We're now initializing the PM interrupt registers on SNB and
VLV."


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



-- 
Paulo Zanoni

[PATCH 12/14] drm/i915: unify GT/PM irq postinstall code

[Daniel Vetter]

Again extract a common helper. For the postinstall hook things are a
bit more complicated since we have more cases on ilk-hsw/vlv here.

But since vlv was clearly broken by failing to initialize
dev_priv->gt_irq_mask correclty (mayb that explains the strange
RING_IMR clearing in the preinstall hook?) clearly justified the
shared code.

Also kill the PMIER setting in the async rps enable work. I should
have been save, but also clearly looked rather fragile.

With this we now have the usual interrupt register sequence for GT/PM
irq registers:

- IER is setup once with all the interrupts we ever need in the
  postinstall hook and never touched again. Exceptions are SDEIER,
  which is touched in the preinstall hook (when the irq handler isn't
  enabled) and then only from the irq handler. And DEIER/VLV_IER with
  is used in the irq handler but also written to once in the
  postinstall hook. But since that write is essentially what enables
  the interrupt and we should always have MSI interrupts we should be
  save. In case we ever have non-MSI interrupts we'd be screwed.

- IIR is cleared in the postinstall hook before we enable/unmask the
  respective interrupt sources. Hence we can't steal an interrupt
  event an accidentally trigger the spurious interrupt logic in the
  core kernel.

- IMR regs are (usually) all masked off. Those are the only regs
  changed at runtime, which is all protected by dev_priv->irq_lock.

This unification also kills the cargo-culted read-modify-write PM
register setup for VECS. Interrupt setup is done without userspace
being able to interfere, so we better know what values we want to put
into those registers. RMW cycles otoh are really good at papering over
races, until stuff magically blows up and no one has a clue why.

v2: Touch the gen6+ PM interrupt registers only on gen6+.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_pm.c |  4 --
 2 files changed, 42 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f4babaa..f4707a2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2723,6 +2723,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(SDEIMR, ~mask);
 }
 
+static void gen5_gt_irq_postinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 pm_irqs, gt_irqs;
+
+	pm_irqs = gt_irqs = 0;
+
+	dev_priv->gt_irq_mask = ~0;
+	if (HAS_L3_GPU_CACHE(dev)) {
+		dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+		gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+	}
+
+	gt_irqs |= GT_RENDER_USER_INTERRUPT;
+	if (IS_GEN5(dev)) {
+		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
+			   ILK_BSD_USER_INTERRUPT;
+	} else {
+		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
+	}
+
+	I915_WRITE(GTIIR, I915_READ(GTIIR));
+	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	I915_WRITE(GTIER, gt_irqs);
+	POSTING_READ(GTIER);
+
+	if (INTEL_INFO(dev)->gen >= 6) {
+		pm_irqs |= GEN6_PM_RPS_EVENTS;
+
+		if (HAS_VEBOX(dev))
+			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
+
+		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
+		I915_WRITE(GEN6_PMIMR, 0xffffffff);
+		I915_WRITE(GEN6_PMIER, pm_irqs);
+		POSTING_READ(GEN6_PMIER);
+	}
+}
+
 static int ironlake_irq_postinstall(struct drm_device *dev)
 {
 	unsigned long irqflags;
@@ -2733,7 +2772,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
 			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
 			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
-	u32 gt_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
 
@@ -2744,21 +2782,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 			  DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
 	POSTING_READ(DEIER);
 
-	dev_priv->gt_irq_mask = ~0;
-
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-
-	gt_irqs = GT_RENDER_USER_INTERRUPT;
-
-	if (IS_GEN6(dev))
-		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
-	else
-		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
-			   ILK_BSD_USER_INTERRUPT;
-
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	gen5_gt_irq_postinstall(dev);
 
 	ibx_irq_postinstall(dev);
 
@@ -2787,8 +2811,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		DE_PLANEA_FLIP_DONE_IVB |
 		DE_AUX_CHANNEL_A_IVB |
 		DE_ERR_INT_IVB;
-	u32 pm_irqs = GEN6_PM_RPS_EVENTS;
-	u32 gt_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
 
@@ -2803,30 +2825,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		   DE_PIPEA_VBLANK_IVB);
 	POSTING_READ(DEIER);
 
-	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-
-	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
-		  GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
-
-	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
-	if (HAS_VEBOX(dev))
-		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
-
-	/* Our enable/disable rps functions may touch these registers so
-	 * make sure to set a known state for only the non-RPS bits.
-	 * The RMW is extra paranoia since this should be called after being set
-	 * to a known state in preinstall.
-	 * */
-	I915_WRITE(GEN6_PMIMR,
-		   (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
-	I915_WRITE(GEN6_PMIER,
-		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
-	POSTING_READ(GEN6_PMIER);
+	gen5_gt_irq_postinstall(dev);
 
 	ibx_irq_postinstall(dev);
 
@@ -2836,7 +2835,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 static int valleyview_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	u32 gt_irqs;
 	u32 enable_mask;
 	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
 	unsigned long irqflags;
@@ -2876,13 +2874,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
 
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-
-	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
-		GT_BLT_USER_INTERRUPT;
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	gen5_gt_irq_postinstall(dev);
 
 	/* ack & enable invalid PTE error interrupts */
 #if 0 /* FIXME: add support to irq handler for checking these bits */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a9be0d1..96f0872 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
 
-	/* requires MSI enabled */
-	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
 	spin_lock_irq(&dev_priv->irq_lock);
 	/* FIXME: Our interrupt enabling sequence is bonghits.
 	 * dev_priv->rps.pm_iir really should be 0 here. */
@@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
-	/* requires MSI enabled */
-	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
 	spin_lock_irq(&dev_priv->irq_lock);
 	WARN_ON(dev_priv->rps.pm_iir != 0);
 	I915_WRITE(GEN6_PMIMR, 0);
-- 
1.8.1.4

Re: [PATCH 12/14] drm/i915: unify GT/PM irq postinstall code

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Again extract a common helper. For the postinstall hook things are a
> bit more complicated since we have more cases on ilk-hsw/vlv here.
>
> But since vlv was clearly broken by failing to initialize
> dev_priv->gt_irq_mask correclty (mayb that explains the strange
> RING_IMR clearing in the preinstall hook?) clearly justified the
> shared code.
>
> Also kill the PMIER setting in the async rps enable work. I should
> have been save, but also clearly looked rather fragile.
>
> With this we now have the usual interrupt register sequence for GT/PM
> irq registers:
>
> - IER is setup once with all the interrupts we ever need in the
>   postinstall hook and never touched again. Exceptions are SDEIER,
>   which is touched in the preinstall hook (when the irq handler isn't
>   enabled) and then only from the irq handler. And DEIER/VLV_IER with
>   is used in the irq handler but also written to once in the
>   postinstall hook. But since that write is essentially what enables
>   the interrupt and we should always have MSI interrupts we should be
>   save. In case we ever have non-MSI interrupts we'd be screwed.
>
> - IIR is cleared in the postinstall hook before we enable/unmask the
>   respective interrupt sources. Hence we can't steal an interrupt
>   event an accidentally trigger the spurious interrupt logic in the
>   core kernel.
>
> - IMR regs are (usually) all masked off. Those are the only regs
>   changed at runtime, which is all protected by dev_priv->irq_lock.
>
> This unification also kills the cargo-culted read-modify-write PM
> register setup for VECS. Interrupt setup is done without userspace
> being able to interfere, so we better know what values we want to put
> into those registers. RMW cycles otoh are really good at papering over
> races, until stuff magically blows up and no one has a clue why.
>
> v2: Touch the gen6+ PM interrupt registers only on gen6+.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
>  drivers/gpu/drm/i915/intel_pm.c |  4 --
>  2 files changed, 42 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f4babaa..f4707a2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2723,6 +2723,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>         I915_WRITE(SDEIMR, ~mask);
>  }
>
> +static void gen5_gt_irq_postinstall(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       u32 pm_irqs, gt_irqs;
> +
> +       pm_irqs = gt_irqs = 0;
> +
> +       dev_priv->gt_irq_mask = ~0;
> +       if (HAS_L3_GPU_CACHE(dev)) {
> +               dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> +               gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> +       }
> +
> +       gt_irqs |= GT_RENDER_USER_INTERRUPT;
> +       if (IS_GEN5(dev)) {
> +               gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> +                          ILK_BSD_USER_INTERRUPT;
> +       } else {
> +               gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> +       }
> +
> +       I915_WRITE(GTIIR, I915_READ(GTIIR));
> +       I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +       I915_WRITE(GTIER, gt_irqs);
> +       POSTING_READ(GTIER);
> +
> +       if (INTEL_INFO(dev)->gen >= 6) {
> +               pm_irqs |= GEN6_PM_RPS_EVENTS;
> +
> +               if (HAS_VEBOX(dev))
> +                       pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> +
> +               I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> +               I915_WRITE(GEN6_PMIMR, 0xffffffff);
> +               I915_WRITE(GEN6_PMIER, pm_irqs);
> +               POSTING_READ(GEN6_PMIER);
> +       }
> +}
> +
>  static int ironlake_irq_postinstall(struct drm_device *dev)
>  {
>         unsigned long irqflags;
> @@ -2733,7 +2772,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>                            DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
>                            DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
>                            DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
> -       u32 gt_irqs;
>
>         dev_priv->irq_mask = ~display_mask;
>
> @@ -2744,21 +2782,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>                           DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
>         POSTING_READ(DEIER);
>
> -       dev_priv->gt_irq_mask = ~0;
> -
> -       I915_WRITE(GTIIR, I915_READ(GTIIR));
> -       I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -
> -       gt_irqs = GT_RENDER_USER_INTERRUPT;
> -
> -       if (IS_GEN6(dev))
> -               gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> -       else
> -               gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> -                          ILK_BSD_USER_INTERRUPT;
> -
> -       I915_WRITE(GTIER, gt_irqs);
> -       POSTING_READ(GTIER);
> +       gen5_gt_irq_postinstall(dev);

Which means we're now initializing GEN6_PM* code on SNB, which seems
good. You might want to dedicate a paragraph for this on the commit
message.

On the IRQ patches I wrote (but did not sent yet) I unified the
ILK/SNB irq_handler vfuncs with IVB/HSW ones. I guess bugs like the
one you've just fixed here and in the previous patch are a good way to
justify my patches :)

>
>         ibx_irq_postinstall(dev);
>
> @@ -2787,8 +2811,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>                 DE_PLANEA_FLIP_DONE_IVB |
>                 DE_AUX_CHANNEL_A_IVB |
>                 DE_ERR_INT_IVB;
> -       u32 pm_irqs = GEN6_PM_RPS_EVENTS;
> -       u32 gt_irqs;
>
>         dev_priv->irq_mask = ~display_mask;
>
> @@ -2803,30 +2825,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>                    DE_PIPEA_VBLANK_IVB);
>         POSTING_READ(DEIER);
>
> -       dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> -
> -       I915_WRITE(GTIIR, I915_READ(GTIIR));
> -       I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -
> -       gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> -                 GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> -       I915_WRITE(GTIER, gt_irqs);
> -       POSTING_READ(GTIER);
> -
> -       I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> -       if (HAS_VEBOX(dev))
> -               pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> -
> -       /* Our enable/disable rps functions may touch these registers so
> -        * make sure to set a known state for only the non-RPS bits.
> -        * The RMW is extra paranoia since this should be called after being set
> -        * to a known state in preinstall.
> -        * */
> -       I915_WRITE(GEN6_PMIMR,
> -                  (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
> -       I915_WRITE(GEN6_PMIER,
> -                  (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
> -       POSTING_READ(GEN6_PMIER);
> +       gen5_gt_irq_postinstall(dev);
>
>         ibx_irq_postinstall(dev);
>
> @@ -2836,7 +2835,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  static int valleyview_irq_postinstall(struct drm_device *dev)
>  {
>         drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -       u32 gt_irqs;
>         u32 enable_mask;
>         u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
>         unsigned long irqflags;
> @@ -2876,13 +2874,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
>         I915_WRITE(VLV_IIR, 0xffffffff);
>         I915_WRITE(VLV_IIR, 0xffffffff);
>
> -       I915_WRITE(GTIIR, I915_READ(GTIIR));
> -       I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -
> -       gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> -               GT_BLT_USER_INTERRUPT;
> -       I915_WRITE(GTIER, gt_irqs);
> -       POSTING_READ(GTIER);
> +       gen5_gt_irq_postinstall(dev);

Besides fixing the uninitialized mask, we're also now initializing
GEN6_PM* regs on VLV. You should also mention this explicitly.

This patch contains a few bug fixes. I certainly won't complain if you
split them into separate commits, and then merge the code in the final
commit. But I can live without that, it's your choice :)

>
>         /* ack & enable invalid PTE error interrupts */
>  #if 0 /* FIXME: add support to irq handler for checking these bits */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a9be0d1..96f0872 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
>
>         gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>
> -       /* requires MSI enabled */
> -       I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);

It's even outside the irq_lock :)
Do we want to at least read the bit and WARN in case it's not enabled?

>         spin_lock_irq(&dev_priv->irq_lock);
>         /* FIXME: Our interrupt enabling sequence is bonghits.
>          * dev_priv->rps.pm_iir really should be 0 here. */
> @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
>
>         valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>
> -       /* requires MSI enabled */
> -       I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
>         spin_lock_irq(&dev_priv->irq_lock);
>         WARN_ON(dev_priv->rps.pm_iir != 0);
>         I915_WRITE(GEN6_PMIMR, 0);
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 12/14] drm/i915: unify GT/PM irq postinstall code

[Daniel Vetter]

On Wed, Jul 10, 2013 at 10:48 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Which means we're now initializing GEN6_PM* code on SNB, which seems
> good. You might want to dedicate a paragraph for this on the commit
> message.
>
> On the IRQ patches I wrote (but did not sent yet) I unified the
> ILK/SNB irq_handler vfuncs with IVB/HSW ones. I guess bugs like the
> one you've just fixed here and in the previous patch are a good way to
> justify my patches :)

I think we have a language communication fail going on here in these
few patches. With "unify" I don't mean "extract identical code, simple
refactoring with no functional change but "make them work the same way
since currently they don't".

Note that with the exception of the gt_irq_mask initialization on vlv
there's no bugfix in here: PM interrupte setup simply works
differently on snb/vlv than on ivb/hsw after Ben's VECS enabling
patches. What these few patches here try to do is unify the sequences
again so all platforms set up the PM registers the same way.

Note that the PMIER update in gen6_enable_rps isn't a bugfix either:
The interrupt handler doesn't touch this register. The only other
place is the hsw vecs interrupt setup in the ivybridge irq vfuncs, but
those two codepaths can never run in parrallel due to init/teardown
sequence ordering (even though that gen6_enable_rps runs from a work
item).

I've read through the commit message again and for me it seems to be
clear what's going on (but I'm obviously biased). So again, do you
have ideas for improvements? Wrt patch splitting I'm not sure whether
it's worth it, since the current code is simply a bit a confusing mess
imo.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/3] drm/i915: unify PM interrupt preinstall sequence

[Daniel Vetter]

Since the addition of VECS we have a slightly different enable
sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
will end up in hard to track down surprises.

Hence unifiy things and since we have copies of this code in 3 places
now, extract it into its own little helper.

Note that this changes the irq preinstall sequence a bit for snb and
vlv: We now also clear the PM registers in the preinstall hook, in
addition to the PM register clearing/setup already done when actually
enabling rps. So this doesn't fix a bug but simply unifies the code
across all platforms. After the postinstall hook is similarly unified
we can rip out the then redundant PM interrupt setup from the rps
code.

v3: Rebase on top of the retained double-GTIIR clearing. Also
resurrect the masking/disabling of the gen6+ PM interrupts as spotted
by Ben Widaswky.

v4: Move the DE interrupt reset code out of gen5_gt_irq_preinstall
back to ironlake_irq_preinstall where it really belongs. Spotted by
Paulo.

v3: Improve the commit message to more clearly spell out why we want
to unify the code and what exactly changes.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index cf1a21a..d5c3bef 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2583,6 +2583,23 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
+static void gen5_gt_irq_preinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* and GT */
+	I915_WRITE(GTIMR, 0xffffffff);
+	I915_WRITE(GTIER, 0x0);
+	POSTING_READ(GTIER);
+
+	if (INTEL_INFO(dev)->gen >= 6) {
+		/* and GT */
+		I915_WRITE(GEN6_PMIMR, 0xffffffff);
+		I915_WRITE(GEN6_PMIER, 0x0);
+		POSTING_READ(GEN6_PMIER);
+	}
+}
+
 /* drm_dma.h hooks
 */
 static void ironlake_irq_preinstall(struct drm_device *dev)
@@ -2593,16 +2610,11 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	/* XXX hotplug from PCH */
-
 	I915_WRITE(DEIMR, 0xffffffff);
 	I915_WRITE(DEIER, 0x0);
 	POSTING_READ(DEIER);
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);
 }
@@ -2621,15 +2633,7 @@ static void ivybridge_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(DEIER, 0x0);
 	POSTING_READ(DEIER);
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
-
-	/* Power management */
-	I915_WRITE(GEN6_PMIMR, 0xffffffff);
-	I915_WRITE(GEN6_PMIER, 0x0);
-	POSTING_READ(GEN6_PMIER);
+	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);
 }
@@ -2650,9 +2654,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	/* and GT */
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+
+	gen5_gt_irq_preinstall(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);
 
-- 
1.8.1.4

[PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Daniel Vetter]

Again extract a common helper. For the postinstall hook things are a
bit more complicated since we have more cases on ilk-hsw/vlv here.

But since vlv was clearly broken by failing to initialize
dev_priv->gt_irq_mask correctly the shared code is clearly justified.

Also kill the PMIER setting in the async rps enable work. I should
have been save, but also clearly looked rather fragile. PMIER setup is
not all down in the irq pre/postinstall hooks.

With this we now have the usual interrupt register sequence for GT/PM
irq registers:

- IER is setup once with all the interrupts we ever need in the
  postinstall hook and never touched again. Exceptions are SDEIER,
  which is touched in the preinstall hook (when the irq handler isn't
  enabled) and then only from the irq handler. And DEIER/VLV_IER with
  is used in the irq handler but also written to once in the
  postinstall hook. But since that write is essentially what enables
  the interrupt and we should always have MSI interrupts we should be
  save. In case we ever have non-MSI interrupts we'd be screwed.

- IIR is cleared in the postinstall hook before we enable/unmask the
  respective interrupt sources. Hence we can't steal an interrupt
  event an accidentally trigger the spurious interrupt logic in the
  core kernel. Note that after some discussion with Ben Widawsky we
  think that we actually should clear the IIR registers in the
  preinstall hook. But doing that is a much larger patch series.

- IMR regs are (usually) all masked off. Those are the only regs
  changed at runtime, which is all protected by dev_priv->irq_lock.

This unification also kills the cargo-culted read-modify-write PM
register setup for VECS. Interrupt setup is done without userspace
being able to interfere, so we better know what values we want to put
into those registers. RMW cycles otoh are really good at papering over
races, until stuff magically blows up and no one has a clue why.

v2: Touch the gen6+ PM interrupt registers only on gen6+.

v3: Improve the commit message to more clearly spell out why we want
to unify the code and what exactly changes.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_pm.c |  4 --
 2 files changed, 42 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d5c3bef..ba61bc7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(SDEIMR, ~mask);
 }
 
+static void gen5_gt_irq_postinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 pm_irqs, gt_irqs;
+
+	pm_irqs = gt_irqs = 0;
+
+	dev_priv->gt_irq_mask = ~0;
+	if (HAS_L3_GPU_CACHE(dev)) {
+		dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+		gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+	}
+
+	gt_irqs |= GT_RENDER_USER_INTERRUPT;
+	if (IS_GEN5(dev)) {
+		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
+			   ILK_BSD_USER_INTERRUPT;
+	} else {
+		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
+	}
+
+	I915_WRITE(GTIIR, I915_READ(GTIIR));
+	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	I915_WRITE(GTIER, gt_irqs);
+	POSTING_READ(GTIER);
+
+	if (INTEL_INFO(dev)->gen >= 6) {
+		pm_irqs |= GEN6_PM_RPS_EVENTS;
+
+		if (HAS_VEBOX(dev))
+			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
+
+		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
+		I915_WRITE(GEN6_PMIMR, 0xffffffff);
+		I915_WRITE(GEN6_PMIER, pm_irqs);
+		POSTING_READ(GEN6_PMIER);
+	}
+}
+
 static int ironlake_irq_postinstall(struct drm_device *dev)
 {
 	unsigned long irqflags;
@@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
 			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
 			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
-	u32 gt_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
 
@@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 			  DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
 	POSTING_READ(DEIER);
 
-	dev_priv->gt_irq_mask = ~0;
-
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-
-	gt_irqs = GT_RENDER_USER_INTERRUPT;
-
-	if (IS_GEN6(dev))
-		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
-	else
-		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
-			   ILK_BSD_USER_INTERRUPT;
-
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	gen5_gt_irq_postinstall(dev);
 
 	ibx_irq_postinstall(dev);
 
@@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		DE_PLANEA_FLIP_DONE_IVB |
 		DE_AUX_CHANNEL_A_IVB |
 		DE_ERR_INT_IVB;
-	u32 pm_irqs = GEN6_PM_RPS_EVENTS;
-	u32 gt_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
 
@@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		   DE_PIPEA_VBLANK_IVB);
 	POSTING_READ(DEIER);
 
-	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-
-	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
-		  GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
-
-	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
-	if (HAS_VEBOX(dev))
-		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
-
-	/* Our enable/disable rps functions may touch these registers so
-	 * make sure to set a known state for only the non-RPS bits.
-	 * The RMW is extra paranoia since this should be called after being set
-	 * to a known state in preinstall.
-	 * */
-	I915_WRITE(GEN6_PMIMR,
-		   (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
-	I915_WRITE(GEN6_PMIER,
-		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
-	POSTING_READ(GEN6_PMIER);
+	gen5_gt_irq_postinstall(dev);
 
 	ibx_irq_postinstall(dev);
 
@@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 static int valleyview_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	u32 gt_irqs;
 	u32 enable_mask;
 	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
 	unsigned long irqflags;
@@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
 
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-
-	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
-		GT_BLT_USER_INTERRUPT;
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	gen5_gt_irq_postinstall(dev);
 
 	/* ack & enable invalid PTE error interrupts */
 #if 0 /* FIXME: add support to irq handler for checking these bits */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fb4afaa..e609232 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
 
-	/* requires MSI enabled */
-	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
 	spin_lock_irq(&dev_priv->irq_lock);
 	/* FIXME: Our interrupt enabling sequence is bonghits.
 	 * dev_priv->rps.pm_iir really should be 0 here. */
@@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
-	/* requires MSI enabled */
-	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
 	spin_lock_irq(&dev_priv->irq_lock);
 	WARN_ON(dev_priv->rps.pm_iir != 0);
 	I915_WRITE(GEN6_PMIMR, 0);
-- 
1.8.1.4

Re: [PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Ben Widawsky]

On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote:
> Again extract a common helper. For the postinstall hook things are a
> bit more complicated since we have more cases on ilk-hsw/vlv here.
> 
> But since vlv was clearly broken by failing to initialize
> dev_priv->gt_irq_mask correctly the shared code is clearly justified.
> 
> Also kill the PMIER setting in the async rps enable work. I should
> have been save, but also clearly looked rather fragile. PMIER setup is
Can you fix up this sentence. It's a bit weird, and I'm not positive
what you're trying to say.
> not all down in the irq pre/postinstall hooks.
> 
> With this we now have the usual interrupt register sequence for GT/PM
> irq registers:
> 
> - IER is setup once with all the interrupts we ever need in the
>   postinstall hook and never touched again. Exceptions are SDEIER,
>   which is touched in the preinstall hook (when the irq handler isn't
>   enabled) and then only from the irq handler. And DEIER/VLV_IER with
>   is used in the irq handler but also written to once in the
>   postinstall hook. But since that write is essentially what enables
>   the interrupt and we should always have MSI interrupts we should be
>   save. In case we ever have non-MSI interrupts we'd be screwed.
> 
> - IIR is cleared in the postinstall hook before we enable/unmask the
>   respective interrupt sources. Hence we can't steal an interrupt
>   event an accidentally trigger the spurious interrupt logic in the
>   core kernel. Note that after some discussion with Ben Widawsky we
>   think that we actually should clear the IIR registers in the
>   preinstall hook. But doing that is a much larger patch series.
> 
> - IMR regs are (usually) all masked off. Those are the only regs
>   changed at runtime, which is all protected by dev_priv->irq_lock.
> 

This comment block is really nice. Maybe supplement it with explanation
about how the ring interrupts work, and shove it in i915_irq.c?

> This unification also kills the cargo-culted read-modify-write PM
> register setup for VECS. Interrupt setup is done without userspace
> being able to interfere, so we better know what values we want to put
> into those registers. RMW cycles otoh are really good at papering over
> races, until stuff magically blows up and no one has a clue why.
> 
> v2: Touch the gen6+ PM interrupt registers only on gen6+.
> 
> v3: Improve the commit message to more clearly spell out why we want
> to unify the code and what exactly changes.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
>  drivers/gpu/drm/i915/intel_pm.c |  4 --
>  2 files changed, 42 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d5c3bef..ba61bc7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  	I915_WRITE(SDEIMR, ~mask);
>  }
>  
> +static void gen5_gt_irq_postinstall(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pm_irqs, gt_irqs;
> +
> +	pm_irqs = gt_irqs = 0;
> +
> +	dev_priv->gt_irq_mask = ~0;
> +	if (HAS_L3_GPU_CACHE(dev)) {
> +		dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> +		gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> +	}

Maybe while you're doing this, explain why the L3 parity interrupt is
special, in a comment. It's the only one to touch dev_priv->gt_irq_mask

> +
> +	gt_irqs |= GT_RENDER_USER_INTERRUPT;
> +	if (IS_GEN5(dev)) {
> +		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> +			   ILK_BSD_USER_INTERRUPT;
> +	} else {
> +		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> +	}
> +
> +	I915_WRITE(GTIIR, I915_READ(GTIIR));
> +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	I915_WRITE(GTIER, gt_irqs);
> +	POSTING_READ(GTIER);
> +
> +	if (INTEL_INFO(dev)->gen >= 6) {
> +		pm_irqs |= GEN6_PM_RPS_EVENTS;
> +
> +		if (HAS_VEBOX(dev))
> +			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> +
> +		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> +		I915_WRITE(GEN6_PMIMR, 0xffffffff);
> +		I915_WRITE(GEN6_PMIER, pm_irqs);
> +		POSTING_READ(GEN6_PMIER);
> +	}
> +}
> +

The ordering still looks funky here to me and your comment in the commit
message hasn't convinced me otherwise. The order should be:
MASK
CLEAR
ENABLE

In your order there is a theoretical race after you've cleared, but
before you've masked. This is trivial to fix, if you want to avoid
reposting, it's fine with me.

>  static int ironlake_irq_postinstall(struct drm_device *dev)
>  {
>  	unsigned long irqflags;
> @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
>  			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
>  			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
> -	u32 gt_irqs;
>  
>  	dev_priv->irq_mask = ~display_mask;
>  
> @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  			  DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
>  	POSTING_READ(DEIER);
>  
> -	dev_priv->gt_irq_mask = ~0;
> -
> -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -
> -	gt_irqs = GT_RENDER_USER_INTERRUPT;
> -
> -	if (IS_GEN6(dev))
> -		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> -	else
> -		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> -			   ILK_BSD_USER_INTERRUPT;
> -
> -	I915_WRITE(GTIER, gt_irqs);
> -	POSTING_READ(GTIER);
> +	gen5_gt_irq_postinstall(dev);
>  
>  	ibx_irq_postinstall(dev);
>  
> @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  		DE_PLANEA_FLIP_DONE_IVB |
>  		DE_AUX_CHANNEL_A_IVB |
>  		DE_ERR_INT_IVB;
> -	u32 pm_irqs = GEN6_PM_RPS_EVENTS;
> -	u32 gt_irqs;
>  
>  	dev_priv->irq_mask = ~display_mask;
>  
> @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  		   DE_PIPEA_VBLANK_IVB);
>  	POSTING_READ(DEIER);
>  
> -	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> -
> -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -
> -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> -		  GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> -	I915_WRITE(GTIER, gt_irqs);
> -	POSTING_READ(GTIER);
> -
> -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> -	if (HAS_VEBOX(dev))
> -		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> -
> -	/* Our enable/disable rps functions may touch these registers so
> -	 * make sure to set a known state for only the non-RPS bits.
> -	 * The RMW is extra paranoia since this should be called after being set
> -	 * to a known state in preinstall.
> -	 * */
> -	I915_WRITE(GEN6_PMIMR,
> -		   (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
> -	I915_WRITE(GEN6_PMIER,
> -		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
> -	POSTING_READ(GEN6_PMIER);
> +	gen5_gt_irq_postinstall(dev);
>  
>  	ibx_irq_postinstall(dev);
>  
> @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  static int valleyview_irq_postinstall(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	u32 gt_irqs;
>  	u32 enable_mask;
>  	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
>  	unsigned long irqflags;
> @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
>  	I915_WRITE(VLV_IIR, 0xffffffff);
>  	I915_WRITE(VLV_IIR, 0xffffffff);
>  
> -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -
> -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> -		GT_BLT_USER_INTERRUPT;
> -	I915_WRITE(GTIER, gt_irqs);
> -	POSTING_READ(GTIER);
> +	gen5_gt_irq_postinstall(dev);
>  
>  	/* ack & enable invalid PTE error interrupts */
>  #if 0 /* FIXME: add support to irq handler for checking these bits */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fb4afaa..e609232 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
>  
>  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>  
> -	/* requires MSI enabled */
> -	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	/* FIXME: Our interrupt enabling sequence is bonghits.
>  	 * dev_priv->rps.pm_iir really should be 0 here. */
> @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  
>  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
> -	/* requires MSI enabled */
> -	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	WARN_ON(dev_priv->rps.pm_iir != 0);
>  	I915_WRITE(GEN6_PMIMR, 0);

With my comments in 1 & 2 address, they're both:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Daniel Vetter]

On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote:
> On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote:
> > Again extract a common helper. For the postinstall hook things are a
> > bit more complicated since we have more cases on ilk-hsw/vlv here.
> > 
> > But since vlv was clearly broken by failing to initialize
> > dev_priv->gt_irq_mask correctly the shared code is clearly justified.
> > 
> > Also kill the PMIER setting in the async rps enable work. I should
> > have been save, but also clearly looked rather fragile. PMIER setup is
> Can you fix up this sentence. It's a bit weird, and I'm not positive
> what you're trying to say.

Stab at your VECS enabling patches which touch PMIER in the rps enabling
work. After this patch PMIER is only set up (once) in the postinstall hook
with all interrupts we ever want already enabled. We only update the PMIMR
register when we enable rps, like with all other interrupts enabled after
irq install time.

> > not all down in the irq pre/postinstall hooks.

s/not/now/ here probably clarifies it?

> > 
> > With this we now have the usual interrupt register sequence for GT/PM
> > irq registers:
> > 
> > - IER is setup once with all the interrupts we ever need in the
> >   postinstall hook and never touched again. Exceptions are SDEIER,
> >   which is touched in the preinstall hook (when the irq handler isn't
> >   enabled) and then only from the irq handler. And DEIER/VLV_IER with
> >   is used in the irq handler but also written to once in the
> >   postinstall hook. But since that write is essentially what enables
> >   the interrupt and we should always have MSI interrupts we should be
> >   save. In case we ever have non-MSI interrupts we'd be screwed.
> > 
> > - IIR is cleared in the postinstall hook before we enable/unmask the
> >   respective interrupt sources. Hence we can't steal an interrupt
> >   event an accidentally trigger the spurious interrupt logic in the
> >   core kernel. Note that after some discussion with Ben Widawsky we
> >   think that we actually should clear the IIR registers in the
> >   preinstall hook. But doing that is a much larger patch series.
> > 
> > - IMR regs are (usually) all masked off. Those are the only regs
> >   changed at runtime, which is all protected by dev_priv->irq_lock.
> > 
> 
> This comment block is really nice. Maybe supplement it with explanation
> about how the ring interrupts work, and shove it in i915_irq.c?

I'm not a fan of big overview comments. The get outdated really fast and
no one ever bothers to update them. In the commit message they're
annotated to a clear point in our history with the implication that they
have been reviewed to be correct at that point.

> 
> > This unification also kills the cargo-culted read-modify-write PM
> > register setup for VECS. Interrupt setup is done without userspace
> > being able to interfere, so we better know what values we want to put
> > into those registers. RMW cycles otoh are really good at papering over
> > races, until stuff magically blows up and no one has a clue why.
> > 
> > v2: Touch the gen6+ PM interrupt registers only on gen6+.
> > 
> > v3: Improve the commit message to more clearly spell out why we want
> > to unify the code and what exactly changes.
> > 
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
> >  drivers/gpu/drm/i915/intel_pm.c |  4 --
> >  2 files changed, 42 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index d5c3bef..ba61bc7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> >  	I915_WRITE(SDEIMR, ~mask);
> >  }
> >  
> > +static void gen5_gt_irq_postinstall(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u32 pm_irqs, gt_irqs;
> > +
> > +	pm_irqs = gt_irqs = 0;
> > +
> > +	dev_priv->gt_irq_mask = ~0;
> > +	if (HAS_L3_GPU_CACHE(dev)) {
> > +		dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > +		gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > +	}
> 
> Maybe while you're doing this, explain why the L3 parity interrupt is
> special, in a comment. It's the only one to touch dev_priv->gt_irq_mask

I'll add 
		/* L3 parity interrupt is always unmasked. */

before the gt_irq_mask assignemnt.

> 
> > +
> > +	gt_irqs |= GT_RENDER_USER_INTERRUPT;
> > +	if (IS_GEN5(dev)) {
> > +		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> > +			   ILK_BSD_USER_INTERRUPT;
> > +	} else {
> > +		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> > +	}
> > +
> > +	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > +	I915_WRITE(GTIER, gt_irqs);
> > +	POSTING_READ(GTIER);
> > +
> > +	if (INTEL_INFO(dev)->gen >= 6) {
> > +		pm_irqs |= GEN6_PM_RPS_EVENTS;
> > +
> > +		if (HAS_VEBOX(dev))
> > +			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> > +
> > +		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > +		I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > +		I915_WRITE(GEN6_PMIER, pm_irqs);
> > +		POSTING_READ(GEN6_PMIER);
> > +	}
> > +}
> > +
> 
> The ordering still looks funky here to me and your comment in the commit
> message hasn't convinced me otherwise. The order should be:
> MASK
> CLEAR
> ENABLE
> 
> In your order there is a theoretical race after you've cleared, but
> before you've masked. This is trivial to fix, if you want to avoid
> reposting, it's fine with me.

Imo fixing that up is a separate patch series, I'm sticking to being
consistent whats there already. In any case I vote for a bit of
refactoring so that all the I.R register sequences use the same code, i.e.
something like the DISABLE_AND_CLEAR_INTERRUPT macro I've proposed in
another thread.

> 
> >  static int ironlake_irq_postinstall(struct drm_device *dev)
> >  {
> >  	unsigned long irqflags;
> > @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >  			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
> >  			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
> >  			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
> > -	u32 gt_irqs;
> >  
> >  	dev_priv->irq_mask = ~display_mask;
> >  
> > @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >  			  DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
> >  	POSTING_READ(DEIER);
> >  
> > -	dev_priv->gt_irq_mask = ~0;
> > -
> > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -
> > -	gt_irqs = GT_RENDER_USER_INTERRUPT;
> > -
> > -	if (IS_GEN6(dev))
> > -		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> > -	else
> > -		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> > -			   ILK_BSD_USER_INTERRUPT;
> > -
> > -	I915_WRITE(GTIER, gt_irqs);
> > -	POSTING_READ(GTIER);
> > +	gen5_gt_irq_postinstall(dev);
> >  
> >  	ibx_irq_postinstall(dev);
> >  
> > @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> >  		DE_PLANEA_FLIP_DONE_IVB |
> >  		DE_AUX_CHANNEL_A_IVB |
> >  		DE_ERR_INT_IVB;
> > -	u32 pm_irqs = GEN6_PM_RPS_EVENTS;
> > -	u32 gt_irqs;
> >  
> >  	dev_priv->irq_mask = ~display_mask;
> >  
> > @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> >  		   DE_PIPEA_VBLANK_IVB);
> >  	POSTING_READ(DEIER);
> >  
> > -	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > -
> > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -
> > -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> > -		  GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > -	I915_WRITE(GTIER, gt_irqs);
> > -	POSTING_READ(GTIER);
> > -
> > -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > -	if (HAS_VEBOX(dev))
> > -		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> > -
> > -	/* Our enable/disable rps functions may touch these registers so
> > -	 * make sure to set a known state for only the non-RPS bits.
> > -	 * The RMW is extra paranoia since this should be called after being set
> > -	 * to a known state in preinstall.
> > -	 * */
> > -	I915_WRITE(GEN6_PMIMR,
> > -		   (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
> > -	I915_WRITE(GEN6_PMIER,
> > -		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
> > -	POSTING_READ(GEN6_PMIER);
> > +	gen5_gt_irq_postinstall(dev);
> >  
> >  	ibx_irq_postinstall(dev);
> >  
> > @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> >  static int valleyview_irq_postinstall(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > -	u32 gt_irqs;
> >  	u32 enable_mask;
> >  	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
> >  	unsigned long irqflags;
> > @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
> >  	I915_WRITE(VLV_IIR, 0xffffffff);
> >  	I915_WRITE(VLV_IIR, 0xffffffff);
> >  
> > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -
> > -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> > -		GT_BLT_USER_INTERRUPT;
> > -	I915_WRITE(GTIER, gt_irqs);
> > -	POSTING_READ(GTIER);
> > +	gen5_gt_irq_postinstall(dev);
> >  
> >  	/* ack & enable invalid PTE error interrupts */
> >  #if 0 /* FIXME: add support to irq handler for checking these bits */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index fb4afaa..e609232 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  
> >  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> >  
> > -	/* requires MSI enabled */
> > -	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
> >  	spin_lock_irq(&dev_priv->irq_lock);
> >  	/* FIXME: Our interrupt enabling sequence is bonghits.
> >  	 * dev_priv->rps.pm_iir really should be 0 here. */
> > @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >  
> >  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> >  
> > -	/* requires MSI enabled */
> > -	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
> >  	spin_lock_irq(&dev_priv->irq_lock);
> >  	WARN_ON(dev_priv->rps.pm_iir != 0);
> >  	I915_WRITE(GEN6_PMIMR, 0);
> 
> With my comments in 1 & 2 address, they're both:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

If you don't object to my comments above I'll bikeshed the patch while
applying and add the missing comment and fix up the commit message.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Ben Widawsky]

On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote:
> > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote:
> > > Again extract a common helper. For the postinstall hook things are a
> > > bit more complicated since we have more cases on ilk-hsw/vlv here.
> > > 
> > > But since vlv was clearly broken by failing to initialize
> > > dev_priv->gt_irq_mask correctly the shared code is clearly justified.
> > > 
> > > Also kill the PMIER setting in the async rps enable work. I should
> > > have been save, but also clearly looked rather fragile. PMIER setup is
> > Can you fix up this sentence. It's a bit weird, and I'm not positive
> > what you're trying to say.
> 
> Stab at your VECS enabling patches which touch PMIER in the rps enabling
> work. After this patch PMIER is only set up (once) in the postinstall hook
> with all interrupts we ever want already enabled. We only update the PMIMR
> register when we enable rps, like with all other interrupts enabled after
> irq install time.
> 
> > > not all down in the irq pre/postinstall hooks.
> 
> s/not/now/ here probably clarifies it?
> 
> > > 
> > > With this we now have the usual interrupt register sequence for GT/PM
> > > irq registers:
> > > 
> > > - IER is setup once with all the interrupts we ever need in the
> > >   postinstall hook and never touched again. Exceptions are SDEIER,
> > >   which is touched in the preinstall hook (when the irq handler isn't
> > >   enabled) and then only from the irq handler. And DEIER/VLV_IER with
> > >   is used in the irq handler but also written to once in the
> > >   postinstall hook. But since that write is essentially what enables
> > >   the interrupt and we should always have MSI interrupts we should be
> > >   save. In case we ever have non-MSI interrupts we'd be screwed.
> > > 
> > > - IIR is cleared in the postinstall hook before we enable/unmask the
> > >   respective interrupt sources. Hence we can't steal an interrupt
> > >   event an accidentally trigger the spurious interrupt logic in the
> > >   core kernel. Note that after some discussion with Ben Widawsky we
> > >   think that we actually should clear the IIR registers in the
> > >   preinstall hook. But doing that is a much larger patch series.
> > > 
> > > - IMR regs are (usually) all masked off. Those are the only regs
> > >   changed at runtime, which is all protected by dev_priv->irq_lock.
> > > 
> > 
> > This comment block is really nice. Maybe supplement it with explanation
> > about how the ring interrupts work, and shove it in i915_irq.c?
> 
> I'm not a fan of big overview comments. The get outdated really fast and
> no one ever bothers to update them. In the commit message they're
> annotated to a clear point in our history with the implication that they
> have been reviewed to be correct at that point.
> 
> > 
> > > This unification also kills the cargo-culted read-modify-write PM
> > > register setup for VECS. Interrupt setup is done without userspace
> > > being able to interfere, so we better know what values we want to put
> > > into those registers. RMW cycles otoh are really good at papering over
> > > races, until stuff magically blows up and no one has a clue why.
> > > 
> > > v2: Touch the gen6+ PM interrupt registers only on gen6+.
> > > 
> > > v3: Improve the commit message to more clearly spell out why we want
> > > to unify the code and what exactly changes.
> > > 
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > Cc: Paulo Zanoni <przanoni@gmail.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
> > >  drivers/gpu/drm/i915/intel_pm.c |  4 --
> > >  2 files changed, 42 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index d5c3bef..ba61bc7 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> > >  	I915_WRITE(SDEIMR, ~mask);
> > >  }
> > >  
> > > +static void gen5_gt_irq_postinstall(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	u32 pm_irqs, gt_irqs;
> > > +
> > > +	pm_irqs = gt_irqs = 0;
> > > +
> > > +	dev_priv->gt_irq_mask = ~0;
> > > +	if (HAS_L3_GPU_CACHE(dev)) {
> > > +		dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > +		gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > +	}
> > 
> > Maybe while you're doing this, explain why the L3 parity interrupt is
> > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask
> 
> I'll add 
> 		/* L3 parity interrupt is always unmasked. */
> 
> before the gt_irq_mask assignemnt.
> 
> > 
> > > +
> > > +	gt_irqs |= GT_RENDER_USER_INTERRUPT;
> > > +	if (IS_GEN5(dev)) {
> > > +		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> > > +			   ILK_BSD_USER_INTERRUPT;
> > > +	} else {
> > > +		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> > > +	}
> > > +
> > > +	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > +	I915_WRITE(GTIER, gt_irqs);
> > > +	POSTING_READ(GTIER);
> > > +
> > > +	if (INTEL_INFO(dev)->gen >= 6) {
> > > +		pm_irqs |= GEN6_PM_RPS_EVENTS;
> > > +
> > > +		if (HAS_VEBOX(dev))
> > > +			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> > > +
> > > +		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > > +		I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > > +		I915_WRITE(GEN6_PMIER, pm_irqs);
> > > +		POSTING_READ(GEN6_PMIER);
> > > +	}
> > > +}
> > > +
> > 
> > The ordering still looks funky here to me and your comment in the commit
> > message hasn't convinced me otherwise. The order should be:
> > MASK
> > CLEAR
> > ENABLE
> > 
> > In your order there is a theoretical race after you've cleared, but
> > before you've masked. This is trivial to fix, if you want to avoid
> > reposting, it's fine with me.
> 
> Imo fixing that up is a separate patch series, I'm sticking to being
> consistent whats there already. In any case I vote for a bit of
> refactoring so that all the I.R register sequences use the same code, i.e.
> something like the DISABLE_AND_CLEAR_INTERRUPT macro I've proposed in
> another thread.
> 
> > 
> > >  static int ironlake_irq_postinstall(struct drm_device *dev)
> > >  {
> > >  	unsigned long irqflags;
> > > @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > >  			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
> > >  			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
> > >  			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
> > > -	u32 gt_irqs;
> > >  
> > >  	dev_priv->irq_mask = ~display_mask;
> > >  
> > > @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > >  			  DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
> > >  	POSTING_READ(DEIER);
> > >  
> > > -	dev_priv->gt_irq_mask = ~0;
> > > -
> > > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > -
> > > -	gt_irqs = GT_RENDER_USER_INTERRUPT;
> > > -
> > > -	if (IS_GEN6(dev))
> > > -		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> > > -	else
> > > -		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> > > -			   ILK_BSD_USER_INTERRUPT;
> > > -
> > > -	I915_WRITE(GTIER, gt_irqs);
> > > -	POSTING_READ(GTIER);
> > > +	gen5_gt_irq_postinstall(dev);
> > >  
> > >  	ibx_irq_postinstall(dev);
> > >  
> > > @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> > >  		DE_PLANEA_FLIP_DONE_IVB |
> > >  		DE_AUX_CHANNEL_A_IVB |
> > >  		DE_ERR_INT_IVB;
> > > -	u32 pm_irqs = GEN6_PM_RPS_EVENTS;
> > > -	u32 gt_irqs;
> > >  
> > >  	dev_priv->irq_mask = ~display_mask;
> > >  
> > > @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> > >  		   DE_PIPEA_VBLANK_IVB);
> > >  	POSTING_READ(DEIER);
> > >  
> > > -	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > -
> > > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > -
> > > -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> > > -		  GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > -	I915_WRITE(GTIER, gt_irqs);
> > > -	POSTING_READ(GTIER);
> > > -
> > > -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > > -	if (HAS_VEBOX(dev))
> > > -		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> > > -
> > > -	/* Our enable/disable rps functions may touch these registers so
> > > -	 * make sure to set a known state for only the non-RPS bits.
> > > -	 * The RMW is extra paranoia since this should be called after being set
> > > -	 * to a known state in preinstall.
> > > -	 * */
> > > -	I915_WRITE(GEN6_PMIMR,
> > > -		   (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
> > > -	I915_WRITE(GEN6_PMIER,
> > > -		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
> > > -	POSTING_READ(GEN6_PMIER);
> > > +	gen5_gt_irq_postinstall(dev);
> > >  
> > >  	ibx_irq_postinstall(dev);
> > >  
> > > @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> > >  static int valleyview_irq_postinstall(struct drm_device *dev)
> > >  {
> > >  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > > -	u32 gt_irqs;
> > >  	u32 enable_mask;
> > >  	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
> > >  	unsigned long irqflags;
> > > @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
> > >  	I915_WRITE(VLV_IIR, 0xffffffff);
> > >  	I915_WRITE(VLV_IIR, 0xffffffff);
> > >  
> > > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > -
> > > -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> > > -		GT_BLT_USER_INTERRUPT;
> > > -	I915_WRITE(GTIER, gt_irqs);
> > > -	POSTING_READ(GTIER);
> > > +	gen5_gt_irq_postinstall(dev);
> > >  
> > >  	/* ack & enable invalid PTE error interrupts */
> > >  #if 0 /* FIXME: add support to irq handler for checking these bits */
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index fb4afaa..e609232 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
> > >  
> > >  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> > >  
> > > -	/* requires MSI enabled */
> > > -	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
> > >  	spin_lock_irq(&dev_priv->irq_lock);
> > >  	/* FIXME: Our interrupt enabling sequence is bonghits.
> > >  	 * dev_priv->rps.pm_iir really should be 0 here. */
> > > @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > >  
> > >  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> > >  
> > > -	/* requires MSI enabled */
> > > -	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
> > >  	spin_lock_irq(&dev_priv->irq_lock);
> > >  	WARN_ON(dev_priv->rps.pm_iir != 0);
> > >  	I915_WRITE(GEN6_PMIMR, 0);
> > 
> > With my comments in 1 & 2 address, they're both:
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> If you don't object to my comments above I'll bikeshed the patch while
> applying and add the missing comment and fix up the commit message.
> -Daniel

ack
-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Ben Widawsky]

On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote:
> > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote:
[snip]

> > 
> > Maybe while you're doing this, explain why the L3 parity interrupt is
> > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask
> 
> I'll add 
> 		/* L3 parity interrupt is always unmasked. */
> 
> before the gt_irq_mask assignemnt.

Actually, I was thinking more along the lines of "L3 parity interrupt is
always unmasked. L3 parity interrupts are asynchronous with regard to
batch submission, however they are delivered through ring interrupt
registers."

[snip]
-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Daniel Vetter]

On Sun, Jul 14, 2013 at 05:13:34PM -0700, Ben Widawsky wrote:
> On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote:
> > On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote:
> > > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote:
> [snip]
> 
> > > 
> > > Maybe while you're doing this, explain why the L3 parity interrupt is
> > > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask
> > 
> > I'll add 
> > 		/* L3 parity interrupt is always unmasked. */
> > 
> > before the gt_irq_mask assignemnt.
> 
> Actually, I was thinking more along the lines of "L3 parity interrupt is
> always unmasked. L3 parity interrupts are asynchronous with regard to
> batch submission, however they are delivered through ring interrupt
> registers."

We have more suche cases now with VECS going through PM interrupts. So I
don't think the fact that hw engineers sometimes steal bits from odd
places requires special mention. So I've opted for the simple version.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/3] drm/i915: extract rps interrupt enable/disable helpers

[Daniel Vetter]

The VECS enabling required some changes to how rps interrupts are
enabled/disabled since VECS interrupts are handling with the PM
interrupt registers.

But now that the pre/postinstall sequences is identical for all
platforms with rps support (snb, ivb, hsw, vlv) we can also use the
exact same sequence to actually enable the rps interrupts. Strictly
speaking using spinlocks is overkill on snb/ivb & vlv since they have
no VECS ring, but imo that's more than made up by the common code.

Hence this just unifies the vlv code with the snb-hsw code which
matched exactly before the VECS enabling. See

commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue May 28 19:22:26 2013 -0700

    drm/i915: Add PM regs to pre/post install

and

commit 4848405cced3b46f4ec7d404b8ed5873171ae10a
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue May 28 19:22:27 2013 -0700

    drm/i915: make PM interrupt writes non-destructive

for why the gen6 code (shared between snb, ivb and hsw) needed to be
changed originally.

v3: Improve the commit message to more clearly spell out why we want
to unify the code and what exactly changes.

Cc: Paulo Zanoni <przanoni@gmail.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e609232..190ab96 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
 }
 
-
-static void gen6_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(GEN6_RC_CONTROL, 0);
-	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
 	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
 	/* Complete PM interrupt masking here doesn't race with the rps work
@@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
 }
 
-static void valleyview_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	I915_WRITE(GEN6_RC_CONTROL, 0);
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-	I915_WRITE(GEN6_PMIER, 0);
-	/* Complete PM interrupt masking here doesn't race with the rps work
-	 * item again unmasking PM interrupts because that is using a different
-	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
-	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
+	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir = 0;
-	spin_unlock_irq(&dev_priv->irq_lock);
+	gen6_disable_rps_interrupts(dev);
+}
+
+static void valleyview_disable_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_RC_CONTROL, 0);
 
-	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
+	gen6_disable_rps_interrupts(dev);
 
 	if (dev_priv->vlv_pctx) {
 		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
@@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
 	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
+static void gen6_enable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	/* FIXME: Our interrupt enabling sequence is bonghits.
+	 * dev_priv->rps.pm_iir really should be 0 here. */
+	dev_priv->rps.pm_iir = 0;
+	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
+	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
+	spin_unlock_irq(&dev_priv->irq_lock);
+	/* unmask all PM interrupts */
+	I915_WRITE(GEN6_PMINTRMSK, 0);
+}
+
 static void gen6_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	/* FIXME: Our interrupt enabling sequence is bonghits.
-	 * dev_priv->rps.pm_iir really should be 0 here. */
-	dev_priv->rps.pm_iir = 0;
-	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
-	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
-	spin_unlock_irq(&dev_priv->irq_lock);
-	/* unmask all PM interrupts */
-	I915_WRITE(GEN6_PMINTRMSK, 0);
+	gen6_enable_rps_interrupts(dev);
 
 	rc6vids = 0;
 	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
@@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	WARN_ON(dev_priv->rps.pm_iir != 0);
-	I915_WRITE(GEN6_PMIMR, 0);
-	spin_unlock_irq(&dev_priv->irq_lock);
-	/* enable all PM interrupts */
-	I915_WRITE(GEN6_PMINTRMSK, 0);
+	gen6_enable_rps_interrupts(dev);
 
 	gen6_gt_force_wake_put(dev_priv);
 }
-- 
1.8.1.4

Re: [PATCH 3/3] drm/i915: extract rps interrupt enable/disable helpers

[Ben Widawsky]

On Fri, Jul 12, 2013 at 10:43:27PM +0200, Daniel Vetter wrote:
> The VECS enabling required some changes to how rps interrupts are
> enabled/disabled since VECS interrupts are handling with the PM
> interrupt registers.
> 
> But now that the pre/postinstall sequences is identical for all
> platforms with rps support (snb, ivb, hsw, vlv) we can also use the
> exact same sequence to actually enable the rps interrupts. Strictly
> speaking using spinlocks is overkill on snb/ivb & vlv since they have
> no VECS ring, but imo that's more than made up by the common code.
> 
> Hence this just unifies the vlv code with the snb-hsw code which
> matched exactly before the VECS enabling. See
> 
> commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Tue May 28 19:22:26 2013 -0700
> 
>     drm/i915: Add PM regs to pre/post install
> 
> and
> 
> commit 4848405cced3b46f4ec7d404b8ed5873171ae10a
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Tue May 28 19:22:27 2013 -0700
> 
>     drm/i915: make PM interrupt writes non-destructive
> 
> for why the gen6 code (shared between snb, ivb and hsw) needed to be
> changed originally.
> 
> v3: Improve the commit message to more clearly spell out why we want
> to unify the code and what exactly changes.
> 
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e609232..190ab96 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
>  }
>  
> -
> -static void gen6_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	I915_WRITE(GEN6_RC_CONTROL, 0);
> -	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>  	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>  	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);

I think actually, we need a POSTING_READ here before clearing
dev_priv->rps.pm_iir. Or maybe just one after disabling RPS is enough.
Not sure. Maybe just a hypothetical race.

>  	/* Complete PM interrupt masking here doesn't race with the rps work
> @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
>  	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
>  }
>  
> -static void valleyview_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
> -	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> -	I915_WRITE(GEN6_PMIER, 0);
> -	/* Complete PM interrupt masking here doesn't race with the rps work
> -	 * item again unmasking PM interrupts because that is using a different
> -	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> -	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> +	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>  
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	dev_priv->rps.pm_iir = 0;
> -	spin_unlock_irq(&dev_priv->irq_lock);
> +	gen6_disable_rps_interrupts(dev);
> +}
> +
> +static void valleyview_disable_rps(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(GEN6_RC_CONTROL, 0);
>  
> -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> +	gen6_disable_rps_interrupts(dev);
>  
>  	if (dev_priv->vlv_pctx) {
>  		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
>  	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
>  }
>  
> +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	/* FIXME: Our interrupt enabling sequence is bonghits.
> +	 * dev_priv->rps.pm_iir really should be 0 here. */
> +	dev_priv->rps.pm_iir = 0;
> +	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);

I thought for enable, we want to modify PMIER? If it's not required, I
want a comment when not. Also, with your changes, it should be safe to
remove the FIXME if I am not mistaken.

> +	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +	/* unmask all PM interrupts */
> +	I915_WRITE(GEN6_PMINTRMSK, 0);
> +}
> +

Also, maybe move this (with disable interrupts) to i915_irq.c?

>  static void gen6_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>  
>  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>  
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	/* FIXME: Our interrupt enabling sequence is bonghits.
> -	 * dev_priv->rps.pm_iir really should be 0 here. */
> -	dev_priv->rps.pm_iir = 0;
> -	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> -	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> -	spin_unlock_irq(&dev_priv->irq_lock);
> -	/* unmask all PM interrupts */
> -	I915_WRITE(GEN6_PMINTRMSK, 0);
> +	gen6_enable_rps_interrupts(dev);
>  
>  	rc6vids = 0;
>  	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  
>  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	WARN_ON(dev_priv->rps.pm_iir != 0);
> -	I915_WRITE(GEN6_PMIMR, 0);
> -	spin_unlock_irq(&dev_priv->irq_lock);
> -	/* enable all PM interrupts */
> -	I915_WRITE(GEN6_PMINTRMSK, 0);
> +	gen6_enable_rps_interrupts(dev);
>  
>  	gen6_gt_force_wake_put(dev_priv);
>  }

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: extract rps interrupt enable/disable helpers

[Daniel Vetter]

On Sun, Jul 14, 2013 at 02:06:28PM -0700, Ben Widawsky wrote:
> On Fri, Jul 12, 2013 at 10:43:27PM +0200, Daniel Vetter wrote:
> > The VECS enabling required some changes to how rps interrupts are
> > enabled/disabled since VECS interrupts are handling with the PM
> > interrupt registers.
> > 
> > But now that the pre/postinstall sequences is identical for all
> > platforms with rps support (snb, ivb, hsw, vlv) we can also use the
> > exact same sequence to actually enable the rps interrupts. Strictly
> > speaking using spinlocks is overkill on snb/ivb & vlv since they have
> > no VECS ring, but imo that's more than made up by the common code.
> > 
> > Hence this just unifies the vlv code with the snb-hsw code which
> > matched exactly before the VECS enabling. See
> > 
> > commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
> > Author: Ben Widawsky <ben@bwidawsk.net>
> > Date:   Tue May 28 19:22:26 2013 -0700
> > 
> >     drm/i915: Add PM regs to pre/post install
> > 
> > and
> > 
> > commit 4848405cced3b46f4ec7d404b8ed5873171ae10a
> > Author: Ben Widawsky <ben@bwidawsk.net>
> > Date:   Tue May 28 19:22:27 2013 -0700
> > 
> >     drm/i915: make PM interrupt writes non-destructive
> > 
> > for why the gen6 code (shared between snb, ivb and hsw) needed to be
> > changed originally.
> > 
> > v3: Improve the commit message to more clearly spell out why we want
> > to unify the code and what exactly changes.
> > 
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
> >  1 file changed, 29 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index e609232..190ab96 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> >  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
> >  }
> >  
> > -
> > -static void gen6_disable_rps(struct drm_device *dev)
> > +static void gen6_disable_rps_interrupts(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > -	I915_WRITE(GEN6_RC_CONTROL, 0);
> > -	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >  	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> >  	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
> 
> I think actually, we need a POSTING_READ here before clearing
> dev_priv->rps.pm_iir. Or maybe just one after disabling RPS is enough.
> Not sure. Maybe just a hypothetical race.

I certainly won't hurt, but again I think a separate patch series. Atm
most of our uninstall code doesn't use POSTING_READs at all.

> 
> >  	/* Complete PM interrupt masking here doesn't race with the rps work
> > @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
> >  	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> >  }
> >  
> > -static void valleyview_disable_rps(struct drm_device *dev)
> > +static void gen6_disable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	I915_WRITE(GEN6_RC_CONTROL, 0);
> > -	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > -	I915_WRITE(GEN6_PMIER, 0);
> > -	/* Complete PM interrupt masking here doesn't race with the rps work
> > -	 * item again unmasking PM interrupts because that is using a different
> > -	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> > -	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> > +	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >  
> > -	spin_lock_irq(&dev_priv->irq_lock);
> > -	dev_priv->rps.pm_iir = 0;
> > -	spin_unlock_irq(&dev_priv->irq_lock);
> > +	gen6_disable_rps_interrupts(dev);
> > +}
> > +
> > +static void valleyview_disable_rps(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	I915_WRITE(GEN6_RC_CONTROL, 0);
> >  
> > -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > +	gen6_disable_rps_interrupts(dev);
> >  
> >  	if (dev_priv->vlv_pctx) {
> >  		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> > @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
> >  	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> >  }
> >  
> > +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	spin_lock_irq(&dev_priv->irq_lock);
> > +	/* FIXME: Our interrupt enabling sequence is bonghits.
> > +	 * dev_priv->rps.pm_iir really should be 0 here. */
> > +	dev_priv->rps.pm_iir = 0;
> > +	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> 
> I thought for enable, we want to modify PMIER? If it's not required, I
> want a comment when not. Also, with your changes, it should be safe to
> remove the FIXME if I am not mistaken.

Next patch (i.e. the final one, which I haven't resent) does that. And
you've slapped your r-b onto it already.

> 
> > +	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > +	spin_unlock_irq(&dev_priv->irq_lock);
> > +	/* unmask all PM interrupts */
> > +	I915_WRITE(GEN6_PMINTRMSK, 0);
> > +}
> > +
> 
> Also, maybe move this (with disable interrupts) to i915_irq.c?

More forward declarations, which are my rough (and sometimes misguided
metric) to judge whether a function is at the right place. Hence I've
decided against moving it. Has the downside that not all code touching
PMIMR is at the same place, but alas. We can't share a commen
enable/disable_pm_interrupts like we have for display interrupt registers
and similar stuff since we want to do a bit more than just set up
interrupt registers under the spinlock protection.

Also, the other guy toching PMIMR at runtime lives in intel_ringbuffer.c,
so moving this to i915_irq.c won't bring them any closer.
-Daniel

> 
> >  static void gen6_enable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  
> >  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> >  
> > -	spin_lock_irq(&dev_priv->irq_lock);
> > -	/* FIXME: Our interrupt enabling sequence is bonghits.
> > -	 * dev_priv->rps.pm_iir really should be 0 here. */
> > -	dev_priv->rps.pm_iir = 0;
> > -	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > -	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > -	spin_unlock_irq(&dev_priv->irq_lock);
> > -	/* unmask all PM interrupts */
> > -	I915_WRITE(GEN6_PMINTRMSK, 0);
> > +	gen6_enable_rps_interrupts(dev);
> >  
> >  	rc6vids = 0;
> >  	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >  
> >  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> >  
> > -	spin_lock_irq(&dev_priv->irq_lock);
> > -	WARN_ON(dev_priv->rps.pm_iir != 0);
> > -	I915_WRITE(GEN6_PMIMR, 0);
> > -	spin_unlock_irq(&dev_priv->irq_lock);
> > -	/* enable all PM interrupts */
> > -	I915_WRITE(GEN6_PMINTRMSK, 0);
> > +	gen6_enable_rps_interrupts(dev);
> >  
> >  	gen6_gt_force_wake_put(dev_priv);
> >  }
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center

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

Re: [PATCH 3/3] drm/i915: extract rps interrupt enable/disable helpers

[Ben Widawsky]

On Sun, Jul 14, 2013 at 11:35:46PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 02:06:28PM -0700, Ben Widawsky wrote:
> > On Fri, Jul 12, 2013 at 10:43:27PM +0200, Daniel Vetter wrote:
> > > The VECS enabling required some changes to how rps interrupts are
> > > enabled/disabled since VECS interrupts are handling with the PM
> > > interrupt registers.
> > > 
> > > But now that the pre/postinstall sequences is identical for all
> > > platforms with rps support (snb, ivb, hsw, vlv) we can also use the
> > > exact same sequence to actually enable the rps interrupts. Strictly
> > > speaking using spinlocks is overkill on snb/ivb & vlv since they have
> > > no VECS ring, but imo that's more than made up by the common code.
> > > 
> > > Hence this just unifies the vlv code with the snb-hsw code which
> > > matched exactly before the VECS enabling. See
> > > 
> > > commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Tue May 28 19:22:26 2013 -0700
> > > 
> > >     drm/i915: Add PM regs to pre/post install
> > > 
> > > and
> > > 
> > > commit 4848405cced3b46f4ec7d404b8ed5873171ae10a
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Tue May 28 19:22:27 2013 -0700
> > > 
> > >     drm/i915: make PM interrupt writes non-destructive
> > > 
> > > for why the gen6 code (shared between snb, ivb and hsw) needed to be
> > > changed originally.
> > > 
> > > v3: Improve the commit message to more clearly spell out why we want
> > > to unify the code and what exactly changes.
> > > 
> > > Cc: Paulo Zanoni <przanoni@gmail.com>
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
> > >  1 file changed, 29 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index e609232..190ab96 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> > >  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
> > >  }
> > >  
> > > -
> > > -static void gen6_disable_rps(struct drm_device *dev)
> > > +static void gen6_disable_rps_interrupts(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > -	I915_WRITE(GEN6_RC_CONTROL, 0);
> > > -	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> > >  	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > >  	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
> > 
> > I think actually, we need a POSTING_READ here before clearing
> > dev_priv->rps.pm_iir. Or maybe just one after disabling RPS is enough.
> > Not sure. Maybe just a hypothetical race.
> 
> I certainly won't hurt, but again I think a separate patch series. Atm
> most of our uninstall code doesn't use POSTING_READs at all.
> 
> > 
> > >  	/* Complete PM interrupt masking here doesn't race with the rps work
> > > @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
> > >  	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > >  }
> > >  
> > > -static void valleyview_disable_rps(struct drm_device *dev)
> > > +static void gen6_disable_rps(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > >  	I915_WRITE(GEN6_RC_CONTROL, 0);
> > > -	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > > -	I915_WRITE(GEN6_PMIER, 0);
> > > -	/* Complete PM interrupt masking here doesn't race with the rps work
> > > -	 * item again unmasking PM interrupts because that is using a different
> > > -	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> > > -	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> > > +	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> > >  
> > > -	spin_lock_irq(&dev_priv->irq_lock);
> > > -	dev_priv->rps.pm_iir = 0;
> > > -	spin_unlock_irq(&dev_priv->irq_lock);
> > > +	gen6_disable_rps_interrupts(dev);
> > > +}
> > > +
> > > +static void valleyview_disable_rps(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	I915_WRITE(GEN6_RC_CONTROL, 0);
> > >  
> > > -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > > +	gen6_disable_rps_interrupts(dev);
> > >  
> > >  	if (dev_priv->vlv_pctx) {
> > >  		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> > > @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
> > >  	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> > >  }
> > >  
> > > +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	spin_lock_irq(&dev_priv->irq_lock);
> > > +	/* FIXME: Our interrupt enabling sequence is bonghits.
> > > +	 * dev_priv->rps.pm_iir really should be 0 here. */
> > > +	dev_priv->rps.pm_iir = 0;
> > > +	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > 
> > I thought for enable, we want to modify PMIER? If it's not required, I
> > want a comment when not. Also, with your changes, it should be safe to
> > remove the FIXME if I am not mistaken.
> 
> Next patch (i.e. the final one, which I haven't resent) does that. And
> you've slapped your r-b onto it already.
> 
> > 
> > > +	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > > +	spin_unlock_irq(&dev_priv->irq_lock);
> > > +	/* unmask all PM interrupts */
> > > +	I915_WRITE(GEN6_PMINTRMSK, 0);
> > > +}
> > > +
> > 
> > Also, maybe move this (with disable interrupts) to i915_irq.c?
> 
> More forward declarations, which are my rough (and sometimes misguided
> metric) to judge whether a function is at the right place. Hence I've
> decided against moving it. Has the downside that not all code touching
> PMIMR is at the same place, but alas. We can't share a commen
> enable/disable_pm_interrupts like we have for display interrupt registers
> and similar stuff since we want to do a bit more than just set up
> interrupt registers under the spinlock protection.
> 
> Also, the other guy toching PMIMR at runtime lives in intel_ringbuffer.c,
> so moving this to i915_irq.c won't bring them any closer.
> -Daniel
> 
> > 
> > >  static void gen6_enable_rps(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> > >  
> > >  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> > >  
> > > -	spin_lock_irq(&dev_priv->irq_lock);
> > > -	/* FIXME: Our interrupt enabling sequence is bonghits.
> > > -	 * dev_priv->rps.pm_iir really should be 0 here. */
> > > -	dev_priv->rps.pm_iir = 0;
> > > -	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > > -	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > > -	spin_unlock_irq(&dev_priv->irq_lock);
> > > -	/* unmask all PM interrupts */
> > > -	I915_WRITE(GEN6_PMINTRMSK, 0);
> > > +	gen6_enable_rps_interrupts(dev);
> > >  
> > >  	rc6vids = 0;
> > >  	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > > @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > >  
> > >  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> > >  
> > > -	spin_lock_irq(&dev_priv->irq_lock);
> > > -	WARN_ON(dev_priv->rps.pm_iir != 0);
> > > -	I915_WRITE(GEN6_PMIMR, 0);
> > > -	spin_unlock_irq(&dev_priv->irq_lock);
> > > -	/* enable all PM interrupts */
> > > -	I915_WRITE(GEN6_PMINTRMSK, 0);
> > > +	gen6_enable_rps_interrupts(dev);
> > >  
> > >  	gen6_gt_force_wake_put(dev_priv);
> > >  }
> > 
> > -- 
> > Ben Widawsky, Intel Open Source Technology Center
> 

FWIW, I'd still prefer all the non interrupt handling code to be in
i915_irq.c (with intel_ringbuffer.c as the only exception), but I think
you make a fair point. 
As long as my other comments are not forgotten..
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: unify PM interrupt preinstall sequence

[Ben Widawsky]

On Fri, Jul 12, 2013 at 10:43:25PM +0200, Daniel Vetter wrote:
> Since the addition of VECS we have a slightly different enable
> sequence for PM interrupts on ivb/hsw vs snb and vlv. Usually that
> will end up in hard to track down surprises.
> 
> Hence unifiy things and since we have copies of this code in 3 places
> now, extract it into its own little helper.
> 
> Note that this changes the irq preinstall sequence a bit for snb and
> vlv: We now also clear the PM registers in the preinstall hook, in
> addition to the PM register clearing/setup already done when actually
> enabling rps. So this doesn't fix a bug but simply unifies the code
> across all platforms. After the postinstall hook is similarly unified
> we can rip out the then redundant PM interrupt setup from the rps
> code.
> 
> v3: Rebase on top of the retained double-GTIIR clearing. Also
> resurrect the masking/disabling of the gen6+ PM interrupts as spotted
> by Ben Widaswky.
> 
> v4: Move the DE interrupt reset code out of gen5_gt_irq_preinstall
> back to ironlake_irq_preinstall where it really belongs. Spotted by
> Paulo.
> 
> v3: Improve the commit message to more clearly spell out why we want
> to unify the code and what exactly changes.
> 
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cf1a21a..d5c3bef 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2583,6 +2583,23 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>  	POSTING_READ(SDEIER);
>  }
>  
> +static void gen5_gt_irq_preinstall(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* and GT */
> +	I915_WRITE(GTIMR, 0xffffffff);
> +	I915_WRITE(GTIER, 0x0);
> +	POSTING_READ(GTIER);
> +
> +	if (INTEL_INFO(dev)->gen >= 6) {
> +		/* and GT */

and PM?
-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 13/14] drm/i915: extract rps interrupt enable/disable helpers

[Daniel Vetter]

This just unifies the vlv code with the snb-hsw code which matched
exactly before the VECS enabling.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 96f0872..787a528 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
 }
 
-
-static void gen6_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(GEN6_RC_CONTROL, 0);
-	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
 	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
 	/* Complete PM interrupt masking here doesn't race with the rps work
@@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
 }
 
-static void valleyview_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	I915_WRITE(GEN6_RC_CONTROL, 0);
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-	I915_WRITE(GEN6_PMIER, 0);
-	/* Complete PM interrupt masking here doesn't race with the rps work
-	 * item again unmasking PM interrupts because that is using a different
-	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
-	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
+	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir = 0;
-	spin_unlock_irq(&dev_priv->irq_lock);
+	gen6_disable_rps_interrupts(dev);
+}
+
+static void valleyview_disable_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_RC_CONTROL, 0);
 
-	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
+	gen6_disable_rps_interrupts(dev);
 
 	if (dev_priv->vlv_pctx) {
 		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
@@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
 	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
+static void gen6_enable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	/* FIXME: Our interrupt enabling sequence is bonghits.
+	 * dev_priv->rps.pm_iir really should be 0 here. */
+	dev_priv->rps.pm_iir = 0;
+	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
+	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
+	spin_unlock_irq(&dev_priv->irq_lock);
+	/* unmask all PM interrupts */
+	I915_WRITE(GEN6_PMINTRMSK, 0);
+}
+
 static void gen6_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	/* FIXME: Our interrupt enabling sequence is bonghits.
-	 * dev_priv->rps.pm_iir really should be 0 here. */
-	dev_priv->rps.pm_iir = 0;
-	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
-	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
-	spin_unlock_irq(&dev_priv->irq_lock);
-	/* unmask all PM interrupts */
-	I915_WRITE(GEN6_PMINTRMSK, 0);
+	gen6_enable_rps_interrupts(dev);
 
 	rc6vids = 0;
 	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
@@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	WARN_ON(dev_priv->rps.pm_iir != 0);
-	I915_WRITE(GEN6_PMIMR, 0);
-	spin_unlock_irq(&dev_priv->irq_lock);
-	/* enable all PM interrupts */
-	I915_WRITE(GEN6_PMINTRMSK, 0);
+	gen6_enable_rps_interrupts(dev);
 
 	gen6_gt_force_wake_put(dev_priv);
 }
-- 
1.8.1.4

Re: [PATCH 13/14] drm/i915: extract rps interrupt enable/disable helpers

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This just unifies the vlv code with the snb-hsw code which matched
> exactly before the VECS enabling.

So now the VLV code is behaving differently. Is that intentional? You
could write about the implications here.


>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 96f0872..787a528 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>         trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
>  }
>
> -
> -static void gen6_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps_interrupts(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       I915_WRITE(GEN6_RC_CONTROL, 0);
> -       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>         I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>         I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
>         /* Complete PM interrupt masking here doesn't race with the rps work
> @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
>         I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
>  }
>
> -static void valleyview_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         I915_WRITE(GEN6_RC_CONTROL, 0);
> -       I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> -       I915_WRITE(GEN6_PMIER, 0);
> -       /* Complete PM interrupt masking here doesn't race with the rps work
> -        * item again unmasking PM interrupts because that is using a different
> -        * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> -        * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> +       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       dev_priv->rps.pm_iir = 0;
> -       spin_unlock_irq(&dev_priv->irq_lock);
> +       gen6_disable_rps_interrupts(dev);
> +}
> +
> +static void valleyview_disable_rps(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       I915_WRITE(GEN6_RC_CONTROL, 0);
>
> -       I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> +       gen6_disable_rps_interrupts(dev);
>
>         if (dev_priv->vlv_pctx) {
>                 drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
>         return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
>  }
>
> +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       spin_lock_irq(&dev_priv->irq_lock);
> +       /* FIXME: Our interrupt enabling sequence is bonghits.
> +        * dev_priv->rps.pm_iir really should be 0 here. */
> +       dev_priv->rps.pm_iir = 0;
> +       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> +       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> +       spin_unlock_irq(&dev_priv->irq_lock);
> +       /* unmask all PM interrupts */
> +       I915_WRITE(GEN6_PMINTRMSK, 0);
> +}
> +
>  static void gen6_enable_rps(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>
>         gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       /* FIXME: Our interrupt enabling sequence is bonghits.
> -        * dev_priv->rps.pm_iir really should be 0 here. */
> -       dev_priv->rps.pm_iir = 0;
> -       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> -       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> -       spin_unlock_irq(&dev_priv->irq_lock);
> -       /* unmask all PM interrupts */
> -       I915_WRITE(GEN6_PMINTRMSK, 0);
> +       gen6_enable_rps_interrupts(dev);
>
>         rc6vids = 0;
>         ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>
>         valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       WARN_ON(dev_priv->rps.pm_iir != 0);
> -       I915_WRITE(GEN6_PMIMR, 0);
> -       spin_unlock_irq(&dev_priv->irq_lock);
> -       /* enable all PM interrupts */
> -       I915_WRITE(GEN6_PMINTRMSK, 0);
> +       gen6_enable_rps_interrupts(dev);
>
>         gen6_gt_force_wake_put(dev_priv);
>  }
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 13/14] drm/i915: extract rps interrupt enable/disable helpers

[Daniel Vetter]

On Wed, Jul 10, 2013 at 06:12:13PM -0300, Paulo Zanoni wrote:
> 2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > This just unifies the vlv code with the snb-hsw code which matched
> > exactly before the VECS enabling.
> 
> So now the VLV code is behaving differently. Is that intentional? You
> could write about the implications here.

It's just for consistency that we now also reset GEN6_PMINTRMSK for vlv
and that the additional clearing of the rps event bits in PMIIR now
consistently happens in enable_interrupts. Like I've said in the next
commit message, the PMIIR handling here still smells a bit funny - as long
as we properly clear them at irq install time and keep the actual rps
events masked we should never see PMIIR bits here.
-Daniel

> 
> 
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
> >  1 file changed, 29 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 96f0872..787a528 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> >         trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
> >  }
> >
> > -
> > -static void gen6_disable_rps(struct drm_device *dev)
> > +static void gen6_disable_rps_interrupts(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > -       I915_WRITE(GEN6_RC_CONTROL, 0);
> > -       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >         I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> >         I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
> >         /* Complete PM interrupt masking here doesn't race with the rps work
> > @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
> >         I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> >  }
> >
> > -static void valleyview_disable_rps(struct drm_device *dev)
> > +static void gen6_disable_rps(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> >         I915_WRITE(GEN6_RC_CONTROL, 0);
> > -       I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > -       I915_WRITE(GEN6_PMIER, 0);
> > -       /* Complete PM interrupt masking here doesn't race with the rps work
> > -        * item again unmasking PM interrupts because that is using a different
> > -        * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> > -        * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> > +       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       dev_priv->rps.pm_iir = 0;
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> > +       gen6_disable_rps_interrupts(dev);
> > +}
> > +
> > +static void valleyview_disable_rps(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       I915_WRITE(GEN6_RC_CONTROL, 0);
> >
> > -       I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > +       gen6_disable_rps_interrupts(dev);
> >
> >         if (dev_priv->vlv_pctx) {
> >                 drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> > @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
> >         return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> >  }
> >
> > +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       spin_lock_irq(&dev_priv->irq_lock);
> > +       /* FIXME: Our interrupt enabling sequence is bonghits.
> > +        * dev_priv->rps.pm_iir really should be 0 here. */
> > +       dev_priv->rps.pm_iir = 0;
> > +       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > +       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > +       spin_unlock_irq(&dev_priv->irq_lock);
> > +       /* unmask all PM interrupts */
> > +       I915_WRITE(GEN6_PMINTRMSK, 0);
> > +}
> > +
> >  static void gen6_enable_rps(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> >
> >         gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       /* FIXME: Our interrupt enabling sequence is bonghits.
> > -        * dev_priv->rps.pm_iir really should be 0 here. */
> > -       dev_priv->rps.pm_iir = 0;
> > -       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > -       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> > -       /* unmask all PM interrupts */
> > -       I915_WRITE(GEN6_PMINTRMSK, 0);
> > +       gen6_enable_rps_interrupts(dev);
> >
> >         rc6vids = 0;
> >         ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >
> >         valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       WARN_ON(dev_priv->rps.pm_iir != 0);
> > -       I915_WRITE(GEN6_PMIMR, 0);
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> > -       /* enable all PM interrupts */
> > -       I915_WRITE(GEN6_PMINTRMSK, 0);
> > +       gen6_enable_rps_interrupts(dev);
> >
> >         gen6_gt_force_wake_put(dev_priv);
> >  }
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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

[PATCH 14/14] drm/i915: simplify rps interrupt enabling/disabling sequence

[Daniel Vetter]

At the moment we have the following interrupt enabling sequence:
1. irq preinstall hook
2. enabling the interrupt handler and calling irq postinstall hook
3. enable rps interrupts from the async work

And the folliwing disable sequence:
1. disabling the interrupt handler and calling the uninstall hook
2. disabling the rps interrupt

Since the postinstall hook now always sets up PMIIR, PMIER and PMIMR
to known-good states there no way for an interrupt to sneak in in the
enable sequence, so we can reinstate the WARN lost in

commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue May 28 19:22:26 2013 -0700

    drm/i915: Add PM regs to pre/post install

Note that there's some room for future cleanups since most of the
interrupt register clearing in the disable function is rather
redundant. But that's better done in follow-up patches, if at all.

Cc: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 787a528..bc5aae0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3193,9 +3193,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	/* FIXME: Our interrupt enabling sequence is bonghits.
-	 * dev_priv->rps.pm_iir really should be 0 here. */
-	dev_priv->rps.pm_iir = 0;
+	WARN_ON(dev_priv->rps.pm_iir);
 	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
 	spin_unlock_irq(&dev_priv->irq_lock);
-- 
1.8.1.4

Re: [PATCH 14/14] drm/i915: simplify rps interrupt enabling/disabling sequence

[Daniel Vetter]

On Thu, Jul 04, 2013 at 11:35:34PM +0200, Daniel Vetter wrote:
> At the moment we have the following interrupt enabling sequence:
> 1. irq preinstall hook
> 2. enabling the interrupt handler and calling irq postinstall hook
> 3. enable rps interrupts from the async work
> 
> And the folliwing disable sequence:
> 1. disabling the interrupt handler and calling the uninstall hook
> 2. disabling the rps interrupt
> 
> Since the postinstall hook now always sets up PMIIR, PMIER and PMIMR
> to known-good states there no way for an interrupt to sneak in in the
> enable sequence, so we can reinstate the WARN lost in
> 
> commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Tue May 28 19:22:26 2013 -0700
> 
>     drm/i915: Add PM regs to pre/post install
> 
> Note that there's some room for future cleanups since most of the
> interrupt register clearing in the disable function is rather
> redundant. But that's better done in follow-up patches, if at all.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok, final 4 patches from this series are now merged. Thanks everyone for
the review.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 787a528..bc5aae0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3193,9 +3193,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> -	/* FIXME: Our interrupt enabling sequence is bonghits.
> -	 * dev_priv->rps.pm_iir really should be 0 here. */
> -	dev_priv->rps.pm_iir = 0;
> +	WARN_ON(dev_priv->rps.pm_iir);
>  	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
>  	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
>  	spin_unlock_irq(&dev_priv->irq_lock);
> -- 
> 1.8.1.4
> 

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