| Message ID | 818eab08de0d5210ebe0e80ead3a7e8e6d9d8590.1367223972.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 29, 2013 at 01:02:52PM +0300, Jani Nikula wrote: > Stop calling intel_opregion_enable_asle() and consequently > intel_enable_asle() on opregion init. It should not be necessary for > these reasons: > > 1) On PCH split platforms, it only enables GSE interrupt, which is > enabled in irq postinstall anyway. Moreover, the irq enable uses the > wrong bit on IVB+. > > 2) On gen 2, it would enable a reserved pipestat bit. If there were gen > 2 systems with opregion asle support, that is. And the gen 2 irq > handler won't handle it anyway. > > 3) On gen 3-4, the irq postinstall will call > intel_opregion_enable_asle() to enable the pipestat. > > In short, move the asle irq/pipestat enable responsibility to irq > postinstall, which already happens to be in place. > > This should not cause any functional changes, but only do the one line > change here for easier bisectability, just in case, and leave all the > cleanups this allows to followup patches. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
On Mon, Apr 29, 2013 at 01:02:52PM +0300, Jani Nikula wrote: > Stop calling intel_opregion_enable_asle() and consequently > intel_enable_asle() on opregion init. It should not be necessary for > these reasons: > > 1) On PCH split platforms, it only enables GSE interrupt, which is > enabled in irq postinstall anyway. Moreover, the irq enable uses the > wrong bit on IVB+. > > 2) On gen 2, it would enable a reserved pipestat bit. If there were gen > 2 systems with opregion asle support, that is. And the gen 2 irq > handler won't handle it anyway. > > 3) On gen 3-4, the irq postinstall will call > intel_opregion_enable_asle() to enable the pipestat. > > In short, move the asle irq/pipestat enable responsibility to irq > postinstall, which already happens to be in place. > > This should not cause any functional changes, but only do the one line > change here for easier bisectability, just in case, and leave all the > cleanups this allows to followup patches. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Oh wait, this means that at this commit we don't enable pipestat on gen 3/4 and end up with a broken backlight? maybe this untangling could also make the postinstall functions call intel_enable_asle()?
On Mon, Apr 29, 2013 at 12:29:24PM +0100, Damien Lespiau wrote: > On Mon, Apr 29, 2013 at 01:02:52PM +0300, Jani Nikula wrote: > > Stop calling intel_opregion_enable_asle() and consequently > > intel_enable_asle() on opregion init. It should not be necessary for > > these reasons: > > > > 1) On PCH split platforms, it only enables GSE interrupt, which is > > enabled in irq postinstall anyway. Moreover, the irq enable uses the > > wrong bit on IVB+. > > > > 2) On gen 2, it would enable a reserved pipestat bit. If there were gen > > 2 systems with opregion asle support, that is. And the gen 2 irq > > handler won't handle it anyway. > > > > 3) On gen 3-4, the irq postinstall will call > > intel_opregion_enable_asle() to enable the pipestat. > > > > In short, move the asle irq/pipestat enable responsibility to irq > > postinstall, which already happens to be in place. > > > > This should not cause any functional changes, but only do the one line > > change here for easier bisectability, just in case, and leave all the > > cleanups this allows to followup patches. > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > Oh wait, this means that at this commit we don't enable pipestat on gen > 3/4 and end up with a broken backlight? maybe this untangling could also > make the postinstall functions call intel_enable_asle()? Disregard that comment, the naming was confusing after all...
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 5b6d202..4e69799 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -427,8 +427,6 @@ void intel_opregion_init(struct drm_device *dev) } if (opregion->asle) { - intel_opregion_enable_asle(dev); - iowrite32(ASLE_TCHE_BLC_EN, &opregion->asle->tche); iowrite32(ASLE_ARDY_READY, &opregion->asle->ardy); }
Stop calling intel_opregion_enable_asle() and consequently intel_enable_asle() on opregion init. It should not be necessary for these reasons: 1) On PCH split platforms, it only enables GSE interrupt, which is enabled in irq postinstall anyway. Moreover, the irq enable uses the wrong bit on IVB+. 2) On gen 2, it would enable a reserved pipestat bit. If there were gen 2 systems with opregion asle support, that is. And the gen 2 irq handler won't handle it anyway. 3) On gen 3-4, the irq postinstall will call intel_opregion_enable_asle() to enable the pipestat. In short, move the asle irq/pipestat enable responsibility to irq postinstall, which already happens to be in place. This should not cause any functional changes, but only do the one line change here for easier bisectability, just in case, and leave all the cleanups this allows to followup patches. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_opregion.c | 2 -- 1 file changed, 2 deletions(-)