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

[Daniel Vetter]

On Mon, Mar 18, 2013 at 10:00 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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?

Also maybe time to extract the backlight stuff into a substruct ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch