diff mbox

[5/6] drm/i915: don't wait for power cycle when waiting for power off

Message ID 1387470584-1662-6-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Dec. 19, 2013, 4:29 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Function ironlake_wait_panel_off should just wait for the power off
delay, while function ironlake_wait_panel_power_cycle should wait for
the panel cycle (that's required after we turn the panel off, before
we enable it again).

The problem is that, currently, ironlake_wait_panel_off is waiting not
just for the panel to be off, but also for the power cycle delay and
the backlight off delay. This function relies on the PP_STATUS bits
3:0, which are not documented and not supposed to be used. A quick
analysis of the values we get while waiting quickly shows that power
off is reached while bits 3:0 are still 0x1, and the time it takes to
become 0x0 is the power cycle delay.

On my system with backlight off delay of 200ms, power down delay of
50ms and power cycle delay of 500ms, this is what I get:
 - Start waiting with value 0x80000008, timestamp 6.429364.
 - Jumps to 0xa0000003, timestamp 6.431360 (time waited: 0.001996)
 - Jumps to 0xa0000002, timestamp 6.631277 (time waited: 0.201913)
 - Jumps to 0x08000001, timestamp 6.681258 (time waited: 0.251894)
 - Jumps to 0x00000000, timestamp 7.192012 (time waited: 0.762648)

As you can see, ironlake_wait_panel_off is sleeping 760ms instead of
the expected 50ms: the first 200ms matches the backlight off delay
(which we should already have waited for!), then the 50ms for the real
panel off delay, then the 500ms for the panel power cycle.

This patch makes is look just at bits 31 and 29:28, which will ignore
the panel power cycle.

And just to be clear: this saves 500ms on my system every time we
disable the panel. But we can still save 200ms more (the backlight off
delay) on the next patches.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jesse Barnes Dec. 19, 2013, 5:38 p.m. UTC | #1 
On Thu, 19 Dec 2013 14:29:43 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Function ironlake_wait_panel_off should just wait for the power off
> delay, while function ironlake_wait_panel_power_cycle should wait for
> the panel cycle (that's required after we turn the panel off, before
> we enable it again).
> 
> The problem is that, currently, ironlake_wait_panel_off is waiting not
> just for the panel to be off, but also for the power cycle delay and
> the backlight off delay. This function relies on the PP_STATUS bits
> 3:0, which are not documented and not supposed to be used. A quick
> analysis of the values we get while waiting quickly shows that power
> off is reached while bits 3:0 are still 0x1, and the time it takes to
> become 0x0 is the power cycle delay.
> 
> On my system with backlight off delay of 200ms, power down delay of
> 50ms and power cycle delay of 500ms, this is what I get:
>  - Start waiting with value 0x80000008, timestamp 6.429364.
>  - Jumps to 0xa0000003, timestamp 6.431360 (time waited: 0.001996)
>  - Jumps to 0xa0000002, timestamp 6.631277 (time waited: 0.201913)
>  - Jumps to 0x08000001, timestamp 6.681258 (time waited: 0.251894)
>  - Jumps to 0x00000000, timestamp 7.192012 (time waited: 0.762648)
> 
> As you can see, ironlake_wait_panel_off is sleeping 760ms instead of
> the expected 50ms: the first 200ms matches the backlight off delay
> (which we should already have waited for!), then the 50ms for the real
> panel off delay, then the 500ms for the panel power cycle.
> 
> This patch makes is look just at bits 31 and 29:28, which will ignore
> the panel power cycle.
> 
> And just to be clear: this saves 500ms on my system every time we
> disable the panel. But we can still save 200ms more (the backlight off
> delay) on the next patches.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a6a4c4f..69d8f1c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1011,8 +1011,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
>  #define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
>  #define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
>  
> -#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> -#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> +#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | 0)
> +#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | 0)
>  
>  #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
>  #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)

It would be good to note confirmation from the hw guys in the commit
msg, and maybe get some data on an LVDS panel just to be sure, but
otherwise:

Reviewed-by: Jesse Barnes <jbarnes@virtuougseek.org>
Jesse Barnes Dec. 19, 2013, 6:34 p.m. UTC | #2 
On Thu, 19 Dec 2013 09:38:45 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu, 19 Dec 2013 14:29:43 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Function ironlake_wait_panel_off should just wait for the power off
> > delay, while function ironlake_wait_panel_power_cycle should wait for
> > the panel cycle (that's required after we turn the panel off, before
> > we enable it again).
> > 
> > The problem is that, currently, ironlake_wait_panel_off is waiting not
> > just for the panel to be off, but also for the power cycle delay and
> > the backlight off delay. This function relies on the PP_STATUS bits
> > 3:0, which are not documented and not supposed to be used. A quick
> > analysis of the values we get while waiting quickly shows that power
> > off is reached while bits 3:0 are still 0x1, and the time it takes to
> > become 0x0 is the power cycle delay.
> > 
> > On my system with backlight off delay of 200ms, power down delay of
> > 50ms and power cycle delay of 500ms, this is what I get:
> >  - Start waiting with value 0x80000008, timestamp 6.429364.
> >  - Jumps to 0xa0000003, timestamp 6.431360 (time waited: 0.001996)
> >  - Jumps to 0xa0000002, timestamp 6.631277 (time waited: 0.201913)
> >  - Jumps to 0x08000001, timestamp 6.681258 (time waited: 0.251894)
> >  - Jumps to 0x00000000, timestamp 7.192012 (time waited: 0.762648)
> > 
> > As you can see, ironlake_wait_panel_off is sleeping 760ms instead of
> > the expected 50ms: the first 200ms matches the backlight off delay
> > (which we should already have waited for!), then the 50ms for the real
> > panel off delay, then the 500ms for the panel power cycle.
> > 
> > This patch makes is look just at bits 31 and 29:28, which will ignore
> > the panel power cycle.
> > 
> > And just to be clear: this saves 500ms on my system every time we
> > disable the panel. But we can still save 200ms more (the backlight off
> > delay) on the next patches.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index a6a4c4f..69d8f1c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1011,8 +1011,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
> >  #define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> >  #define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
> >  
> > -#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> > -#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> > +#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | 0)
> > +#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | 0)
> >  
> >  #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
> >  #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> 
> It would be good to note confirmation from the hw guys in the commit
> msg, and maybe get some data on an LVDS panel just to be sure, but
> otherwise:

For the LVDS bit we'd need to apply similar changes on the LVDS side,
but I think that may already be pretty fast, so you can ignore that bit.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a6a4c4f..69d8f1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1011,8 +1011,8 @@  static void intel_dp_mode_set(struct intel_encoder *encoder)
 #define IDLE_ON_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
 #define IDLE_ON_VALUE   	(PP_ON | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
 
-#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
-#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+#define IDLE_OFF_MASK		(PP_ON | PP_SEQUENCE_MASK | 0                     | 0)
+#define IDLE_OFF_VALUE		(0     | PP_SEQUENCE_NONE | 0                     | 0)
 
 #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
 #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)