* [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
@ 2012-06-20 7:17 Takashi Iwai
2012-06-20 8:05 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2012-06-20 7:17 UTC (permalink / raw)
To: intel-gfx; +Cc: John Sundragon Waitz, Rodrigo Vivi
This patch fixes the problem on some HP desktop machines with eDP
which give blank screens after S3 resume.
The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
register is already restored at the beginning of resume, it doesn't
seem to take effect. Simply re-issuing the register write restores
the backlight gracefully.
Tested with 3.5-rc3 kernel.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/gpu/drm/i915/i915_drv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9fe9ebe..2ba1350 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -658,6 +658,9 @@ static int i915_drm_thaw(struct drm_device *dev)
intel_opregion_init(dev);
+ if (dev_priv->backlight)
+ backlight_update_status(dev_priv->backlight);
+
dev_priv->modeset_on_lid = 0;
console_lock();
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-20 7:17 [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops Takashi Iwai
@ 2012-06-20 8:05 ` Daniel Vetter
2012-06-20 9:21 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2012-06-20 8:05 UTC (permalink / raw)
To: Takashi Iwai; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> This patch fixes the problem on some HP desktop machines with eDP
> which give blank screens after S3 resume.
>
> The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> register is already restored at the beginning of resume, it doesn't
> seem to take effect. Simply re-issuing the register write restores
> the backlight gracefully.
>
> Tested with 3.5-rc3 kernel.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
This patch looks very fishy as-is, simply because I don't like adding
random calls to random functions at pretty much random places without any
hint why it works. Just a few weeks ago we've had tons of fun with writes
that don't stick when the ring isn't yet set up. Can you please check with
a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
I.e. whether the write doesn't stick or whether it gets reset somewhere
between the register restore functions and the new place.
I suspect the opregion_init call could allow the bios to frob with these
registers. If that's the case, the patch is missing a comment that to that
effect.
Essentially I want to know why this place here works and why not move the
call a few lines up or down.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9fe9ebe..2ba1350 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -658,6 +658,9 @@ static int i915_drm_thaw(struct drm_device *dev)
>
> intel_opregion_init(dev);
>
> + if (dev_priv->backlight)
> + backlight_update_status(dev_priv->backlight);
> +
> dev_priv->modeset_on_lid = 0;
>
> console_lock();
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-20 8:05 ` Daniel Vetter
@ 2012-06-20 9:21 ` Takashi Iwai
2012-06-20 9:36 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2012-06-20 9:21 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
At Wed, 20 Jun 2012 10:05:12 +0200,
Daniel Vetter wrote:
>
> On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > This patch fixes the problem on some HP desktop machines with eDP
> > which give blank screens after S3 resume.
> >
> > The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> > register is already restored at the beginning of resume, it doesn't
> > seem to take effect. Simply re-issuing the register write restores
> > the backlight gracefully.
> >
> > Tested with 3.5-rc3 kernel.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
>
> This patch looks very fishy as-is, simply because I don't like adding
> random calls to random functions at pretty much random places without any
> hint why it works.
It's difficult to know exactly why it works. We can only guess, but
nothing more than that, just like between husband and wife :)
> Just a few weeks ago we've had tons of fun with writes
> that don't stick when the ring isn't yet set up. Can you please check with
> a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> I.e. whether the write doesn't stick or whether it gets reset somewhere
> between the register restore functions and the new place.
The register value is already set, and not touched after that.
I've already checked.
> I suspect the opregion_init call could allow the bios to frob with these
> registers. If that's the case, the patch is missing a comment that to that
> effect.
Right. That's also my suspect.
> Essentially I want to know why this place here works and why not move the
> call a few lines up or down.
A few lines down works definitely. Even just writing to sysfs works.
A few lines up, well, I need to check where is the border line.
Takashi
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 9fe9ebe..2ba1350 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -658,6 +658,9 @@ static int i915_drm_thaw(struct drm_device *dev)
> >
> > intel_opregion_init(dev);
> >
> > + if (dev_priv->backlight)
> > + backlight_update_status(dev_priv->backlight);
> > +
> > dev_priv->modeset_on_lid = 0;
> >
> > console_lock();
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-20 9:21 ` Takashi Iwai
@ 2012-06-20 9:36 ` Daniel Vetter
2012-06-20 9:39 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2012-06-20 9:36 UTC (permalink / raw)
To: Takashi Iwai; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
On Wed, Jun 20, 2012 at 11:21:11AM +0200, Takashi Iwai wrote:
> At Wed, 20 Jun 2012 10:05:12 +0200,
> Daniel Vetter wrote:
> >
> > On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > > This patch fixes the problem on some HP desktop machines with eDP
> > > which give blank screens after S3 resume.
> > >
> > > The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> > > register is already restored at the beginning of resume, it doesn't
> > > seem to take effect. Simply re-issuing the register write restores
> > > the backlight gracefully.
> > >
> > > Tested with 3.5-rc3 kernel.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> > >
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >
> > This patch looks very fishy as-is, simply because I don't like adding
> > random calls to random functions at pretty much random places without any
> > hint why it works.
>
> It's difficult to know exactly why it works. We can only guess, but
> nothing more than that, just like between husband and wife :)
Yeah I know, but I've been burnt a bit recently with too much
cargo-culting. Hence why I'm so dense about this stuff ;-)
> > Just a few weeks ago we've had tons of fun with writes
> > that don't stick when the ring isn't yet set up. Can you please check with
> > a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> > I.e. whether the write doesn't stick or whether it gets reset somewhere
> > between the register restore functions and the new place.
>
> The register value is already set, and not touched after that.
> I've already checked.
So immediately before the backlight_update_status call we have the
_correct_ value in BLC_PWM_CPU_CTL? That would be indeed puzzling ...
> > I suspect the opregion_init call could allow the bios to frob with these
> > registers. If that's the case, the patch is missing a comment that to that
> > effect.
>
> Right. That's also my suspect.
>
> > Essentially I want to know why this place here works and why not move the
> > call a few lines up or down.
>
> A few lines down works definitely. Even just writing to sysfs works.
> A few lines up, well, I need to check where is the border line.
Thanks, that would be interesting. Especially if it's opregion that we
need to have set up before restoring the backlight properly.
-Daniel
>
>
> Takashi
>
> > -Daniel
> >
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 9fe9ebe..2ba1350 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -658,6 +658,9 @@ static int i915_drm_thaw(struct drm_device *dev)
> > >
> > > intel_opregion_init(dev);
> > >
> > > + if (dev_priv->backlight)
> > > + backlight_update_status(dev_priv->backlight);
> > > +
> > > dev_priv->modeset_on_lid = 0;
> > >
> > > console_lock();
> > > --
> > > 1.7.10.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Mail: daniel@ffwll.ch
> > Mobile: +41 (0)79 365 57 48
> >
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-20 9:36 ` Daniel Vetter
@ 2012-06-20 9:39 ` Takashi Iwai
2012-06-21 12:11 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2012-06-20 9:39 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
At Wed, 20 Jun 2012 11:36:51 +0200,
Daniel Vetter wrote:
>
> On Wed, Jun 20, 2012 at 11:21:11AM +0200, Takashi Iwai wrote:
> > At Wed, 20 Jun 2012 10:05:12 +0200,
> > Daniel Vetter wrote:
> > >
> > > On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > > > This patch fixes the problem on some HP desktop machines with eDP
> > > > which give blank screens after S3 resume.
> > > >
> > > > The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> > > > register is already restored at the beginning of resume, it doesn't
> > > > seem to take effect. Simply re-issuing the register write restores
> > > > the backlight gracefully.
> > > >
> > > > Tested with 3.5-rc3 kernel.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> > > >
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > >
> > > This patch looks very fishy as-is, simply because I don't like adding
> > > random calls to random functions at pretty much random places without any
> > > hint why it works.
> >
> > It's difficult to know exactly why it works. We can only guess, but
> > nothing more than that, just like between husband and wife :)
>
> Yeah I know, but I've been burnt a bit recently with too much
> cargo-culting. Hence why I'm so dense about this stuff ;-)
>
> > > Just a few weeks ago we've had tons of fun with writes
> > > that don't stick when the ring isn't yet set up. Can you please check with
> > > a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> > > I.e. whether the write doesn't stick or whether it gets reset somewhere
> > > between the register restore functions and the new place.
> >
> > The register value is already set, and not touched after that.
> > I've already checked.
>
> So immediately before the backlight_update_status call we have the
> _correct_ value in BLC_PWM_CPU_CTL? That would be indeed puzzling ...
Yes.
> > > I suspect the opregion_init call could allow the bios to frob with these
> > > registers. If that's the case, the patch is missing a comment that to that
> > > effect.
> >
> > Right. That's also my suspect.
> >
> > > Essentially I want to know why this place here works and why not move the
> > > call a few lines up or down.
> >
> > A few lines down works definitely. Even just writing to sysfs works.
> > A few lines up, well, I need to check where is the border line.
>
> Thanks, that would be interesting. Especially if it's opregion that we
> need to have set up before restoring the backlight properly.
This is getting really puzzling. It's not about the opregion.
Just some order of the register restoration.
Essentially the patch below fixes the problem. But I have absolutely
no idea why.
Takashi
---
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 0ede02a..0f89dd0 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -740,8 +740,8 @@ static void i915_restore_display(struct drm_device *dev)
if (HAS_PCH_SPLIT(dev)) {
I915_WRITE(BLC_PWM_PCH_CTL1, dev_priv->saveBLC_PWM_CTL);
I915_WRITE(BLC_PWM_PCH_CTL2, dev_priv->saveBLC_PWM_CTL2);
- I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
I915_WRITE(BLC_PWM_CPU_CTL2, dev_priv->saveBLC_CPU_PWM_CTL2);
+ I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->savePP_ON_DELAYS);
I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
I915_WRITE(PCH_PP_DIVISOR, dev_priv->savePP_DIVISOR);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-20 9:39 ` Takashi Iwai
@ 2012-06-21 12:11 ` Takashi Iwai
2012-06-21 12:33 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2012-06-21 12:11 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
At Wed, 20 Jun 2012 11:39:51 +0200,
Takashi Iwai wrote:
>
> At Wed, 20 Jun 2012 11:36:51 +0200,
> Daniel Vetter wrote:
> >
> > On Wed, Jun 20, 2012 at 11:21:11AM +0200, Takashi Iwai wrote:
> > > At Wed, 20 Jun 2012 10:05:12 +0200,
> > > Daniel Vetter wrote:
> > > >
> > > > On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > > > > This patch fixes the problem on some HP desktop machines with eDP
> > > > > which give blank screens after S3 resume.
> > > > >
> > > > > The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> > > > > register is already restored at the beginning of resume, it doesn't
> > > > > seem to take effect. Simply re-issuing the register write restores
> > > > > the backlight gracefully.
> > > > >
> > > > > Tested with 3.5-rc3 kernel.
> > > > >
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> > > > >
> > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > >
> > > > This patch looks very fishy as-is, simply because I don't like adding
> > > > random calls to random functions at pretty much random places without any
> > > > hint why it works.
> > >
> > > It's difficult to know exactly why it works. We can only guess, but
> > > nothing more than that, just like between husband and wife :)
> >
> > Yeah I know, but I've been burnt a bit recently with too much
> > cargo-culting. Hence why I'm so dense about this stuff ;-)
> >
> > > > Just a few weeks ago we've had tons of fun with writes
> > > > that don't stick when the ring isn't yet set up. Can you please check with
> > > > a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> > > > I.e. whether the write doesn't stick or whether it gets reset somewhere
> > > > between the register restore functions and the new place.
> > >
> > > The register value is already set, and not touched after that.
> > > I've already checked.
> >
> > So immediately before the backlight_update_status call we have the
> > _correct_ value in BLC_PWM_CPU_CTL? That would be indeed puzzling ...
>
> Yes.
>
> > > > I suspect the opregion_init call could allow the bios to frob with these
> > > > registers. If that's the case, the patch is missing a comment that to that
> > > > effect.
> > >
> > > Right. That's also my suspect.
> > >
> > > > Essentially I want to know why this place here works and why not move the
> > > > call a few lines up or down.
> > >
> > > A few lines down works definitely. Even just writing to sysfs works.
> > > A few lines up, well, I need to check where is the border line.
> >
> > Thanks, that would be interesting. Especially if it's opregion that we
> > need to have set up before restoring the backlight properly.
>
> This is getting really puzzling. It's not about the opregion.
> Just some order of the register restoration.
>
> Essentially the patch below fixes the problem. But I have absolutely
> no idea why.
Daniel, do you have any clue about this?
Is it a strict order of these registers?
Takashi
>
> Takashi
>
> ---
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 0ede02a..0f89dd0 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -740,8 +740,8 @@ static void i915_restore_display(struct drm_device *dev)
> if (HAS_PCH_SPLIT(dev)) {
> I915_WRITE(BLC_PWM_PCH_CTL1, dev_priv->saveBLC_PWM_CTL);
> I915_WRITE(BLC_PWM_PCH_CTL2, dev_priv->saveBLC_PWM_CTL2);
> - I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
> I915_WRITE(BLC_PWM_CPU_CTL2, dev_priv->saveBLC_CPU_PWM_CTL2);
> + I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
> I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->savePP_ON_DELAYS);
> I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
> I915_WRITE(PCH_PP_DIVISOR, dev_priv->savePP_DIVISOR);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-21 12:11 ` Takashi Iwai
@ 2012-06-21 12:33 ` Daniel Vetter
2012-06-21 13:17 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2012-06-21 12:33 UTC (permalink / raw)
To: Takashi Iwai; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
On Thu, Jun 21, 2012 at 02:11:50PM +0200, Takashi Iwai wrote:
> At Wed, 20 Jun 2012 11:39:51 +0200,
> Takashi Iwai wrote:
> >
> > At Wed, 20 Jun 2012 11:36:51 +0200,
> > Daniel Vetter wrote:
> > >
> > > On Wed, Jun 20, 2012 at 11:21:11AM +0200, Takashi Iwai wrote:
> > > > At Wed, 20 Jun 2012 10:05:12 +0200,
> > > > Daniel Vetter wrote:
> > > > >
> > > > > On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > > > > > This patch fixes the problem on some HP desktop machines with eDP
> > > > > > which give blank screens after S3 resume.
> > > > > >
> > > > > > The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> > > > > > register is already restored at the beginning of resume, it doesn't
> > > > > > seem to take effect. Simply re-issuing the register write restores
> > > > > > the backlight gracefully.
> > > > > >
> > > > > > Tested with 3.5-rc3 kernel.
> > > > > >
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> > > > > >
> > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > >
> > > > > This patch looks very fishy as-is, simply because I don't like adding
> > > > > random calls to random functions at pretty much random places without any
> > > > > hint why it works.
> > > >
> > > > It's difficult to know exactly why it works. We can only guess, but
> > > > nothing more than that, just like between husband and wife :)
> > >
> > > Yeah I know, but I've been burnt a bit recently with too much
> > > cargo-culting. Hence why I'm so dense about this stuff ;-)
> > >
> > > > > Just a few weeks ago we've had tons of fun with writes
> > > > > that don't stick when the ring isn't yet set up. Can you please check with
> > > > > a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> > > > > I.e. whether the write doesn't stick or whether it gets reset somewhere
> > > > > between the register restore functions and the new place.
> > > >
> > > > The register value is already set, and not touched after that.
> > > > I've already checked.
> > >
> > > So immediately before the backlight_update_status call we have the
> > > _correct_ value in BLC_PWM_CPU_CTL? That would be indeed puzzling ...
> >
> > Yes.
> >
> > > > > I suspect the opregion_init call could allow the bios to frob with these
> > > > > registers. If that's the case, the patch is missing a comment that to that
> > > > > effect.
> > > >
> > > > Right. That's also my suspect.
> > > >
> > > > > Essentially I want to know why this place here works and why not move the
> > > > > call a few lines up or down.
> > > >
> > > > A few lines down works definitely. Even just writing to sysfs works.
> > > > A few lines up, well, I need to check where is the border line.
> > >
> > > Thanks, that would be interesting. Especially if it's opregion that we
> > > need to have set up before restoring the backlight properly.
> >
> > This is getting really puzzling. It's not about the opregion.
> > Just some order of the register restoration.
> >
> > Essentially the patch below fixes the problem. But I have absolutely
> > no idea why.
>
> Daniel, do you have any clue about this?
>
> Is it a strict order of these registers?
Could very well be that the write does not get forwarded internally when
the BLC stuff isn't enabled yet. Or the other way round. E.g. the new pipe
select stuff for the blc clock I've implemented just recently only works
while the backlight is off.
I'm much happier with this diff than the older one to just hit the regs
harder, so can you please bake this into a proper patch? And please add a
1-line comment to the code that for some unknown reason we need to write
the _CTL reg after the _CTL2.
Also, I loathe this save/restore register dance. It duplicates code we
already have, but with the twist that we might accidentally get the
ordering (or some timing constraint) slightly wrong. Resulting in broken
s/r, at least on some machines. Imo we should try to wean off these in the
long term.
Thanks, Daniel
>
>
>
> Takashi
>
> >
> > Takashi
> >
> > ---
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 0ede02a..0f89dd0 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -740,8 +740,8 @@ static void i915_restore_display(struct drm_device *dev)
> > if (HAS_PCH_SPLIT(dev)) {
> > I915_WRITE(BLC_PWM_PCH_CTL1, dev_priv->saveBLC_PWM_CTL);
> > I915_WRITE(BLC_PWM_PCH_CTL2, dev_priv->saveBLC_PWM_CTL2);
> > - I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
> > I915_WRITE(BLC_PWM_CPU_CTL2, dev_priv->saveBLC_CPU_PWM_CTL2);
> > + I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
> > I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->savePP_ON_DELAYS);
> > I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
> > I915_WRITE(PCH_PP_DIVISOR, dev_priv->savePP_DIVISOR);
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops
2012-06-21 12:33 ` Daniel Vetter
@ 2012-06-21 13:17 ` Takashi Iwai
0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2012-06-21 13:17 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, John Sundragon Waitz, Rodrigo Vivi
At Thu, 21 Jun 2012 14:33:07 +0200,
Daniel Vetter wrote:
>
> On Thu, Jun 21, 2012 at 02:11:50PM +0200, Takashi Iwai wrote:
> > At Wed, 20 Jun 2012 11:39:51 +0200,
> > Takashi Iwai wrote:
> > >
> > > At Wed, 20 Jun 2012 11:36:51 +0200,
> > > Daniel Vetter wrote:
> > > >
> > > > On Wed, Jun 20, 2012 at 11:21:11AM +0200, Takashi Iwai wrote:
> > > > > At Wed, 20 Jun 2012 10:05:12 +0200,
> > > > > Daniel Vetter wrote:
> > > > > >
> > > > > > On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > > > > > > This patch fixes the problem on some HP desktop machines with eDP
> > > > > > > which give blank screens after S3 resume.
> > > > > > >
> > > > > > > The problem looks like a timing issue. Although BLC_PWM_CPU_CTL
> > > > > > > register is already restored at the beginning of resume, it doesn't
> > > > > > > seem to take effect. Simply re-issuing the register write restores
> > > > > > > the backlight gracefully.
> > > > > > >
> > > > > > > Tested with 3.5-rc3 kernel.
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> > > > > > >
> > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > >
> > > > > > This patch looks very fishy as-is, simply because I don't like adding
> > > > > > random calls to random functions at pretty much random places without any
> > > > > > hint why it works.
> > > > >
> > > > > It's difficult to know exactly why it works. We can only guess, but
> > > > > nothing more than that, just like between husband and wife :)
> > > >
> > > > Yeah I know, but I've been burnt a bit recently with too much
> > > > cargo-culting. Hence why I'm so dense about this stuff ;-)
> > > >
> > > > > > Just a few weeks ago we've had tons of fun with writes
> > > > > > that don't stick when the ring isn't yet set up. Can you please check with
> > > > > > a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> > > > > > I.e. whether the write doesn't stick or whether it gets reset somewhere
> > > > > > between the register restore functions and the new place.
> > > > >
> > > > > The register value is already set, and not touched after that.
> > > > > I've already checked.
> > > >
> > > > So immediately before the backlight_update_status call we have the
> > > > _correct_ value in BLC_PWM_CPU_CTL? That would be indeed puzzling ...
> > >
> > > Yes.
> > >
> > > > > > I suspect the opregion_init call could allow the bios to frob with these
> > > > > > registers. If that's the case, the patch is missing a comment that to that
> > > > > > effect.
> > > > >
> > > > > Right. That's also my suspect.
> > > > >
> > > > > > Essentially I want to know why this place here works and why not move the
> > > > > > call a few lines up or down.
> > > > >
> > > > > A few lines down works definitely. Even just writing to sysfs works.
> > > > > A few lines up, well, I need to check where is the border line.
> > > >
> > > > Thanks, that would be interesting. Especially if it's opregion that we
> > > > need to have set up before restoring the backlight properly.
> > >
> > > This is getting really puzzling. It's not about the opregion.
> > > Just some order of the register restoration.
> > >
> > > Essentially the patch below fixes the problem. But I have absolutely
> > > no idea why.
> >
> > Daniel, do you have any clue about this?
> >
> > Is it a strict order of these registers?
>
> Could very well be that the write does not get forwarded internally when
> the BLC stuff isn't enabled yet. Or the other way round. E.g. the new pipe
> select stuff for the blc clock I've implemented just recently only works
> while the backlight is off.
Sounds possible.
> I'm much happier with this diff than the older one to just hit the regs
> harder, so can you please bake this into a proper patch? And please add a
> 1-line comment to the code that for some unknown reason we need to write
> the _CTL reg after the _CTL2.
Sure, will be sent soon later.
> Also, I loathe this save/restore register dance. It duplicates code we
> already have, but with the twist that we might accidentally get the
> ordering (or some timing constraint) slightly wrong. Resulting in broken
> s/r, at least on some machines. Imo we should try to wean off these in the
> long term.
OTOH, these allow shorter initialization than the normal full init.
But I agree with the point of code maintenance mess by such.
thanks,
Takashi
>
> Thanks, Daniel
>
> >
> >
> >
> > Takashi
> >
> > >
> > > Takashi
> > >
> > > ---
> > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > > index 0ede02a..0f89dd0 100644
> > > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > > @@ -740,8 +740,8 @@ static void i915_restore_display(struct drm_device *dev)
> > > if (HAS_PCH_SPLIT(dev)) {
> > > I915_WRITE(BLC_PWM_PCH_CTL1, dev_priv->saveBLC_PWM_CTL);
> > > I915_WRITE(BLC_PWM_PCH_CTL2, dev_priv->saveBLC_PWM_CTL2);
> > > - I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
> > > I915_WRITE(BLC_PWM_CPU_CTL2, dev_priv->saveBLC_CPU_PWM_CTL2);
> > > + I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
> > > I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->savePP_ON_DELAYS);
> > > I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
> > > I915_WRITE(PCH_PP_DIVISOR, dev_priv->savePP_DIVISOR);
>
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-06-21 13:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-20 7:17 [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops Takashi Iwai
2012-06-20 8:05 ` Daniel Vetter
2012-06-20 9:21 ` Takashi Iwai
2012-06-20 9:36 ` Daniel Vetter
2012-06-20 9:39 ` Takashi Iwai
2012-06-21 12:11 ` Takashi Iwai
2012-06-21 12:33 ` Daniel Vetter
2012-06-21 13:17 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.