| Message ID | 918c526028f34df880d0eb5db6bd3b862092743e.1346136448.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, You seem to be making exactly the same mistake I made. On Tue, August 28, 2012 08:53, Jani Nikula wrote: > The combination/legacy mode was first removed in > > commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb > Author: Indan Zupancic <indan@nul.nu> > Date: Thu Feb 17 02:41:49 2011 +0100 > > drm/i915: Do not handle backlight combination mode specially > > which was subsequently reverted due to regression in > > commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0 > Author: Takashi Iwai <tiwai@suse.de> > Date: Thu Mar 10 14:02:12 2011 +0100 > > drm/i915: Revive combination mode for backlight control > > It seems that on some machines the combination mode was necessary because > it reset the LBPC register value on resume. No, it was bad interaction with systems using ASLE to control backlight. To quote my old email: "On Thu, March 10, 2011 14:02, Takashi Iwai wrote: > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb > > drm/i915: Do not handle backlight combination mode specially > > since this commit introduced other regressions due to untouched LBPC > register, e.g. the backlight dimmed after resume. The regression was that, if ALSE was used, the maximum brightness would be the brightness at boot, because LBPC isn't touched and the BIOS restores it at boot. So the sympom would be less brightness levels or lower maximum brightness. Systems with no ASLE support work fine because there the code is only used to save and restore the PWM register. LBPC is saved and restored by the BIOS. The backlight dimming after resume/blanking was the original bug caused by the bogus val <<=1 shift." See http://marc.info/?l=dri-devel&m=129980668309522&w=2 > Since the driver now saves and > restores the register over suspend, the combination mode no longer needs to > be treated specially, and the driver doesn't need to modify the LBPC > register at all. That's what I thought when I wrote my original patch. But that only seems to be true for GEN2 hardware which has no ASLE. It seems GEN4 with ASLE needs the combination mode check because there the BIOS might use LBPC to set the brightness at system boot, even though the driver only uses the PWM registers to set it. (If the BIOS does something similar at resume time then it's really messed up.) But assuming it only happens at bootup, you could check for it once at module init and turn it off if it's enabled. But it seems you can't totally ignore legacy/combination mode. Some backlight problems on GEN4 can be solved by not fiddling with the backlight. The current code sets the backlight to 0 to disable the panel (last year anyway, maybe the code changed), but on gen >= 4 it can do the same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight value doesn't need to be saved anywhere. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153 > CC: Indan Zupancic <indan@nul.nu> > CC: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- > 1 file changed, 32 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index 9546f97..0a7f47a 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -115,19 +115,6 @@ done: > dev_priv->pch_pf_size = (width << 16) | height; > } > > -static int is_backlight_combination_mode(struct drm_device *dev) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - if (INTEL_INFO(dev)->gen >= 4) > - return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE; > - > - if (IS_GEN2(dev)) > - return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE; > - > - return 0; > -} > - > static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv) > { > u32 val; > @@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) > max >>= 17; > else > max >>= 16; > - > - if (is_backlight_combination_mode(dev)) > - max *= 0xff; > } > > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); > @@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; > if (INTEL_INFO(dev)->gen < 4) > val >>= 1; > - > - if (is_backlight_combination_mode(dev)) { > - u8 lbpc; > - > - pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); > - val *= lbpc; > - } > } > > val = intel_panel_compute_brightness(dev, val); > @@ -254,15 +231,6 @@ static void intel_panel_actually_set_backlight(struct drm_device > *dev, u32 level > if (HAS_PCH_SPLIT(dev)) > return intel_pch_panel_set_backlight(dev, level); > > - if (is_backlight_combination_mode(dev)) { > - u32 max = intel_panel_get_max_backlight(dev); > - u8 lbpc; > - > - lbpc = level * 0xfe / max + 1; > - level /= lbpc; > - pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > - } > - > tmp = I915_READ(BLC_PWM_CTL); > if (INTEL_INFO(dev)->gen < 4) > level <<= 1; > -- > 1.7.9.5 > > Greetings, Indan
On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote: > Hello, > > You seem to be making exactly the same mistake I made. > > On Tue, August 28, 2012 08:53, Jani Nikula wrote: > > The combination/legacy mode was first removed in > > > > commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb > > Author: Indan Zupancic <indan@nul.nu> > > Date: Thu Feb 17 02:41:49 2011 +0100 > > > > drm/i915: Do not handle backlight combination mode specially > > > > which was subsequently reverted due to regression in > > > > commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0 > > Author: Takashi Iwai <tiwai@suse.de> > > Date: Thu Mar 10 14:02:12 2011 +0100 > > > > drm/i915: Revive combination mode for backlight control > > > > It seems that on some machines the combination mode was necessary because > > it reset the LBPC register value on resume. > > No, it was bad interaction with systems using ASLE to control backlight. > > To quote my old email: > > "On Thu, March 10, 2011 14:02, Takashi Iwai wrote: > > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb > > > > drm/i915: Do not handle backlight combination mode specially > > > > since this commit introduced other regressions due to untouched LBPC > > register, e.g. the backlight dimmed after resume. > > The regression was that, if ALSE was used, the maximum brightness > would be the brightness at boot, because LBPC isn't touched and > the BIOS restores it at boot. So the sympom would be less brightness > levels or lower maximum brightness. > > Systems with no ASLE support work fine because there the code is only > used to save and restore the PWM register. LBPC is saved and restored > by the BIOS. > > The backlight dimming after resume/blanking was the original bug > caused by the bogus val <<=1 shift." > > See http://marc.info/?l=dri-devel&m=129980668309522&w=2 Meh, I've totally failed to dig this out. Makes quite a bit more sense and clarifies the confusion revert commit message quite a bit. Thanks for digging this out again, I guess we should include it. > > Since the driver now saves and > > restores the register over suspend, the combination mode no longer needs to > > be treated specially, and the driver doesn't need to modify the LBPC > > register at all. > > That's what I thought when I wrote my original patch. But that only seems > to be true for GEN2 hardware which has no ASLE. It seems GEN4 with ASLE > needs the combination mode check because there the BIOS might use LBPC to > set the brightness at system boot, even though the driver only uses the > PWM registers to set it. (If the BIOS does something similar at resume > time then it's really messed up.) But assuming it only happens at bootup, > you could check for it once at module init and turn it off if it's enabled. > But it seems you can't totally ignore legacy/combination mode. > > Some backlight problems on GEN4 can be solved by not fiddling with the > backlight. The current code sets the backlight to 0 to disable the panel > (last year anyway, maybe the code changed), but on gen >= 4 it can do the > same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight > value doesn't need to be saved anywhere. A while back I've improved the backlight code to properly switch the pipe that controls the pwm and also to properly toggle the enable bit for gen4+, see the new intel_panel_enable_backlight functions. Would it be correct in your opinion to simply ditch the call to intel_panel_actually_set_backlight for gen4+ unconditionally? Same for intel_panel_disable_backlight obviously? Thanks, Daniel > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153 > > CC: Indan Zupancic <indan@nul.nu> > > CC: Takashi Iwai <tiwai@suse.de> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- > > 1 file changed, 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index 9546f97..0a7f47a 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -115,19 +115,6 @@ done: > > dev_priv->pch_pf_size = (width << 16) | height; > > } > > > > -static int is_backlight_combination_mode(struct drm_device *dev) > > -{ > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - > > - if (INTEL_INFO(dev)->gen >= 4) > > - return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE; > > - > > - if (IS_GEN2(dev)) > > - return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE; > > - > > - return 0; > > -} > > - > > static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv) > > { > > u32 val; > > @@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) > > max >>= 17; > > else > > max >>= 16; > > - > > - if (is_backlight_combination_mode(dev)) > > - max *= 0xff; > > } > > > > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); > > @@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) > > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; > > if (INTEL_INFO(dev)->gen < 4) > > val >>= 1; > > - > > - if (is_backlight_combination_mode(dev)) { > > - u8 lbpc; > > - > > - pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); > > - val *= lbpc; > > - } > > } > > > > val = intel_panel_compute_brightness(dev, val); > > @@ -254,15 +231,6 @@ static void intel_panel_actually_set_backlight(struct drm_device > > *dev, u32 level > > if (HAS_PCH_SPLIT(dev)) > > return intel_pch_panel_set_backlight(dev, level); > > > > - if (is_backlight_combination_mode(dev)) { > > - u32 max = intel_panel_get_max_backlight(dev); > > - u8 lbpc; > > - > > - lbpc = level * 0xfe / max + 1; > > - level /= lbpc; > > - pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > > - } > > - > > tmp = I915_READ(BLC_PWM_CTL); > > if (INTEL_INFO(dev)->gen < 4) > > level <<= 1; > > -- > > 1.7.9.5 > > > > > > Greetings, > > Indan > >
On Tue, August 28, 2012 16:55, Daniel Vetter wrote: > On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote: >> Some backlight problems on GEN4 can be solved by not fiddling with the >> backlight. The current code sets the backlight to 0 to disable the panel >> (last year anyway, maybe the code changed), but on gen >= 4 it can do the >> same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight >> value doesn't need to be saved anywhere. > > A while back I've improved the backlight code to properly switch the pipe > that controls the pwm and also to properly toggle the enable bit for > gen4+, see the new intel_panel_enable_backlight functions. Would it be > correct in your opinion to simply ditch the call to > intel_panel_actually_set_backlight for gen4+ unconditionally? Same for > intel_panel_disable_backlight obviously? Yes, that seems the whole point of having the PWM disable bit. I would also ditch the backlight_enabled state tracking, as it's unnecessary and incorrect because the enable/disable calls aren't balanced. I'll update my kernel to the latest git code and try out how the current code works and if not touching LBPC at all works for gen 2 hardware. If it doesn't then LBPC needs to be saved/restored in i915_suspend.c after all. It would be nice if backlight control worked with ASLE disabled, that would get rid of all complexity, including combination mode. Or alternatively, if disabling combination mode at boot works then we can get rid of the complicated brightness setting code to cope with it. Greetings, Indan
On Thu, 30 Aug 2012 11:29:11 +0200 "Indan Zupancic" <indan@nul.nu> wrote: > On Tue, August 28, 2012 16:55, Daniel Vetter wrote: > > On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote: > >> Some backlight problems on GEN4 can be solved by not fiddling with the > >> backlight. The current code sets the backlight to 0 to disable the panel > >> (last year anyway, maybe the code changed), but on gen >= 4 it can do the > >> same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight > >> value doesn't need to be saved anywhere. > > > > A while back I've improved the backlight code to properly switch the pipe > > that controls the pwm and also to properly toggle the enable bit for > > gen4+, see the new intel_panel_enable_backlight functions. Would it be > > correct in your opinion to simply ditch the call to > > intel_panel_actually_set_backlight for gen4+ unconditionally? Same for > > intel_panel_disable_backlight obviously? > > Yes, that seems the whole point of having the PWM disable bit. > > I would also ditch the backlight_enabled state tracking, as it's > unnecessary and incorrect because the enable/disable calls aren't > balanced. > > I'll update my kernel to the latest git code and try out how the > current code works and if not touching LBPC at all works for gen 2 > hardware. If it doesn't then LBPC needs to be saved/restored in > i915_suspend.c after all. > > It would be nice if backlight control worked with ASLE disabled, > that would get rid of all complexity, including combination mode. > Or alternatively, if disabling combination mode at boot works then > we can get rid of the complicated brightness setting code to cope > with it. Did we bottom out on this discussion? Reading through the thread I don't see any firm conclusions, and we definitely still have bugs open that may be resolved by this incorrect patchset...
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 9546f97..0a7f47a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -115,19 +115,6 @@ done: dev_priv->pch_pf_size = (width << 16) | height; } -static int is_backlight_combination_mode(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - - if (INTEL_INFO(dev)->gen >= 4) - return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE; - - if (IS_GEN2(dev)) - return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE; - - return 0; -} - static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv) { u32 val; @@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; else max >>= 16; - - if (is_backlight_combination_mode(dev)) - max *= 0xff; } DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); @@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (INTEL_INFO(dev)->gen < 4) val >>= 1; - - if (is_backlight_combination_mode(dev)) { - u8 lbpc; - - pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); - val *= lbpc; - } } val = intel_panel_compute_brightness(dev, val); @@ -254,15 +231,6 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level); - if (is_backlight_combination_mode(dev)) { - u32 max = intel_panel_get_max_backlight(dev); - u8 lbpc; - - lbpc = level * 0xfe / max + 1; - level /= lbpc; - pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); - } - tmp = I915_READ(BLC_PWM_CTL); if (INTEL_INFO(dev)->gen < 4) level <<= 1;
The combination/legacy mode was first removed in commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb Author: Indan Zupancic <indan@nul.nu> Date: Thu Feb 17 02:41:49 2011 +0100 drm/i915: Do not handle backlight combination mode specially which was subsequently reverted due to regression in commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0 Author: Takashi Iwai <tiwai@suse.de> Date: Thu Mar 10 14:02:12 2011 +0100 drm/i915: Revive combination mode for backlight control It seems that on some machines the combination mode was necessary because it reset the LBPC register value on resume. Since the driver now saves and restores the register over suspend, the combination mode no longer needs to be treated specially, and the driver doesn't need to modify the LBPC register at all. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153 CC: Indan Zupancic <indan@nul.nu> CC: Takashi Iwai <tiwai@suse.de> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- 1 file changed, 32 deletions(-)