Re: [PATCH 2/3] drm/i915: Nuke drm_driver irq vfuncs

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

On Tue, Jun 18, 2019 at 03:54:01PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-06-18 15:21:07)
> [snip mechanical changes]
> 
> > @@ -4839,65 +4792,18 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> >         dev->driver->get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos;
> >         dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> >  
> > -       if (IS_CHERRYVIEW(dev_priv)) {
> > -               dev->driver->irq_handler = cherryview_irq_handler;
> > -               dev->driver->irq_preinstall = cherryview_irq_reset;
> > -               dev->driver->irq_postinstall = cherryview_irq_postinstall;
> > -               dev->driver->irq_uninstall = cherryview_irq_reset;
> > -               dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> > -       } else if (IS_VALLEYVIEW(dev_priv)) {
> > -               dev->driver->irq_handler = valleyview_irq_handler;
> > -               dev->driver->irq_preinstall = valleyview_irq_reset;
> > -               dev->driver->irq_postinstall = valleyview_irq_postinstall;
> > -               dev->driver->irq_uninstall = valleyview_irq_reset;
> > -               dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> > -       } else if (INTEL_GEN(dev_priv) >= 11) {
> > -               dev->driver->irq_handler = gen11_irq_handler;
> > -               dev->driver->irq_preinstall = gen11_irq_reset;
> > -               dev->driver->irq_postinstall = gen11_irq_postinstall;
> > -               dev->driver->irq_uninstall = gen11_irq_reset;
> > -               dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
> > -       } else if (INTEL_GEN(dev_priv) >= 8) {
> > -               dev->driver->irq_handler = gen8_irq_handler;
> > -               dev->driver->irq_preinstall = gen8_irq_reset;
> > -               dev->driver->irq_postinstall = gen8_irq_postinstall;
> > -               dev->driver->irq_uninstall = gen8_irq_reset;
> > -               if (IS_GEN9_LP(dev_priv))
> > +       if (HAS_GMCH(dev_priv)) {
> > +               if (I915_HAS_HOTPLUG(dev_priv))
> > +                       dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> 
> Includes chv/vlv.
> 
> > +       } else {
> > +               if (INTEL_GEN(dev_priv) >= 11)
> > +                       dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
> 
> Ok.
> 
> > +               else if (IS_GEN9_LP(dev_priv))
> >                         dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> >                 else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> >                         dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> 
> As before.
> 
> >                 else
> >                         dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> 
> The rest of !GMCH.
> 
> Code be one level if-chain though.

"could be ..."? Sure. Not entirely sure I'd be able to follow it though.
I guess just flattening it and checking for HAS_PCH_SPLIT() for ilk+
could be a reasonably non-confusing way to write it. Silly vlv/chv
always interfering with the mostly nice chronological order.

> 
> > @@ -4918,6 +4824,75 @@ void intel_irq_fini(struct drm_i915_private *i915)
> >                 kfree(i915->l3_parity.remap_info[i]);
> >  }
> >  
> > +static irq_handler_t intel_irq_handler(struct drm_i915_private *dev_priv)
> > +{
> > +       if (HAS_GMCH(dev_priv)) {
> > +               if (IS_CHERRYVIEW(dev_priv))
> > +                       return cherryview_irq_handler;
> > +               else if (IS_VALLEYVIEW(dev_priv))
> > +                       return valleyview_irq_handler;
> > +               else if (IS_GEN(dev_priv, 4))
> > +                       return i965_irq_handler;
> > +               else if (IS_GEN(dev_priv, 3))
> > +                       return i915_irq_handler;
> > +               else
> > +                       return i8xx_irq_handler;
> > +       } else {
> > +               if (INTEL_GEN(dev_priv) >= 11)
> > +                       return gen11_irq_handler;
> > +               else if (INTEL_GEN(dev_priv) >= 8)
> > +                       return gen8_irq_handler;
> > +               else
> > +                       return ironlake_irq_handler;
> > +       }
> > +}
> > +
> > +static void intel_irq_reset(struct drm_i915_private *dev_priv)
> > +{
> > +       if (HAS_GMCH(dev_priv)) {
> > +               if (IS_CHERRYVIEW(dev_priv))
> > +                       cherryview_irq_reset(dev_priv);
> > +               else if (IS_VALLEYVIEW(dev_priv))
> > +                       valleyview_irq_reset(dev_priv);
> > +               else if (IS_GEN(dev_priv, 4))
> > +                       i965_irq_reset(dev_priv);
> > +               else if (IS_GEN(dev_priv, 3))
> > +                       i915_irq_reset(dev_priv);
> > +               else
> > +                       i8xx_irq_reset(dev_priv);
> > +       } else {
> > +               if (INTEL_GEN(dev_priv) >= 11)
> > +                       gen11_irq_reset(dev_priv);
> > +               else if (INTEL_GEN(dev_priv) >= 8)
> > +                       gen8_irq_reset(dev_priv);
> > +               else
> > +                       ironlake_irq_reset(dev_priv);
> > +       }
> > +}
> > +
> > +static void intel_irq_postinstall(struct drm_i915_private *dev_priv)
> > +{
> > +       if (HAS_GMCH(dev_priv)) {
> > +               if (IS_CHERRYVIEW(dev_priv))
> > +                       cherryview_irq_postinstall(dev_priv);
> > +               else if (IS_VALLEYVIEW(dev_priv))
> > +                       valleyview_irq_postinstall(dev_priv);
> > +               else if (IS_GEN(dev_priv, 4))
> > +                       i965_irq_postinstall(dev_priv);
> > +               else if (IS_GEN(dev_priv, 3))
> > +                       i915_irq_postinstall(dev_priv);
> > +               else
> > +                       i8xx_irq_postinstall(dev_priv);
> > +       } else {
> > +               if (INTEL_GEN(dev_priv) >= 11)
> > +                       gen11_irq_postinstall(dev_priv);
> > +               else if (INTEL_GEN(dev_priv) >= 8)
> > +                       gen8_irq_postinstall(dev_priv);
> > +               else
> > +                       ironlake_irq_postinstall(dev_priv);
> > +       }
> > +}
> > +
> >  /**
> >   * intel_irq_install - enables the hardware interrupt
> >   * @dev_priv: i915 device instance
> > @@ -4931,6 +4906,9 @@ void intel_irq_fini(struct drm_i915_private *i915)
> >   */
> >  int intel_irq_install(struct drm_i915_private *dev_priv)
> >  {
> > +       int irq = dev_priv->drm.pdev->irq;
> > +       int ret;
> > +
> >         /*
> >          * We enable some interrupt sources in our postinstall hooks, so mark
> >          * interrupts as enabled _before_ actually enabling them to avoid
> > @@ -4938,7 +4916,20 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> >          */
> >         dev_priv->runtime_pm.irqs_enabled = true;
> >  
> > -       return drm_irq_install(&dev_priv->drm, dev_priv->drm.pdev->irq);
> > +       dev_priv->drm.irq_enabled = true;
> > +
> > +       intel_irq_reset(dev_priv);
> > +
> > +       ret = request_irq(irq, intel_irq_handler(dev_priv),
> 
> Oh fancy.

I was debating to vfunc or not to vfunc. Seemed simple enough without.
Though the repetition of the platform checks in intel_irq_*() is a bit
annoying.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

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