| Message ID | 1363946021-26651-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Fri, 22 Mar 2013 10:53:41 +0100, Daniel Vetter wrote: > > This reverts the following commits: > > cf0a6584aa6d382f802 drm/i915: write backlight harder > 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid > > We've come full-circle in this mess and now broke the originally fixed > machines again with the new trick. So remove it all and start over. > > References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Well, although I fully agree that it's better to clear all voodoo once and fix from scratch, my concern is that it'll cause mess for stable kernel. This path won't fix anything by itself, so it's not suitable for stable alone. If any, it should be together with the real fix patch. I guess you'll submit the proper fix patch(es) to stable later on? thanks, Takashi > --- > drivers/gpu/drm/i915/intel_panel.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index a3730e0..f31064a 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -336,7 +336,7 @@ void intel_panel_enable_backlight(struct drm_device *dev, > * we don't track the backlight dpms state, hence check whether > * we have to do anything first. */ > if (tmp & BLM_PWM_ENABLE) > - goto set_level; > + return; > > if (dev_priv->num_pipe == 3) > tmp &= ~BLM_PIPE_SELECT_IVB; > @@ -357,14 +357,6 @@ void intel_panel_enable_backlight(struct drm_device *dev, > I915_WRITE(BLC_PWM_PCH_CTL1, tmp); > } > } > - > -set_level: > - /* Check the current backlight level and try to set again if it's zero. > - * On some machines, BLC_PWM_CPU_CTL is cleared to zero automatically > - * when BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1 are written. > - */ > - if (!intel_panel_get_backlight(dev)) > - intel_panel_actually_set_backlight(dev, dev_priv->backlight_level); > } > > static void intel_panel_init_backlight(struct drm_device *dev) > -- > 1.7.10.4 >
On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Fri, 22 Mar 2013 10:53:41 +0100, > Daniel Vetter wrote: >> >> This reverts the following commits: >> >> cf0a6584aa6d382f802 drm/i915: write backlight harder >> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid >> >> We've come full-circle in this mess and now broke the originally fixed >> machines again with the new trick. So remove it all and start over. >> >> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Takashi Iwai <tiwai@suse.de> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Well, although I fully agree that it's better to clear all voodoo once > and fix from scratch, my concern is that it'll cause mess for stable > kernel. This path won't fix anything by itself, so it's not suitable > for stable alone. If any, it should be together with the real fix > patch. > > I guess you'll submit the proper fix patch(es) to stable later on? Oops, I've missed that 770c12312ad617172 itself is a regression fix, but lacks the relevant commit citations. I'll resend with more aggressive reverting. -Daniel
On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote: >> At Fri, 22 Mar 2013 10:53:41 +0100, >> Daniel Vetter wrote: >>> >>> This reverts the following commits: >>> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid >>> >>> We've come full-circle in this mess and now broke the originally fixed >>> machines again with the new trick. So remove it all and start over. >>> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Takashi Iwai <tiwai@suse.de> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> Well, although I fully agree that it's better to clear all voodoo once >> and fix from scratch, my concern is that it'll cause mess for stable >> kernel. This path won't fix anything by itself, so it's not suitable >> for stable alone. If any, it should be together with the real fix >> patch. >> >> I guess you'll submit the proper fix patch(es) to stable later on? > > Oops, I've missed that 770c12312ad617172 itself is a regression fix, > but lacks the relevant commit citations. I'll resend with more > aggressive reverting. Actually the only other patch I've found is 6db65cbb941f, but no one reported a regression against that one yet. So I think we can leave that in for now. So I still think that this is the patch I want for 3.9&stable, since oldest regression wins. -Daniel
At Fri, 22 Mar 2013 11:22:57 +0100, Daniel Vetter wrote: > > On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote: > >> At Fri, 22 Mar 2013 10:53:41 +0100, > >> Daniel Vetter wrote: > >>> > >>> This reverts the following commits: > >>> > >>> cf0a6584aa6d382f802 drm/i915: write backlight harder > >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid > >>> > >>> We've come full-circle in this mess and now broke the originally fixed > >>> machines again with the new trick. So remove it all and start over. > >>> > >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html > >>> Cc: Jani Nikula <jani.nikula@intel.com> > >>> Cc: Takashi Iwai <tiwai@suse.de> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > >> Well, although I fully agree that it's better to clear all voodoo once > >> and fix from scratch, my concern is that it'll cause mess for stable > >> kernel. This path won't fix anything by itself, so it's not suitable > >> for stable alone. If any, it should be together with the real fix > >> patch. > >> > >> I guess you'll submit the proper fix patch(es) to stable later on? > > > > Oops, I've missed that 770c12312ad617172 itself is a regression fix, > > but lacks the relevant commit citations. I'll resend with more > > aggressive reverting. > > Actually the only other patch I've found is 6db65cbb941f, but no one > reported a regression against that one yet. So I think we can leave > that in for now. So I still think that this is the patch I want for > 3.9&stable, since oldest regression wins. Yeah, the situation is messy. Let me write down as far as I remember: - 770c12312ad617172 itself fixed the backlight at resume originally with 3.6 kernel. The backlight worked at boot time, IIRC. It had no mention of regressing commit since it was a series of changes, IIRC. It fixed the same problem for a few Lenovo and HP laptops. However, this turned out to break a backlight on a Dell machine. - cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went in 3.9 and propagated to stable. But it hadn't been tested on Lenovo nor HP machines. Now this broke HP machines (not sure about Lenovo) again. Interestingly, at this time, it broke the backlight at boot time. Maybe the recent changes of initialization sequence at boot made difference? - 6db65cbb941f is slightly different fix from other commits. AFAIK, this was found only on SandyBridge eDP (HP Z-something model), and this happens only at resume, again. Unfortunately I have no longer access to this machine, so I can't check the current status. Now, you revert both first two, that is, the backlight handling is like 3.6-rc1. A Dell laptop should still work, but HP/Lenovo are likely broken yet. That's why I wonder whether it's suitable for stable. It cleans up the code but doesn't fix actual bugs. Apart from that, a big question is why the commit 770c12312ad617172 broke the Dell laptop. It just writes BLC_PWM_CPU_CTL again. If this really breaks, won't further changing the backlight (or even setting the same level) break again even with the "fix" patch? Since intel_panel_set_backlight() just updates BLC_PWM_CPU_CTL, the effect should be identical... Takashi
On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Fri, 22 Mar 2013 11:22:57 +0100, > Daniel Vetter wrote: >> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote: >> >> At Fri, 22 Mar 2013 10:53:41 +0100, >> >> Daniel Vetter wrote: >> >>> >> >>> This reverts the following commits: >> >>> >> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder >> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid >> >>> >> >>> We've come full-circle in this mess and now broke the originally fixed >> >>> machines again with the new trick. So remove it all and start over. >> >>> >> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html >> >>> Cc: Jani Nikula <jani.nikula@intel.com> >> >>> Cc: Takashi Iwai <tiwai@suse.de> >> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> >> >> Well, although I fully agree that it's better to clear all voodoo once >> >> and fix from scratch, my concern is that it'll cause mess for stable >> >> kernel. This path won't fix anything by itself, so it's not suitable >> >> for stable alone. If any, it should be together with the real fix >> >> patch. >> >> >> >> I guess you'll submit the proper fix patch(es) to stable later on? >> > >> > Oops, I've missed that 770c12312ad617172 itself is a regression fix, >> > but lacks the relevant commit citations. I'll resend with more >> > aggressive reverting. >> >> Actually the only other patch I've found is 6db65cbb941f, but no one >> reported a regression against that one yet. So I think we can leave >> that in for now. So I still think that this is the patch I want for >> 3.9&stable, since oldest regression wins. > > Yeah, the situation is messy. Let me write down as far as I remember: > > - 770c12312ad617172 itself fixed the backlight at resume originally > with 3.6 kernel. The backlight worked at boot time, IIRC. It had > no mention of regressing commit since it was a series of changes, > IIRC. It fixed the same problem for a few Lenovo and HP laptops. I think the important question is whether those Lenovo/HPs ever worked? If 770c is a regression fix, too, we'll need to revert more I think. > However, this turned out to break a backlight on a Dell machine. > > - cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went > in 3.9 and propagated to stable. But it hadn't been tested on > Lenovo nor HP machines. > > Now this broke HP machines (not sure about Lenovo) again. > Interestingly, at this time, it broke the backlight at boot time. > Maybe the recent changes of initialization sequence at boot made > difference? > > - 6db65cbb941f is slightly different fix from other commits. > AFAIK, this was found only on SandyBridge eDP (HP Z-something > model), and this happens only at resume, again. > > Unfortunately I have no longer access to this machine, so I can't > check the current status. Iirc we've used 6db6 as inspiration/justification for the above commits. > Now, you revert both first two, that is, the backlight handling is > like 3.6-rc1. A Dell laptop should still work, but HP/Lenovo are > likely broken yet. That's why I wonder whether it's suitable for > stable. It cleans up the code but doesn't fix actual bugs. > > Apart from that, a big question is why the commit 770c12312ad617172 > broke the Dell laptop. It just writes BLC_PWM_CPU_CTL again. If this > really breaks, won't further changing the backlight (or even setting > the same level) break again even with the "fix" patch? > Since intel_panel_set_backlight() just updates BLC_PWM_CPU_CTL, the > effect should be identical... One of the reasons why I want to go back to the beginning and rip out all the hacks is that I've noticed that we completely lack locking in the backlight code. And we have at least 3 concurrent useres: - modeset code - sysfs backlight interface - asle interrupt handler Especially the last one is known to fire when touching some of the blc/panel registers through some smm magic. So imo before we add more hacks again, we should fix up the locking (and generally make the code a bit less messy, Jani has started with some patches), and then check which bugs are left. Yeah, this will suck a bit for distros :( Yours, Daniel
At Fri, 22 Mar 2013 12:22:42 +0100, Daniel Vetter wrote: > > On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote: > > At Fri, 22 Mar 2013 11:22:57 +0100, > > Daniel Vetter wrote: > >> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > >> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote: > >> >> At Fri, 22 Mar 2013 10:53:41 +0100, > >> >> Daniel Vetter wrote: > >> >>> > >> >>> This reverts the following commits: > >> >>> > >> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder > >> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid > >> >>> > >> >>> We've come full-circle in this mess and now broke the originally fixed > >> >>> machines again with the new trick. So remove it all and start over. > >> >>> > >> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html > >> >>> Cc: Jani Nikula <jani.nikula@intel.com> > >> >>> Cc: Takashi Iwai <tiwai@suse.de> > >> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> >> > >> >> Well, although I fully agree that it's better to clear all voodoo once > >> >> and fix from scratch, my concern is that it'll cause mess for stable > >> >> kernel. This path won't fix anything by itself, so it's not suitable > >> >> for stable alone. If any, it should be together with the real fix > >> >> patch. > >> >> > >> >> I guess you'll submit the proper fix patch(es) to stable later on? > >> > > >> > Oops, I've missed that 770c12312ad617172 itself is a regression fix, > >> > but lacks the relevant commit citations. I'll resend with more > >> > aggressive reverting. > >> > >> Actually the only other patch I've found is 6db65cbb941f, but no one > >> reported a regression against that one yet. So I think we can leave > >> that in for now. So I still think that this is the patch I want for > >> 3.9&stable, since oldest regression wins. > > > > Yeah, the situation is messy. Let me write down as far as I remember: > > > > - 770c12312ad617172 itself fixed the backlight at resume originally > > with 3.6 kernel. The backlight worked at boot time, IIRC. It had > > no mention of regressing commit since it was a series of changes, > > IIRC. It fixed the same problem for a few Lenovo and HP laptops. > > I think the important question is whether those Lenovo/HPs ever > worked? IIRC, the boot screen worked on 3.6-rc but the resume was broken -- at least on HP machines. On Lenovo, it might be different. Sorry, I don't remember. Need to dig into LKML archive. > If 770c is a regression fix, too, we'll need to revert more I > think. Yes, but a revert wasn't trivial at that time because of large changes in i915 code in 3.6-rc1. Maybe we can try it again... > > However, this turned out to break a backlight on a Dell machine. > > > > - cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went > > in 3.9 and propagated to stable. But it hadn't been tested on > > Lenovo nor HP machines. > > > > Now this broke HP machines (not sure about Lenovo) again. > > Interestingly, at this time, it broke the backlight at boot time. > > Maybe the recent changes of initialization sequence at boot made > > difference? > > > > - 6db65cbb941f is slightly different fix from other commits. > > AFAIK, this was found only on SandyBridge eDP (HP Z-something > > model), and this happens only at resume, again. > > > > Unfortunately I have no longer access to this machine, so I can't > > check the current status. > > Iirc we've used 6db6 as inspiration/justification for the above commits. Well, not from the beginning; it was reported on different machines as an individual issue. But the end fix looks similar, so very likely related. Another point is that the fixes are targeted to different ranges: The fix 770c was for a variety of machines and all were LVDS. (But now we see it's affecting to HSW and IVY eDP, too.) The fix 6db6 is for SNB eDP only. We didn't see the issue on LVDS at that time. > > Now, you revert both first two, that is, the backlight handling is > > like 3.6-rc1. A Dell laptop should still work, but HP/Lenovo are > > likely broken yet. That's why I wonder whether it's suitable for > > stable. It cleans up the code but doesn't fix actual bugs. > > > > Apart from that, a big question is why the commit 770c12312ad617172 > > broke the Dell laptop. It just writes BLC_PWM_CPU_CTL again. If this > > really breaks, won't further changing the backlight (or even setting > > the same level) break again even with the "fix" patch? > > Since intel_panel_set_backlight() just updates BLC_PWM_CPU_CTL, the > > effect should be identical... > > One of the reasons why I want to go back to the beginning and rip out > all the hacks is that I've noticed that we completely lack locking in > the backlight code. And we have at least 3 concurrent useres: > - modeset code > - sysfs backlight interface > - asle interrupt handler Yeah, that looks fragile. > Especially the last one is known to fire when touching some of the > blc/panel registers through some smm magic. So imo before we add more > hacks again, we should fix up the locking (and generally make the code > a bit less messy, Jani has started with some patches), and then check > which bugs are left. > > Yeah, this will suck a bit for distros :( Hehe, that's hard life :p Takashi
On Fri, Mar 22, 2013 at 12:34:41PM +0100, Takashi Iwai wrote: > At Fri, 22 Mar 2013 12:22:42 +0100, > Daniel Vetter wrote: > > > > On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote: > > > At Fri, 22 Mar 2013 11:22:57 +0100, > > > Daniel Vetter wrote: > > >> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > >> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote: > > >> >> At Fri, 22 Mar 2013 10:53:41 +0100, > > >> >> Daniel Vetter wrote: > > >> >>> > > >> >>> This reverts the following commits: > > >> >>> > > >> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder > > >> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid > > >> >>> > > >> >>> We've come full-circle in this mess and now broke the originally fixed > > >> >>> machines again with the new trick. So remove it all and start over. > > >> >>> > > >> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html > > >> >>> Cc: Jani Nikula <jani.nikula@intel.com> > > >> >>> Cc: Takashi Iwai <tiwai@suse.de> > > >> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > >> >> > > >> >> Well, although I fully agree that it's better to clear all voodoo once > > >> >> and fix from scratch, my concern is that it'll cause mess for stable > > >> >> kernel. This path won't fix anything by itself, so it's not suitable > > >> >> for stable alone. If any, it should be together with the real fix > > >> >> patch. > > >> >> > > >> >> I guess you'll submit the proper fix patch(es) to stable later on? > > >> > > > >> > Oops, I've missed that 770c12312ad617172 itself is a regression fix, > > >> > but lacks the relevant commit citations. I'll resend with more > > >> > aggressive reverting. > > >> > > >> Actually the only other patch I've found is 6db65cbb941f, but no one > > >> reported a regression against that one yet. So I think we can leave > > >> that in for now. So I still think that this is the patch I want for > > >> 3.9&stable, since oldest regression wins. > > > > > > Yeah, the situation is messy. Let me write down as far as I remember: > > > > > > - 770c12312ad617172 itself fixed the backlight at resume originally > > > with 3.6 kernel. The backlight worked at boot time, IIRC. It had > > > no mention of regressing commit since it was a series of changes, > > > IIRC. It fixed the same problem for a few Lenovo and HP laptops. > > > > I think the important question is whether those Lenovo/HPs ever > > worked? > > IIRC, the boot screen worked on 3.6-rc but the resume was broken -- at > least on HP machines. On Lenovo, it might be different. Sorry, I > don't remember. Need to dig into LKML archive. Hm, right. And iirc (my google fu is lacking atm) we never figured out what's really broken, only noticed that your patch fixes it. So I think I should only revert cf0a6584aa6d382f802 drm/i915: write backlight harder. Which leaves us with another regression, but afaics there's no way to win this game here right now. I really hope that cleaning up the code and introducing proper locking helps a bit to win this game of whack-a-mole. I'll send the new revert asap. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index a3730e0..f31064a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -336,7 +336,7 @@ void intel_panel_enable_backlight(struct drm_device *dev, * we don't track the backlight dpms state, hence check whether * we have to do anything first. */ if (tmp & BLM_PWM_ENABLE) - goto set_level; + return; if (dev_priv->num_pipe == 3) tmp &= ~BLM_PIPE_SELECT_IVB; @@ -357,14 +357,6 @@ void intel_panel_enable_backlight(struct drm_device *dev, I915_WRITE(BLC_PWM_PCH_CTL1, tmp); } } - -set_level: - /* Check the current backlight level and try to set again if it's zero. - * On some machines, BLC_PWM_CPU_CTL is cleared to zero automatically - * when BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1 are written. - */ - if (!intel_panel_get_backlight(dev)) - intel_panel_actually_set_backlight(dev, dev_priv->backlight_level); } static void intel_panel_init_backlight(struct drm_device *dev)
This reverts the following commits: cf0a6584aa6d382f802 drm/i915: write backlight harder 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid We've come full-circle in this mess and now broke the originally fixed machines again with the new trick. So remove it all and start over. References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html Cc: Jani Nikula <jani.nikula@intel.com> Cc: Takashi Iwai <tiwai@suse.de> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_panel.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)