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
