Re: [PATCH 10/14] drm/i915: Refactor vlv_display_irq_uninstall()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 30, 2014 at 06:37:46PM -0200, Paulo Zanoni wrote:
> 2014-10-30 18:22 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Pull the vlv display irq uninstall code into a separate function, for
> >> eventual sharing with chv.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++----------
> >>  1 file changed, 15 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 38e57dd..b05dee5 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -3585,6 +3585,20 @@ static void gen8_irq_uninstall(struct drm_device *dev)
> >>         gen8_irq_reset(dev);
> >>  }
> >>
> >> +static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
> 
> Actually, now we have vlv_display_irq_uninstall() and
> valleyview_display_irqs_uninstall(), which is very confusing. Please
> rename something :)

Yeah that did bug me as well. I think I need to come up with another
name for the valleyview_display_irqs_* functions because the other ones
already get their names from the drm interrupt setup split. But I'll
need to give it some thought so I don't make things even worse with a
bad name :)

> 
> >> +{
> >> +       /* Interrupt setup is already guaranteed to be single-threaded, this is
> >> +        * just to make the assert_spin_locked check happy. */
> >> +       spin_lock_irq(&dev_priv->irq_lock);
> >> +       if (dev_priv->display_irqs_enabled)
> >> +               valleyview_display_irqs_uninstall(dev_priv);
> >> +       spin_unlock_irq(&dev_priv->irq_lock);
> >
> > I wonder how much I'll be able to review without needing to understand
> > why we have the chunk above...
> >
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> >> +
> >> +       vlv_display_irq_reset(dev_priv);
> >> +
> >> +       dev_priv->irq_mask = 0;
> >> +}
> >> +
> >>  static void valleyview_irq_uninstall(struct drm_device *dev)
> >>  {
> >>         struct drm_i915_private *dev_priv = dev->dev_private;
> >> @@ -3598,16 +3612,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >>
> >>         I915_WRITE(HWSTAM, 0xffffffff);
> >>
> >> -       /* Interrupt setup is already guaranteed to be single-threaded, this is
> >> -        * just to make the assert_spin_locked check happy. */
> >> -       spin_lock_irq(&dev_priv->irq_lock);
> >> -       if (dev_priv->display_irqs_enabled)
> >> -               valleyview_display_irqs_uninstall(dev_priv);
> >> -       spin_unlock_irq(&dev_priv->irq_lock);
> >> -
> >> -       vlv_display_irq_reset(dev_priv);
> >> -
> >> -       dev_priv->irq_mask = 0;
> >> +       vlv_display_irq_uninstall(dev_priv);
> >>  }
> >>
> >>  static void cherryview_irq_uninstall(struct drm_device *dev)
> >> --
> >> 2.0.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx