[Intel-gfx] [PATCH] drm/i915: remove redundant and partially broken GSE interrupt enable

[Daniel Vetter]

On Tue, Mar 12, 2013 at 9:50 AM, Jani Nikula <jani.nikula at intel.com> wrote:
> GSE interrupts are always enabled on PCH split platforms, so remove the
> redundant enable for ASLE. Moreover, the same interrupt bit was used on all
> PCH split platforms, even though the bit definitions changed in IVB, thus
> unmasking a reserved bit.
>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>
> ---
>
> An alternative to this patch would be keeping GSE interrupts masked until
> separately enabled. The question is, when are we ready to handle GSE
> interrupts? And if we need to care about that, would the right choice be to
> mask the interrupts, or rather tell the BIOS through opregion ARDY/DRDY
> fields that we are not ready yet?
>
> I chose the approach in this patch because it's the smallest change towards
> being more correct; removing a NOP unmask for ILK+SNB and removing a bogus
> unmask for IVB and later.
>
> Let the bikeshedding begin. ;)

Ok, I've read through the entire mess. There's more than just a bit of
fishy here:
- The backlight state isn't protected by locks, despite that we can
change it from a lot of contexts: Through modesets, concurrently
through the backlight interface directly, and in irq context through
asle request. I think we need a spinlock here ...
- We set up the opregion stuff way before enabling interrupts. Which
means we can probably kill all the callers of this, safe for the
postinstall hooks. And there we only need it on the pipestate based
machines, so maybe rename that function.
- intel_opregion_asle_intr and intel_opregion_gse_intr are almost the
same functions, safe that the later has many fewer features. I'm
somewhat hopeful that this might alleviate some of our backlight bugs
on more recent platforms, if we'd bother to implement this. Imo the
right approach would be to pimp the asle callbacks to work on ilk and
then kill the gse_intr. This particular mess goes back to the original
ilk opregion enabling in

commit 01c66889c14aa163c49355b3be2ccfb214500599
Author: Zhao Yakui <yakui.zhao at intel.com>
Date:   Wed Oct 28 05:10:00 2009 +0000

    drm/i915: Add ACPI OpRegion support for Ironlake

There's probably more. Volunteered?

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2139714..ba47ec0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -98,17 +98,15 @@ void intel_enable_asle(struct drm_device *dev)
>         if (IS_VALLEYVIEW(dev))
>                 return;
>
> +       /* GSE interrupt is always enabled */
> +       if (HAS_PCH_SPLIT(dev))
> +               return;
> +
>         spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>
> -       if (HAS_PCH_SPLIT(dev))
> -               ironlake_enable_display_irq(dev_priv, DE_GSE);
> -       else {
> -               i915_enable_pipestat(dev_priv, 1,
> -                                    PIPE_LEGACY_BLC_EVENT_ENABLE);
> -               if (INTEL_INFO(dev)->gen >= 4)
> -                       i915_enable_pipestat(dev_priv, 0,
> -                                            PIPE_LEGACY_BLC_EVENT_ENABLE);
> -       }
> +       i915_enable_pipestat(dev_priv, 1, PIPE_LEGACY_BLC_EVENT_ENABLE);
> +       if (INTEL_INFO(dev)->gen >= 4)
> +               i915_enable_pipestat(dev_priv, 0, PIPE_LEGACY_BLC_EVENT_ENABLE);
>
>         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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