diff mbox

[6/6] drm/i915: set the backlight panel delays registers to 1

Message ID 1387470584-1662-7-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>

Because we already do the wait in software: see
ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.

For the "backlight on" delay, even BSpec says we need to program 0x1
to PP_ON_DELAYS 12:0.

For the "backlight off" delay, if we don't do the same thing, when we
call ironlake_wait_panel_off we'll end up waiting for the it again.

On my machine the off delay is 200ms, so we save this amount of time
whenever we disable the panel (e.g, suspend).

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

Comments

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

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because we already do the wait in software: see
> ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> 
> For the "backlight on" delay, even BSpec says we need to program 0x1
> to PP_ON_DELAYS 12:0.
> 
> For the "backlight off" delay, if we don't do the same thing, when we
> call ironlake_wait_panel_off we'll end up waiting for the it again.
> 
> On my machine the off delay is 200ms, so we save this amount of time
> whenever we disable the panel (e.g, suspend).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 69d8f1c..90ff059 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
>  	}
>  
> -	/* And finally store the new values in the power sequencer. */
> +	/*
> +	 * And finally store the new values in the power sequencer. The
> +	 * backlight delays are set to 1 because we do manual waits on them. For
> +	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
> +	 * we'll end up waiting for the backlight off delay twice: once when we
> +	 * do the manual sleep, and once when we disable the panel and wait for
> +	 * the PP_STATUS bit to become zero.
> +	 */
>  	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
> -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> +		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
> +	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
>  		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
>  	/* Compute the divisor for the pp clock, simply match the Bspec
>  	 * formula. */

Yay!

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Daniel Vetter Jan. 17, 2014, 1:57 p.m. UTC | #2 
On Thu, Dec 19, 2013 at 09:39:12AM -0800, Jesse Barnes wrote:
> On Thu, 19 Dec 2013 14:29:44 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Because we already do the wait in software: see
> > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> > 
> > For the "backlight on" delay, even BSpec says we need to program 0x1
> > to PP_ON_DELAYS 12:0.
> > 
> > For the "backlight off" delay, if we don't do the same thing, when we
> > call ironlake_wait_panel_off we'll end up waiting for the it again.
> > 
> > On my machine the off delay is 200ms, so we save this amount of time
> > whenever we disable the panel (e.g, suspend).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 69d8f1c..90ff059 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> >  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> >  	}
> >  
> > -	/* And finally store the new values in the power sequencer. */
> > +	/*
> > +	 * And finally store the new values in the power sequencer. The
> > +	 * backlight delays are set to 1 because we do manual waits on them. For
> > +	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
> > +	 * we'll end up waiting for the backlight off delay twice: once when we
> > +	 * do the manual sleep, and once when we disable the panel and wait for
> > +	 * the PP_STATUS bit to become zero.
> > +	 */
> >  	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
> > -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> > -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> > +		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
> > +	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> >  		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
> >  	/* Compute the divisor for the pp clock, simply match the Bspec
> >  	 * formula. */
> 
> Yay!
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I'll wait with this one until the updated version of patch 3 is finalized,
but I've meanwhile pulled in patches 4&5.
-Daniel
Daniel Vetter Jan. 20, 2014, 4:12 p.m. UTC | #3 
On Fri, Jan 17, 2014 at 02:57:42PM +0100, Daniel Vetter wrote:
> On Thu, Dec 19, 2013 at 09:39:12AM -0800, Jesse Barnes wrote:
> > On Thu, 19 Dec 2013 14:29:44 -0200
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> > 
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > Because we already do the wait in software: see
> > > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> > > 
> > > For the "backlight on" delay, even BSpec says we need to program 0x1
> > > to PP_ON_DELAYS 12:0.
> > > 
> > > For the "backlight off" delay, if we don't do the same thing, when we
> > > call ironlake_wait_panel_off we'll end up waiting for the it again.
> > > 
> > > On my machine the off delay is 200ms, so we save this amount of time
> > > whenever we disable the panel (e.g, suspend).
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 69d8f1c..90ff059 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > >  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> > >  	}
> > >  
> > > -	/* And finally store the new values in the power sequencer. */
> > > +	/*
> > > +	 * And finally store the new values in the power sequencer. The
> > > +	 * backlight delays are set to 1 because we do manual waits on them. For
> > > +	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
> > > +	 * we'll end up waiting for the backlight off delay twice: once when we
> > > +	 * do the manual sleep, and once when we disable the panel and wait for
> > > +	 * the PP_STATUS bit to become zero.
> > > +	 */
> > >  	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
> > > -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> > > -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> > > +		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
> > > +	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> > >  		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
> > >  	/* Compute the divisor for the pp clock, simply match the Bspec
> > >  	 * formula. */
> > 
> > Yay!
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> I'll wait with this one until the updated version of patch 3 is finalized,
> but I've meanwhile pulled in patches 4&5.

Paulo convinced me that this is save already, so merged to dinq.
-Daniel
Jani Nikula Jan. 28, 2014, 7:57 a.m. UTC | #4 
On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Because we already do the wait in software: see
> ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
>
> For the "backlight on" delay, even BSpec says we need to program 0x1
> to PP_ON_DELAYS 12:0.
>
> For the "backlight off" delay, if we don't do the same thing, when we
> call ironlake_wait_panel_off we'll end up waiting for the it again.
>
> On my machine the off delay is 200ms, so we save this amount of time
> whenever we disable the panel (e.g, suspend).

Whoa. This appears to fix an eDP black screen with 24->18 bpp dithering:

https://bugs.freedesktop.org/show_bug.cgi?id=73567#c21

Jani.


>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 69d8f1c..90ff059 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3535,10 +3535,17 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
>  	}
>  
> -	/* And finally store the new values in the power sequencer. */
> +	/*
> +	 * And finally store the new values in the power sequencer. The
> +	 * backlight delays are set to 1 because we do manual waits on them. For
> +	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
> +	 * we'll end up waiting for the backlight off delay twice: once when we
> +	 * do the manual sleep, and once when we disable the panel and wait for
> +	 * the PP_STATUS bit to become zero.
> +	 */
>  	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
> -		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> -	pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> +		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
> +	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
>  		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
>  	/* Compute the divisor for the pp clock, simply match the Bspec
>  	 * formula. */
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Jan. 28, 2014, 8:02 a.m. UTC | #5 
On Tue, Jan 28, 2014 at 09:57:57AM +0200, Jani Nikula wrote:
> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Because we already do the wait in software: see
> > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
> >
> > For the "backlight on" delay, even BSpec says we need to program 0x1
> > to PP_ON_DELAYS 12:0.
> >
> > For the "backlight off" delay, if we don't do the same thing, when we
> > call ironlake_wait_panel_off we'll end up waiting for the it again.
> >
> > On my machine the off delay is 200ms, so we save this amount of time
> > whenever we disable the panel (e.g, suspend).
> 
> Whoa. This appears to fix an eDP black screen with 24->18 bpp dithering:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=73567#c21

I dunno whether I should cry or laugh ... Problem is that I don't really
see a way to port just this patch to 3.14. So we need the entire series,
which is a bit much imo. At least until we have more users scaling our
walls.
-Daniel
Jani Nikula Jan. 28, 2014, 8:23 a.m. UTC | #6 
On Tue, 28 Jan 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jan 28, 2014 at 09:57:57AM +0200, Jani Nikula wrote:
>> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > Because we already do the wait in software: see
>> > ironlake_wait_backlight_on and ironlake_edp_wait_backlight_off.
>> >
>> > For the "backlight on" delay, even BSpec says we need to program 0x1
>> > to PP_ON_DELAYS 12:0.
>> >
>> > For the "backlight off" delay, if we don't do the same thing, when we
>> > call ironlake_wait_panel_off we'll end up waiting for the it again.
>> >
>> > On my machine the off delay is 200ms, so we save this amount of time
>> > whenever we disable the panel (e.g, suspend).
>> 
>> Whoa. This appears to fix an eDP black screen with 24->18 bpp dithering:
>> 
>> https://bugs.freedesktop.org/show_bug.cgi?id=73567#c21
>
> I dunno whether I should cry or laugh ... Problem is that I don't really
> see a way to port just this patch to 3.14. So we need the entire series,
> which is a bit much imo. At least until we have more users scaling our
> walls.

I also dunno whether I should cry or laugh... but for me it's because
the report has two devices of the same model, and one gets fixed by this
and the other not...

BR,
Jani.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 69d8f1c..90ff059 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3535,10 +3535,17 @@  intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
 	}
 
-	/* And finally store the new values in the power sequencer. */
+	/*
+	 * And finally store the new values in the power sequencer. The
+	 * backlight delays are set to 1 because we do manual waits on them. For
+	 * T8, even BSpec recommends doing it. For T9, if we don't do this,
+	 * we'll end up waiting for the backlight off delay twice: once when we
+	 * do the manual sleep, and once when we disable the panel and wait for
+	 * the PP_STATUS bit to become zero.
+	 */
 	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
-		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
-	pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
+		(1 << PANEL_LIGHT_ON_DELAY_SHIFT);
+	pp_off = (1 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
 		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
 	/* Compute the divisor for the pp clock, simply match the Bspec
 	 * formula. */