[Intel-gfx] [PATCH 08/14] drm/i915: Make valleyview_display_irqs_(un)install() work for chv

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 06:12:51PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala at linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Genralize valleyview_display_irqs_install() and
> > valleyview_display_irqs_uninstall() enough so that they work on chv.
> > The only difference to vlv here being the third pipe that chv brings.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 67c046b..50431f6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3335,24 +3335,27 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
> >  {
> >         u32 pipestat_mask;
> >         u32 iir_mask;
> > +       enum pipe pipe;
> >
> >         pipestat_mask = PIPESTAT_INT_STATUS_MASK |
> >                         PIPE_FIFO_UNDERRUN_STATUS;
> >
> > -       I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> > -       I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> > +       for_each_pipe(dev_priv, pipe)
> > +               I915_WRITE(PIPESTAT(pipe), pipestat_mask);
> >         POSTING_READ(PIPESTAT(PIPE_A));
> >
> >         pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> >                         PIPE_CRC_DONE_INTERRUPT_STATUS;
> >
> > -       i915_enable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> > -                                              PIPE_GMBUS_INTERRUPT_STATUS);
> > -       i915_enable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> > +       i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> > +       for_each_pipe(dev_priv, pipe)
> > +                     i915_enable_pipestat(dev_priv, pipe, pipestat_mask);
> 
> While trying to check for the correctness of the lines above, I
> noticed that in __i915_enable_pipestat(), if the enable mask is
> already what we want, we won't clear/update the status bits. Is that
> correct? Why? Anyway, any problems in that function should be fixed by
> a separate patch.

I think the current behaviour is correct. We don't want to lose
interrupts in case someone enables the same pipestat bit twice. But
obviously then disabling twice doesn't work because we don't refcount
the individual bits. So perhaps we want to print a warning if some of
the bits we're trying to set are already set?

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> >
> >         iir_mask = I915_DISPLAY_PORT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> > +       if (IS_CHERRYVIEW(dev_priv))
> > +               iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> >         dev_priv->irq_mask &= ~iir_mask;
> >
> >         I915_WRITE(VLV_IIR, iir_mask);
> > @@ -3366,10 +3369,13 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> >  {
> >         u32 pipestat_mask;
> >         u32 iir_mask;
> > +       enum pipe pipe;
> >
> >         iir_mask = I915_DISPLAY_PORT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> > +       if (IS_CHERRYVIEW(dev_priv))
> > +               iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> >
> >         dev_priv->irq_mask |= iir_mask;
> >         I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > @@ -3381,14 +3387,15 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> >         pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> >                         PIPE_CRC_DONE_INTERRUPT_STATUS;
> >
> > -       i915_disable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> > -                                               PIPE_GMBUS_INTERRUPT_STATUS);
> > -       i915_disable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> > +       i915_disable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> > +       for_each_pipe(dev_priv, pipe)
> > +               i915_disable_pipestat(dev_priv, pipe, pipestat_mask);
> >
> >         pipestat_mask = PIPESTAT_INT_STATUS_MASK |
> >                         PIPE_FIFO_UNDERRUN_STATUS;
> > -       I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> > -       I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> > +
> > +       for_each_pipe(dev_priv, pipe)
> > +               I915_WRITE(PIPESTAT(pipe), pipestat_mask);
> >         POSTING_READ(PIPESTAT(PIPE_A));
> >  }
> >
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC