| Message ID | 179a19e78c99201a9ec18967c122a5a3aedbf555.1346136448.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 28 Aug 2012 09:53:36 +0300, Jani Nikula <jani.nikula@intel.com> wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > This is a prep patch to stop drm/i915 from changing the LBPC registers > itself - but we still need to properly save/restore it on > suspend/resume. My presumption was that there were BIOSes out there that only adjusted the LBPC values, and so any device setting that flag would likely only with it through the BIOS hotkeys. I couldn't see any other rational reason for the hardware maintaining multiple interfaces... -Chris
On Tue, Aug 28, 2012 at 08:16:54AM +0100, Chris Wilson wrote: > On Tue, 28 Aug 2012 09:53:36 +0300, Jani Nikula <jani.nikula@intel.com> wrote: > > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > This is a prep patch to stop drm/i915 from changing the LBPC registers > > itself - but we still need to properly save/restore it on > > suspend/resume. > > My presumption was that there were BIOSes out there that only adjusted > the LBPC values, and so any device setting that flag would likely only > with it through the BIOS hotkeys. I couldn't see any other rational > reason for the hardware maintaining multiple interfaces... I've run a hackish version of this through a few bug reports and they reported that there's no more black screen at boot-up, but also that the backlight doesn't work. My conclusion was: - there are machines that are only controlled through the lbpc reg - some of those use lbpc in an inverted sense - we have now idea how to figure out the sense of lbpc Hence my thinking is that we should avoid to touch lbpc (but just restore it across suspend/resume since some machines need that) and rely on any firmware madness to properly adjust the backlight. I also realize that this will regress machines without working firmware, but for which the current lbpc-adjusting mess seems to do the right thing. But if the only regression this causes is some non-working backlight adjusting I'll ignore that, since it looks like we fundamentally can't do the right thing with lbpc, and this series here should kill a few black screen bugs. Cheers, Daniel
Hello, On Tue, August 28, 2012 08:53, Jani Nikula wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > This is a prep patch to stop drm/i915 from changing the LBPC registers > itself - but we still need to properly save/restore it on > suspend/resume. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/i915_suspend.c | 8 ++++++++ > drivers/gpu/drm/i915/intel_panel.c | 2 -- > 4 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 58b43db..af1701c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -606,6 +606,7 @@ typedef struct drm_i915_private { > u32 savePP_CONTROL; > u32 savePP_DIVISOR; > u32 savePFIT_CONTROL; > + u8 saveLBPC; > u32 save_palette_a[256]; > u32 save_palette_b[256]; > u32 saveDPFC_CB_BASE; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index d0b60f2..3303c18 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1889,6 +1889,9 @@ > > #define PFIT_AUTO_RATIOS 0x61238 > > +/* legacy/combination backlight modes in pci config space. */ > +#define PCI_LBPC 0xf4 > + > /* Backlight control */ > #define BLC_PWM_CTL2 0x61250 /* 965+ only */ > #define BLM_PWM_ENABLE (1 << 31) > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c > index 4776ccf..05daff7 100644 > --- a/drivers/gpu/drm/i915/i915_suspend.c > +++ b/drivers/gpu/drm/i915/i915_suspend.c > @@ -639,6 +639,10 @@ static void i915_save_display(struct drm_device *dev) > dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2); > if (IS_MOBILE(dev) && !IS_I830(dev)) > dev_priv->saveLVDS = I915_READ(LVDS); > + > + if (IS_GEN2(dev) || IS_GEN4(dev)) > + pci_read_config_byte(dev->pdev, PCI_LBPC, > + &dev_priv->saveLBPC); What about GEN3? It seems weird that LBPC wouldn't be restored during resume by some BIOSes, is this really necessary? Greetings, Indan
On Tue, Aug 28, 2012 at 03:56:31PM +0200, Indan Zupancic wrote: > Hello, > > On Tue, August 28, 2012 08:53, Jani Nikula wrote: > > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > This is a prep patch to stop drm/i915 from changing the LBPC registers > > itself - but we still need to properly save/restore it on > > suspend/resume. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > > drivers/gpu/drm/i915/i915_suspend.c | 8 ++++++++ > > drivers/gpu/drm/i915/intel_panel.c | 2 -- > > 4 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 58b43db..af1701c 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -606,6 +606,7 @@ typedef struct drm_i915_private { > > u32 savePP_CONTROL; > > u32 savePP_DIVISOR; > > u32 savePFIT_CONTROL; > > + u8 saveLBPC; > > u32 save_palette_a[256]; > > u32 save_palette_b[256]; > > u32 saveDPFC_CB_BASE; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index d0b60f2..3303c18 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1889,6 +1889,9 @@ > > > > #define PFIT_AUTO_RATIOS 0x61238 > > > > +/* legacy/combination backlight modes in pci config space. */ > > +#define PCI_LBPC 0xf4 > > + > > /* Backlight control */ > > #define BLC_PWM_CTL2 0x61250 /* 965+ only */ > > #define BLM_PWM_ENABLE (1 << 31) > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c > > index 4776ccf..05daff7 100644 > > --- a/drivers/gpu/drm/i915/i915_suspend.c > > +++ b/drivers/gpu/drm/i915/i915_suspend.c > > @@ -639,6 +639,10 @@ static void i915_save_display(struct drm_device *dev) > > dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2); > > if (IS_MOBILE(dev) && !IS_I830(dev)) > > dev_priv->saveLVDS = I915_READ(LVDS); > > + > > + if (IS_GEN2(dev) || IS_GEN4(dev)) > > + pci_read_config_byte(dev->pdev, PCI_LBPC, > > + &dev_priv->saveLBPC); > > What about GEN3? > > It seems weird that LBPC wouldn't be restored during resume by some BIOSes, > is this really necessary? ba3820ade317ee36e496b9b40d2ec3987dd4aef0 claims so. But that commit managed to put too many things into the same thing unfortunately. -Daniel
On Tue, August 28, 2012 16:14, Daniel Vetter wrote: > On Tue, Aug 28, 2012 at 03:56:31PM +0200, Indan Zupancic wrote: >> Hello, >> >> On Tue, August 28, 2012 08:53, Jani Nikula wrote: >> > From: Daniel Vetter <daniel.vetter@ffwll.ch> >> > >> > This is a prep patch to stop drm/i915 from changing the LBPC registers >> > itself - but we still need to properly save/restore it on >> > suspend/resume. >> > >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> > --- [...] >> It seems weird that LBPC wouldn't be restored during resume by some BIOSes, >> is this really necessary? > > ba3820ade317ee36e496b9b40d2ec3987dd4aef0 claims so. But that commit > managed to put too many things into the same thing unfortunately. Is that the right SHA? Because that just reverts my combination mode removal patch. Assuming it is the right commit, then I think it's incorrect in saying that it caused backlight dimming problems after resume. That particular problem was caused by a bogus shift. The problems caused by removing the mode was a lower max brightness and/or less brightness levels. By the way, saving LBPC only makes sense if it's done before it was set to 0 to disable the panel. I don't know if the current code does the right thing, I haven't looked at it for a while. Greetings, Indan
On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote: > On Tue, August 28, 2012 16:14, Daniel Vetter wrote: > > On Tue, Aug 28, 2012 at 03:56:31PM +0200, Indan Zupancic wrote: > >> Hello, > >> > >> On Tue, August 28, 2012 08:53, Jani Nikula wrote: > >> > From: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > >> > This is a prep patch to stop drm/i915 from changing the LBPC registers > >> > itself - but we still need to properly save/restore it on > >> > suspend/resume. > >> > > >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > --- > [...] > >> It seems weird that LBPC wouldn't be restored during resume by some BIOSes, > >> is this really necessary? > > > > ba3820ade317ee36e496b9b40d2ec3987dd4aef0 claims so. But that commit > > managed to put too many things into the same thing unfortunately. > > Is that the right SHA? Because that just reverts my combination mode > removal patch. Assuming it is the right commit, then I think it's > incorrect in saying that it caused backlight dimming problems after > resume. That particular problem was caused by a bogus shift. The > problems caused by removing the mode was a lower max brightness > and/or less brightness levels. Yeah, right commit but imo with sub-par commit message. Your other mail clarified things, thanks. > By the way, saving LBPC only makes sense if it's done before it was > set to 0 to disable the panel. I don't know if the current code does > the right thing, I haven't looked at it for a while. I think we can coax it into doing the right thing, see my other mail. If your completely sure that lbpc /should/ be handled by the bios across s/r I think we can drop this. But tbh I have no idea how this really is supposed to work, and unfortunately we're not allowed to cross-check with the windows driver codebase :( Thanks, Daniel
On Tue, August 28, 2012 17:15, Daniel Vetter wrote: > On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote: >> By the way, saving LBPC only makes sense if it's done before it was >> set to 0 to disable the panel. I don't know if the current code does >> the right thing, I haven't looked at it for a while. > > I think we can coax it into doing the right thing, see my other mail. If > your completely sure that lbpc /should/ be handled by the bios across s/r > I think we can drop this. But tbh I have no idea how this really is > supposed to work, and unfortunately we're not allowed to cross-check with > the windows driver codebase :( Of course I'm not completely sure. But I agree with Chris' reasoning, why else have two ways of controlling the backlight? To me LBPC gives the impression of being a system specific thing the OS shouldn't need to worry about. Partly because it wasn't documented in the old docs at all, and its address isn't mentioned in the 965 vol3 manual, but also because it's from around the time that the BIOS used to do a lot more directly (APM versus ACPI etc.) It also gives a way to lower the max output voltage (via PWM) to the backlight hardware. These kind of problems are caused by us not having exactly the same info as the BIOS/machine makers got. Because that's what guided them into making the stuff as it is, not how it's supposed to work. The code could check if LBPC has a reasonable value after resume: Do nothing if it has and restore the last known good value if it doesn't. Writing 0 to LBPC before suspend seems unnecessary because the hardware is going into sleep mode anyway. So the PWM signal should become 0 anyway, or if it doesn't, the BIOS has a chance to disable the backlight. But I'm not sure if that's really true. So to get back to this patch, saving the LBPC value only makes sense if it's done before it's set to 0. I think saving it at i915_save_display() time is too late if the panel got disabled. Then the code is always saving and restoring zeros. BTW, inverted sense of the PWM register could be related to the polarity of the PWM signal. That should be controlled with bit 28 in BLC_PWM_CTL2. Greetings, Indan
On Thu, Aug 30, 2012 at 10:32:48AM +0200, Indan Zupancic wrote: > On Tue, August 28, 2012 17:15, Daniel Vetter wrote: > > On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote: > >> By the way, saving LBPC only makes sense if it's done before it was > >> set to 0 to disable the panel. I don't know if the current code does > >> the right thing, I haven't looked at it for a while. > > > > I think we can coax it into doing the right thing, see my other mail. If > > your completely sure that lbpc /should/ be handled by the bios across s/r > > I think we can drop this. But tbh I have no idea how this really is > > supposed to work, and unfortunately we're not allowed to cross-check with > > the windows driver codebase :( > > Of course I'm not completely sure. But I agree with Chris' reasoning, > why else have two ways of controlling the backlight? To me LBPC gives > the impression of being a system specific thing the OS shouldn't need > to worry about. Partly because it wasn't documented in the old docs at > all, and its address isn't mentioned in the 965 vol3 manual, but also > because it's from around the time that the BIOS used to do a lot more > directly (APM versus ACPI etc.) It also gives a way to lower the max > output voltage (via PWM) to the backlight hardware. > > These kind of problems are caused by us not having exactly the same > info as the BIOS/machine makers got. Because that's what guided them > into making the stuff as it is, not how it's supposed to work. > > The code could check if LBPC has a reasonable value after resume: Do > nothing if it has and restore the last known good value if it doesn't. > > Writing 0 to LBPC before suspend seems unnecessary because the hardware > is going into sleep mode anyway. So the PWM signal should become 0 anyway, > or if it doesn't, the BIOS has a chance to disable the backlight. But I'm > not sure if that's really true. > > So to get back to this patch, saving the LBPC value only makes sense if > it's done before it's set to 0. I think saving it at i915_save_display() > time is too late if the panel got disabled. Then the code is always saving > and restoring zeros. Well, if we use that backlight enable bit for panel on/off, we should no longer clear neither lbpc nor the backlight registers. Hence I think this might just work, without us saving lbpc. But we can easily add that later on I think (if it's required on some kind of crazy machine). > BTW, inverted sense of the PWM register could be related to the polarity > of the PWM signal. That should be controlled with bit 28 in BLC_PWM_CTL2. Well, I've tried that and it didn't work. I suspect that this entire inverted backlight quirk mess we currently have is just an ugly hack to prevent our code from setting the backlight to 0 and doesn't actually work as advertised. Imo it was bad judgment to merge it ... Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 58b43db..af1701c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -606,6 +606,7 @@ typedef struct drm_i915_private { u32 savePP_CONTROL; u32 savePP_DIVISOR; u32 savePFIT_CONTROL; + u8 saveLBPC; u32 save_palette_a[256]; u32 save_palette_b[256]; u32 saveDPFC_CB_BASE; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d0b60f2..3303c18 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1889,6 +1889,9 @@ #define PFIT_AUTO_RATIOS 0x61238 +/* legacy/combination backlight modes in pci config space. */ +#define PCI_LBPC 0xf4 + /* Backlight control */ #define BLC_PWM_CTL2 0x61250 /* 965+ only */ #define BLM_PWM_ENABLE (1 << 31) diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 4776ccf..05daff7 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -639,6 +639,10 @@ static void i915_save_display(struct drm_device *dev) dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2); if (IS_MOBILE(dev) && !IS_I830(dev)) dev_priv->saveLVDS = I915_READ(LVDS); + + if (IS_GEN2(dev) || IS_GEN4(dev)) + pci_read_config_byte(dev->pdev, PCI_LBPC, + &dev_priv->saveLBPC); } if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev)) @@ -759,6 +763,10 @@ static void i915_restore_display(struct drm_device *dev) I915_WRITE(PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS); I915_WRITE(PP_DIVISOR, dev_priv->savePP_DIVISOR); I915_WRITE(PP_CONTROL, dev_priv->savePP_CONTROL); + + if (IS_GEN2(dev) || IS_GEN4(dev)) + pci_write_config_byte(dev->pdev, PCI_LBPC, + dev_priv->saveLBPC); } /* Display Port state */ diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 3df4f5f..9546f97 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -33,8 +33,6 @@ #include <linux/moduleparam.h> #include "intel_drv.h" -#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ - void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode)