| Message ID | 1363078221-18886-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 12, 2013 at 9:50 AM, Jani Nikula <jani.nikula@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@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Mar 18, 2013 at 10:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Mar 12, 2013 at 9:50 AM, Jani Nikula <jani.nikula@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@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@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
On Mon, Mar 18, 2013 at 10:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > - 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. postinstall calls to intel_opregion_enable_asle look superflous, too - intel_opregion_init will take care of this already. And having differing enable points on different platforms isn't that great, either. -Daniel
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); }
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@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. ;) --- drivers/gpu/drm/i915/i915_irq.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)