diff mbox

[11/13] drm/i915: do full backlight setup at enable time

Message ID abfedf3695358dd040016a700d3662b3d5faa982.1383920621.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jani Nikula Nov. 8, 2013, 2:49 p.m. UTC
We should now have all the information we need to do a full
initialization of the backlight registers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c |  145 ++++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 56 deletions(-)

Comments

Imre Deak Nov. 13, 2013, 5:53 p.m. UTC | #1
On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote:
> We should now have all the information we need to do a full
> initialization of the backlight registers.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |  145 ++++++++++++++++++++++--------------
>  1 file changed, 89 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 0e8f0a3..da088e3 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -719,50 +719,71 @@ static void pch_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	enum transcoder cpu_transcoder =
>  		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> -	u32 tmp;
> -
> -	tmp = I915_READ(BLC_PWM_CPU_CTL2);
> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
> -	/* Note that this can also get called through dpms changes. And
> -	 * we don't track the backlight dpms state, hence check whether
> -	 * we have to do anything first. */
> -	if (tmp & BLM_PWM_ENABLE)
> -		return;
> +	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> +	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> +		WARN(1, "cpu backlight already enabled\n");
> +		cpu_ctl2 &= ~BLM_PWM_ENABLE;
> +		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> +	}
>  
> -	if (INTEL_INFO(dev)->num_pipes == 3)
> -		tmp &= ~BLM_PIPE_SELECT_IVB;
> -	else
> -		tmp &= ~BLM_PIPE_SELECT;
> +	pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
> +	if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> +		DRM_DEBUG_KMS("pch backlight already enabled\n");
> +		pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
> +		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> +	}
>  
>  	if (cpu_transcoder == TRANSCODER_EDP)
> -		tmp |= BLM_TRANSCODER_EDP;
> +		cpu_ctl2 = BLM_TRANSCODER_EDP;
>  	else
> -		tmp |= BLM_PIPE(cpu_transcoder);
> -	tmp &= ~BLM_PWM_ENABLE;
> -
> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp);
> +		cpu_ctl2 = BLM_PIPE(cpu_transcoder);
> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
>  	POSTING_READ(BLC_PWM_CPU_CTL2);
> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp | BLM_PWM_ENABLE);
> -
> -	if (!(dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)) {
> -		tmp = I915_READ(BLC_PWM_PCH_CTL1);
> -		tmp |= BLM_PCH_PWM_ENABLE;
> -		tmp &= ~BLM_PCH_OVERRIDE_ENABLE;
> -		I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
> -	}

Like in the previous patch, why is this removed? If the w/a is not
needed any more it should be a separate patch.

> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
>  
> -	/*
> -	 * Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
> -	 * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
> -	 * registers are set.
> -	 */
> +	/* This won't stick until the above enable. */
>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> +
> +	pch_ctl2 = panel->backlight.max << 16;
> +	I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
> +
> +	pch_ctl1 = 0;
> +	if (panel->backlight.active_low_pwm)
> +		pch_ctl1 |= BLM_PCH_POLARITY;
> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> +	POSTING_READ(BLC_PWM_PCH_CTL1);
> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_PWM_ENABLE);
>  }
>  
>  static void i9xx_enable_backlight(struct intel_connector *connector)
>  {
> +	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> +	u32 ctl, freq;
>  
> +	ctl = I915_READ(BLC_PWM_CTL);
> +	if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
> +		WARN(1, "backlight already enabled\n");
> +		I915_WRITE(BLC_PWM_CTL, 0);
> +	}
> +
> +	freq = panel->backlight.max;
> +	if (panel->backlight.combination_mode)
> +		freq /= 0xff;
> +
> +	ctl = freq << 17;
> +	if (IS_GEN2(dev) && panel->backlight.combination_mode)
> +		ctl |= BLM_LEGACY_MODE;
> +	if (IS_PINEVIEW(dev) && panel->backlight.active_low_pwm)
> +		ctl |= BLM_POLARITY_PNV;
> +
> +	I915_WRITE(BLC_PWM_CTL, ctl);
> +	POSTING_READ(BLC_PWM_CTL);
> +
> +	/* XXX: combine this into above write? */
>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>  }
>  
> @@ -772,25 +793,33 @@ static void i965_enable_backlight(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> -	u32 tmp;
> -
> -	tmp = I915_READ(BLC_PWM_CTL2);
> +	u32 ctl, ctl2, freq;
>  
> -	/* Note that this can also get called through dpms changes. And
> -	 * we don't track the backlight dpms state, hence check whether
> -	 * we have to do anything first. */
> -	if (tmp & BLM_PWM_ENABLE)
> -		return;
> +	ctl2 = I915_READ(BLC_PWM_CTL2);
> +	if (ctl2 & BLM_PWM_ENABLE) {
> +		WARN(1, "backlight already enabled\n");
> +		ctl2 &= ~BLM_PWM_ENABLE;
> +		I915_WRITE(BLC_PWM_CTL2, ctl2);
> +	}
>  
> -	tmp &= ~BLM_PIPE_SELECT;
> -	tmp |= BLM_PIPE(pipe);
> -	tmp &= ~BLM_PWM_ENABLE;
> +	freq = panel->backlight.max;
> +	if (panel->backlight.combination_mode)
> +		freq /= 0xff;
>  
> -	I915_WRITE(BLC_PWM_CTL2, tmp);
> -	POSTING_READ(BLC_PWM_CTL2);
> -	I915_WRITE(BLC_PWM_CTL2, tmp | BLM_PWM_ENABLE);
> +	ctl = freq << 16;
> +	I915_WRITE(BLC_PWM_CTL, ctl);
>  
> +	/* XXX: combine this into above write? */
>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> +
> +	ctl2 = BLM_PIPE(pipe);
> +	if (panel->backlight.combination_mode)
> +		ctl2 |= BLM_COMBINATION_MODE;
> +	if (panel->backlight.active_low_pwm)
> +		ctl2 |= BLM_POLARITY_I965;
> +	I915_WRITE(BLC_PWM_CTL2, ctl2);
> +	POSTING_READ(BLC_PWM_CTL2);
> +	I915_WRITE(BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
>  }
>  
>  static void vlv_enable_backlight(struct intel_connector *connector)
> @@ -799,23 +828,27 @@ static void vlv_enable_backlight(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> -	u32 tmp;
> +	u32 ctl, ctl2;
>  
> -	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> +	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> +	if (ctl2 & BLM_PWM_ENABLE) {
> +		WARN(1, "backlight already enabled\n");
> +		ctl2 &= ~BLM_PWM_ENABLE;
> +		I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> +	}
>  
> -	/* Note that this can also get called through dpms changes. And
> -	 * we don't track the backlight dpms state, hence check whether
> -	 * we have to do anything first. */
> -	if (tmp & BLM_PWM_ENABLE)
> -		return;
> +	ctl = panel->backlight.max << 16;
> +	I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl);
>  
> -	tmp &= ~BLM_PWM_ENABLE;
> +	/* XXX: combine this into above write? */
> +	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>  
> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp);
> +	ctl2 = 0;
> +	if (panel->backlight.active_low_pwm)
> +		ctl2 |= BLM_POLARITY_I965;
> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
>  	POSTING_READ(VLV_BLC_PWM_CTL2(pipe));
> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp | BLM_PWM_ENABLE);
> -
> -	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE);
>  }
>  
>  void intel_panel_enable_backlight(struct intel_connector *connector)
Jani Nikula Nov. 14, 2013, 5:43 a.m. UTC | #2
On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote:
>> We should now have all the information we need to do a full
>> initialization of the backlight registers.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_panel.c |  145 ++++++++++++++++++++++--------------
>>  1 file changed, 89 insertions(+), 56 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index 0e8f0a3..da088e3 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -719,50 +719,71 @@ static void pch_enable_backlight(struct intel_connector *connector)
>>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>>  	enum transcoder cpu_transcoder =
>>  		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> -	u32 tmp;
>> -
>> -	tmp = I915_READ(BLC_PWM_CPU_CTL2);
>> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>>  
>> -	/* Note that this can also get called through dpms changes. And
>> -	 * we don't track the backlight dpms state, hence check whether
>> -	 * we have to do anything first. */
>> -	if (tmp & BLM_PWM_ENABLE)
>> -		return;
>> +	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>> +	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>> +		WARN(1, "cpu backlight already enabled\n");
>> +		cpu_ctl2 &= ~BLM_PWM_ENABLE;
>> +		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
>> +	}
>>  
>> -	if (INTEL_INFO(dev)->num_pipes == 3)
>> -		tmp &= ~BLM_PIPE_SELECT_IVB;
>> -	else
>> -		tmp &= ~BLM_PIPE_SELECT;
>> +	pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>> +	if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
>> +		DRM_DEBUG_KMS("pch backlight already enabled\n");
>> +		pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
>> +		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
>> +	}
>>  
>>  	if (cpu_transcoder == TRANSCODER_EDP)
>> -		tmp |= BLM_TRANSCODER_EDP;
>> +		cpu_ctl2 = BLM_TRANSCODER_EDP;
>>  	else
>> -		tmp |= BLM_PIPE(cpu_transcoder);
>> -	tmp &= ~BLM_PWM_ENABLE;
>> -
>> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp);
>> +		cpu_ctl2 = BLM_PIPE(cpu_transcoder);
>> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
>>  	POSTING_READ(BLC_PWM_CPU_CTL2);
>> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp | BLM_PWM_ENABLE);
>> -
>> -	if (!(dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)) {
>> -		tmp = I915_READ(BLC_PWM_PCH_CTL1);
>> -		tmp |= BLM_PCH_PWM_ENABLE;
>> -		tmp &= ~BLM_PCH_OVERRIDE_ENABLE;
>> -		I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
>> -	}
>
> Like in the previous patch, why is this removed? If the w/a is not
> needed any more it should be a separate patch.

As I said, I think the quirk is bogus. With the full backlight setup at
enable, we shouldn't trust this to have been set by the BIOS. Again, it
is required to be enabled, even on the machines that supposedly need the
quirk.

I expect the new enable sequence to tackle the problems. And any fallout
we should fix properly, without quirks - and with this series we are
much more flexible to adjust the sequence on a per platform basis. For
example, I was able to test on an IVB that we can't set the backlight
duty cycle until the cpu pwm has been enabled (hence the comment
below). But we do it the before the pch pwm enable.

All that said, maybe it should be a separate patch.


BR,
Jani.

>
>> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
>>  
>> -	/*
>> -	 * Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
>> -	 * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
>> -	 * registers are set.
>> -	 */
>> +	/* This won't stick until the above enable. */
>>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>> +
>> +	pch_ctl2 = panel->backlight.max << 16;
>> +	I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
>> +
>> +	pch_ctl1 = 0;
>> +	if (panel->backlight.active_low_pwm)
>> +		pch_ctl1 |= BLM_PCH_POLARITY;
>> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
>> +	POSTING_READ(BLC_PWM_PCH_CTL1);
>> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_PWM_ENABLE);
>>  }
>>  
>>  static void i9xx_enable_backlight(struct intel_connector *connector)
>>  {
>> +	struct drm_device *dev = connector->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>> +	u32 ctl, freq;
>>  
>> +	ctl = I915_READ(BLC_PWM_CTL);
>> +	if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
>> +		WARN(1, "backlight already enabled\n");
>> +		I915_WRITE(BLC_PWM_CTL, 0);
>> +	}
>> +
>> +	freq = panel->backlight.max;
>> +	if (panel->backlight.combination_mode)
>> +		freq /= 0xff;
>> +
>> +	ctl = freq << 17;
>> +	if (IS_GEN2(dev) && panel->backlight.combination_mode)
>> +		ctl |= BLM_LEGACY_MODE;
>> +	if (IS_PINEVIEW(dev) && panel->backlight.active_low_pwm)
>> +		ctl |= BLM_POLARITY_PNV;
>> +
>> +	I915_WRITE(BLC_PWM_CTL, ctl);
>> +	POSTING_READ(BLC_PWM_CTL);
>> +
>> +	/* XXX: combine this into above write? */
>>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>>  }
>>  
>> @@ -772,25 +793,33 @@ static void i965_enable_backlight(struct intel_connector *connector)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> -	u32 tmp;
>> -
>> -	tmp = I915_READ(BLC_PWM_CTL2);
>> +	u32 ctl, ctl2, freq;
>>  
>> -	/* Note that this can also get called through dpms changes. And
>> -	 * we don't track the backlight dpms state, hence check whether
>> -	 * we have to do anything first. */
>> -	if (tmp & BLM_PWM_ENABLE)
>> -		return;
>> +	ctl2 = I915_READ(BLC_PWM_CTL2);
>> +	if (ctl2 & BLM_PWM_ENABLE) {
>> +		WARN(1, "backlight already enabled\n");
>> +		ctl2 &= ~BLM_PWM_ENABLE;
>> +		I915_WRITE(BLC_PWM_CTL2, ctl2);
>> +	}
>>  
>> -	tmp &= ~BLM_PIPE_SELECT;
>> -	tmp |= BLM_PIPE(pipe);
>> -	tmp &= ~BLM_PWM_ENABLE;
>> +	freq = panel->backlight.max;
>> +	if (panel->backlight.combination_mode)
>> +		freq /= 0xff;
>>  
>> -	I915_WRITE(BLC_PWM_CTL2, tmp);
>> -	POSTING_READ(BLC_PWM_CTL2);
>> -	I915_WRITE(BLC_PWM_CTL2, tmp | BLM_PWM_ENABLE);
>> +	ctl = freq << 16;
>> +	I915_WRITE(BLC_PWM_CTL, ctl);
>>  
>> +	/* XXX: combine this into above write? */
>>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>> +
>> +	ctl2 = BLM_PIPE(pipe);
>> +	if (panel->backlight.combination_mode)
>> +		ctl2 |= BLM_COMBINATION_MODE;
>> +	if (panel->backlight.active_low_pwm)
>> +		ctl2 |= BLM_POLARITY_I965;
>> +	I915_WRITE(BLC_PWM_CTL2, ctl2);
>> +	POSTING_READ(BLC_PWM_CTL2);
>> +	I915_WRITE(BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
>>  }
>>  
>>  static void vlv_enable_backlight(struct intel_connector *connector)
>> @@ -799,23 +828,27 @@ static void vlv_enable_backlight(struct intel_connector *connector)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> -	u32 tmp;
>> +	u32 ctl, ctl2;
>>  
>> -	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
>> +	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
>> +	if (ctl2 & BLM_PWM_ENABLE) {
>> +		WARN(1, "backlight already enabled\n");
>> +		ctl2 &= ~BLM_PWM_ENABLE;
>> +		I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
>> +	}
>>  
>> -	/* Note that this can also get called through dpms changes. And
>> -	 * we don't track the backlight dpms state, hence check whether
>> -	 * we have to do anything first. */
>> -	if (tmp & BLM_PWM_ENABLE)
>> -		return;
>> +	ctl = panel->backlight.max << 16;
>> +	I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl);
>>  
>> -	tmp &= ~BLM_PWM_ENABLE;
>> +	/* XXX: combine this into above write? */
>> +	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>>  
>> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp);
>> +	ctl2 = 0;
>> +	if (panel->backlight.active_low_pwm)
>> +		ctl2 |= BLM_POLARITY_I965;
>> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
>>  	POSTING_READ(VLV_BLC_PWM_CTL2(pipe));
>> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp | BLM_PWM_ENABLE);
>> -
>> -	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE);
>>  }
>>  
>>  void intel_panel_enable_backlight(struct intel_connector *connector)
>
Daniel Vetter Nov. 14, 2013, 8:27 a.m. UTC | #3
On Thu, Nov 14, 2013 at 07:43:07AM +0200, Jani Nikula wrote:
> On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote:
> >> We should now have all the information we need to do a full
> >> initialization of the backlight registers.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_panel.c |  145 ++++++++++++++++++++++--------------
> >>  1 file changed, 89 insertions(+), 56 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index 0e8f0a3..da088e3 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -719,50 +719,71 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >>  	enum transcoder cpu_transcoder =
> >>  		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> -	u32 tmp;
> >> -
> >> -	tmp = I915_READ(BLC_PWM_CPU_CTL2);
> >> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >>  
> >> -	/* Note that this can also get called through dpms changes. And
> >> -	 * we don't track the backlight dpms state, hence check whether
> >> -	 * we have to do anything first. */
> >> -	if (tmp & BLM_PWM_ENABLE)
> >> -		return;
> >> +	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >> +	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >> +		WARN(1, "cpu backlight already enabled\n");
> >> +		cpu_ctl2 &= ~BLM_PWM_ENABLE;
> >> +		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> >> +	}
> >>  
> >> -	if (INTEL_INFO(dev)->num_pipes == 3)
> >> -		tmp &= ~BLM_PIPE_SELECT_IVB;
> >> -	else
> >> -		tmp &= ~BLM_PIPE_SELECT;
> >> +	pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
> >> +	if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> >> +		DRM_DEBUG_KMS("pch backlight already enabled\n");
> >> +		pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
> >> +		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> >> +	}
> >>  
> >>  	if (cpu_transcoder == TRANSCODER_EDP)
> >> -		tmp |= BLM_TRANSCODER_EDP;
> >> +		cpu_ctl2 = BLM_TRANSCODER_EDP;
> >>  	else
> >> -		tmp |= BLM_PIPE(cpu_transcoder);
> >> -	tmp &= ~BLM_PWM_ENABLE;
> >> -
> >> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp);
> >> +		cpu_ctl2 = BLM_PIPE(cpu_transcoder);
> >> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> >>  	POSTING_READ(BLC_PWM_CPU_CTL2);
> >> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp | BLM_PWM_ENABLE);
> >> -
> >> -	if (!(dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)) {
> >> -		tmp = I915_READ(BLC_PWM_PCH_CTL1);
> >> -		tmp |= BLM_PCH_PWM_ENABLE;
> >> -		tmp &= ~BLM_PCH_OVERRIDE_ENABLE;
> >> -		I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
> >> -	}
> >
> > Like in the previous patch, why is this removed? If the w/a is not
> > needed any more it should be a separate patch.
> 
> As I said, I think the quirk is bogus. With the full backlight setup at
> enable, we shouldn't trust this to have been set by the BIOS. Again, it
> is required to be enabled, even on the machines that supposedly need the
> quirk.
> 
> I expect the new enable sequence to tackle the problems. And any fallout
> we should fix properly, without quirks - and with this series we are
> much more flexible to adjust the sequence on a per platform basis. For
> example, I was able to test on an IVB that we can't set the backlight
> duty cycle until the cpu pwm has been enabled (hence the comment
> below). But we do it the before the pch pwm enable.
> 
> All that said, maybe it should be a separate patch.

Yeah, and then we can also rip out the quirk entries and tables with it,
too. Also maybe mention findings like the one about the cpu pwm register
in the commit message quickly.
-Daniel

> 
> 
> BR,
> Jani.
> 
> >
> >> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
> >>  
> >> -	/*
> >> -	 * Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
> >> -	 * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
> >> -	 * registers are set.
> >> -	 */
> >> +	/* This won't stick until the above enable. */
> >>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +
> >> +	pch_ctl2 = panel->backlight.max << 16;
> >> +	I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
> >> +
> >> +	pch_ctl1 = 0;
> >> +	if (panel->backlight.active_low_pwm)
> >> +		pch_ctl1 |= BLM_PCH_POLARITY;
> >> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> >> +	POSTING_READ(BLC_PWM_PCH_CTL1);
> >> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_PWM_ENABLE);
> >>  }
> >>  
> >>  static void i9xx_enable_backlight(struct intel_connector *connector)
> >>  {
> >> +	struct drm_device *dev = connector->base.dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >> +	u32 ctl, freq;
> >>  
> >> +	ctl = I915_READ(BLC_PWM_CTL);
> >> +	if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
> >> +		WARN(1, "backlight already enabled\n");
> >> +		I915_WRITE(BLC_PWM_CTL, 0);
> >> +	}
> >> +
> >> +	freq = panel->backlight.max;
> >> +	if (panel->backlight.combination_mode)
> >> +		freq /= 0xff;
> >> +
> >> +	ctl = freq << 17;
> >> +	if (IS_GEN2(dev) && panel->backlight.combination_mode)
> >> +		ctl |= BLM_LEGACY_MODE;
> >> +	if (IS_PINEVIEW(dev) && panel->backlight.active_low_pwm)
> >> +		ctl |= BLM_POLARITY_PNV;
> >> +
> >> +	I915_WRITE(BLC_PWM_CTL, ctl);
> >> +	POSTING_READ(BLC_PWM_CTL);
> >> +
> >> +	/* XXX: combine this into above write? */
> >>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >>  }
> >>  
> >> @@ -772,25 +793,33 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> -	u32 tmp;
> >> -
> >> -	tmp = I915_READ(BLC_PWM_CTL2);
> >> +	u32 ctl, ctl2, freq;
> >>  
> >> -	/* Note that this can also get called through dpms changes. And
> >> -	 * we don't track the backlight dpms state, hence check whether
> >> -	 * we have to do anything first. */
> >> -	if (tmp & BLM_PWM_ENABLE)
> >> -		return;
> >> +	ctl2 = I915_READ(BLC_PWM_CTL2);
> >> +	if (ctl2 & BLM_PWM_ENABLE) {
> >> +		WARN(1, "backlight already enabled\n");
> >> +		ctl2 &= ~BLM_PWM_ENABLE;
> >> +		I915_WRITE(BLC_PWM_CTL2, ctl2);
> >> +	}
> >>  
> >> -	tmp &= ~BLM_PIPE_SELECT;
> >> -	tmp |= BLM_PIPE(pipe);
> >> -	tmp &= ~BLM_PWM_ENABLE;
> >> +	freq = panel->backlight.max;
> >> +	if (panel->backlight.combination_mode)
> >> +		freq /= 0xff;
> >>  
> >> -	I915_WRITE(BLC_PWM_CTL2, tmp);
> >> -	POSTING_READ(BLC_PWM_CTL2);
> >> -	I915_WRITE(BLC_PWM_CTL2, tmp | BLM_PWM_ENABLE);
> >> +	ctl = freq << 16;
> >> +	I915_WRITE(BLC_PWM_CTL, ctl);
> >>  
> >> +	/* XXX: combine this into above write? */
> >>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +
> >> +	ctl2 = BLM_PIPE(pipe);
> >> +	if (panel->backlight.combination_mode)
> >> +		ctl2 |= BLM_COMBINATION_MODE;
> >> +	if (panel->backlight.active_low_pwm)
> >> +		ctl2 |= BLM_POLARITY_I965;
> >> +	I915_WRITE(BLC_PWM_CTL2, ctl2);
> >> +	POSTING_READ(BLC_PWM_CTL2);
> >> +	I915_WRITE(BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
> >>  }
> >>  
> >>  static void vlv_enable_backlight(struct intel_connector *connector)
> >> @@ -799,23 +828,27 @@ static void vlv_enable_backlight(struct intel_connector *connector)
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> -	u32 tmp;
> >> +	u32 ctl, ctl2;
> >>  
> >> -	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> >> +	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> >> +	if (ctl2 & BLM_PWM_ENABLE) {
> >> +		WARN(1, "backlight already enabled\n");
> >> +		ctl2 &= ~BLM_PWM_ENABLE;
> >> +		I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> >> +	}
> >>  
> >> -	/* Note that this can also get called through dpms changes. And
> >> -	 * we don't track the backlight dpms state, hence check whether
> >> -	 * we have to do anything first. */
> >> -	if (tmp & BLM_PWM_ENABLE)
> >> -		return;
> >> +	ctl = panel->backlight.max << 16;
> >> +	I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl);
> >>  
> >> -	tmp &= ~BLM_PWM_ENABLE;
> >> +	/* XXX: combine this into above write? */
> >> +	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >>  
> >> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp);
> >> +	ctl2 = 0;
> >> +	if (panel->backlight.active_low_pwm)
> >> +		ctl2 |= BLM_POLARITY_I965;
> >> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> >>  	POSTING_READ(VLV_BLC_PWM_CTL2(pipe));
> >> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp | BLM_PWM_ENABLE);
> >> -
> >> -	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE);
> >>  }
> >>  
> >>  void intel_panel_enable_backlight(struct intel_connector *connector)
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak Nov. 14, 2013, 8:28 a.m. UTC | #4
On Thu, 2013-11-14 at 07:43 +0200, Jani Nikula wrote:
> On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote:
> >> We should now have all the information we need to do a full
> >> initialization of the backlight registers.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_panel.c |  145 ++++++++++++++++++++++--------------
> >>  1 file changed, 89 insertions(+), 56 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index 0e8f0a3..da088e3 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -719,50 +719,71 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >>  	enum transcoder cpu_transcoder =
> >>  		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> -	u32 tmp;
> >> -
> >> -	tmp = I915_READ(BLC_PWM_CPU_CTL2);
> >> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >>  
> >> -	/* Note that this can also get called through dpms changes. And
> >> -	 * we don't track the backlight dpms state, hence check whether
> >> -	 * we have to do anything first. */
> >> -	if (tmp & BLM_PWM_ENABLE)
> >> -		return;
> >> +	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >> +	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >> +		WARN(1, "cpu backlight already enabled\n");
> >> +		cpu_ctl2 &= ~BLM_PWM_ENABLE;
> >> +		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> >> +	}
> >>  
> >> -	if (INTEL_INFO(dev)->num_pipes == 3)
> >> -		tmp &= ~BLM_PIPE_SELECT_IVB;
> >> -	else
> >> -		tmp &= ~BLM_PIPE_SELECT;
> >> +	pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
> >> +	if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> >> +		DRM_DEBUG_KMS("pch backlight already enabled\n");
> >> +		pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
> >> +		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> >> +	}
> >>  
> >>  	if (cpu_transcoder == TRANSCODER_EDP)
> >> -		tmp |= BLM_TRANSCODER_EDP;
> >> +		cpu_ctl2 = BLM_TRANSCODER_EDP;
> >>  	else
> >> -		tmp |= BLM_PIPE(cpu_transcoder);
> >> -	tmp &= ~BLM_PWM_ENABLE;
> >> -
> >> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp);
> >> +		cpu_ctl2 = BLM_PIPE(cpu_transcoder);
> >> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> >>  	POSTING_READ(BLC_PWM_CPU_CTL2);
> >> -	I915_WRITE(BLC_PWM_CPU_CTL2, tmp | BLM_PWM_ENABLE);
> >> -
> >> -	if (!(dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)) {
> >> -		tmp = I915_READ(BLC_PWM_PCH_CTL1);
> >> -		tmp |= BLM_PCH_PWM_ENABLE;
> >> -		tmp &= ~BLM_PCH_OVERRIDE_ENABLE;
> >> -		I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
> >> -	}
> >
> > Like in the previous patch, why is this removed? If the w/a is not
> > needed any more it should be a separate patch.
> 
> As I said, I think the quirk is bogus. With the full backlight setup at
> enable, we shouldn't trust this to have been set by the BIOS. Again, it
> is required to be enabled, even on the machines that supposedly need the
> quirk.
> 
> I expect the new enable sequence to tackle the problems. And any fallout
> we should fix properly, without quirks - and with this series we are
> much more flexible to adjust the sequence on a per platform basis. For
> example, I was able to test on an IVB that we can't set the backlight
> duty cycle until the cpu pwm has been enabled (hence the comment
> below). But we do it the before the pch pwm enable.
> 
> All that said, maybe it should be a separate patch.

Ok, hm I realize now that removing the quirk might not be trivial since
we depend on the new sequence to do things right. So if you decide so,
I'm also fine leaving it as-is, if it's mentioned in the log. Also since
it's not used elsewhere the quirk should be removed fully.

--Imre

> 
> 
> BR,
> Jani.
> 
> >
> >> +	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
> >>  
> >> -	/*
> >> -	 * Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
> >> -	 * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
> >> -	 * registers are set.
> >> -	 */
> >> +	/* This won't stick until the above enable. */
> >>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +
> >> +	pch_ctl2 = panel->backlight.max << 16;
> >> +	I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
> >> +
> >> +	pch_ctl1 = 0;
> >> +	if (panel->backlight.active_low_pwm)
> >> +		pch_ctl1 |= BLM_PCH_POLARITY;
> >> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> >> +	POSTING_READ(BLC_PWM_PCH_CTL1);
> >> +	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_PWM_ENABLE);
> >>  }
> >>  
> >>  static void i9xx_enable_backlight(struct intel_connector *connector)
> >>  {
> >> +	struct drm_device *dev = connector->base.dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >> +	u32 ctl, freq;
> >>  
> >> +	ctl = I915_READ(BLC_PWM_CTL);
> >> +	if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
> >> +		WARN(1, "backlight already enabled\n");
> >> +		I915_WRITE(BLC_PWM_CTL, 0);
> >> +	}
> >> +
> >> +	freq = panel->backlight.max;
> >> +	if (panel->backlight.combination_mode)
> >> +		freq /= 0xff;
> >> +
> >> +	ctl = freq << 17;
> >> +	if (IS_GEN2(dev) && panel->backlight.combination_mode)
> >> +		ctl |= BLM_LEGACY_MODE;
> >> +	if (IS_PINEVIEW(dev) && panel->backlight.active_low_pwm)
> >> +		ctl |= BLM_POLARITY_PNV;
> >> +
> >> +	I915_WRITE(BLC_PWM_CTL, ctl);
> >> +	POSTING_READ(BLC_PWM_CTL);
> >> +
> >> +	/* XXX: combine this into above write? */
> >>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >>  }
> >>  
> >> @@ -772,25 +793,33 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> -	u32 tmp;
> >> -
> >> -	tmp = I915_READ(BLC_PWM_CTL2);
> >> +	u32 ctl, ctl2, freq;
> >>  
> >> -	/* Note that this can also get called through dpms changes. And
> >> -	 * we don't track the backlight dpms state, hence check whether
> >> -	 * we have to do anything first. */
> >> -	if (tmp & BLM_PWM_ENABLE)
> >> -		return;
> >> +	ctl2 = I915_READ(BLC_PWM_CTL2);
> >> +	if (ctl2 & BLM_PWM_ENABLE) {
> >> +		WARN(1, "backlight already enabled\n");
> >> +		ctl2 &= ~BLM_PWM_ENABLE;
> >> +		I915_WRITE(BLC_PWM_CTL2, ctl2);
> >> +	}
> >>  
> >> -	tmp &= ~BLM_PIPE_SELECT;
> >> -	tmp |= BLM_PIPE(pipe);
> >> -	tmp &= ~BLM_PWM_ENABLE;
> >> +	freq = panel->backlight.max;
> >> +	if (panel->backlight.combination_mode)
> >> +		freq /= 0xff;
> >>  
> >> -	I915_WRITE(BLC_PWM_CTL2, tmp);
> >> -	POSTING_READ(BLC_PWM_CTL2);
> >> -	I915_WRITE(BLC_PWM_CTL2, tmp | BLM_PWM_ENABLE);
> >> +	ctl = freq << 16;
> >> +	I915_WRITE(BLC_PWM_CTL, ctl);
> >>  
> >> +	/* XXX: combine this into above write? */
> >>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +
> >> +	ctl2 = BLM_PIPE(pipe);
> >> +	if (panel->backlight.combination_mode)
> >> +		ctl2 |= BLM_COMBINATION_MODE;
> >> +	if (panel->backlight.active_low_pwm)
> >> +		ctl2 |= BLM_POLARITY_I965;
> >> +	I915_WRITE(BLC_PWM_CTL2, ctl2);
> >> +	POSTING_READ(BLC_PWM_CTL2);
> >> +	I915_WRITE(BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
> >>  }
> >>  
> >>  static void vlv_enable_backlight(struct intel_connector *connector)
> >> @@ -799,23 +828,27 @@ static void vlv_enable_backlight(struct intel_connector *connector)
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_panel *panel = &connector->panel;
> >>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> -	u32 tmp;
> >> +	u32 ctl, ctl2;
> >>  
> >> -	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> >> +	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> >> +	if (ctl2 & BLM_PWM_ENABLE) {
> >> +		WARN(1, "backlight already enabled\n");
> >> +		ctl2 &= ~BLM_PWM_ENABLE;
> >> +		I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> >> +	}
> >>  
> >> -	/* Note that this can also get called through dpms changes. And
> >> -	 * we don't track the backlight dpms state, hence check whether
> >> -	 * we have to do anything first. */
> >> -	if (tmp & BLM_PWM_ENABLE)
> >> -		return;
> >> +	ctl = panel->backlight.max << 16;
> >> +	I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl);
> >>  
> >> -	tmp &= ~BLM_PWM_ENABLE;
> >> +	/* XXX: combine this into above write? */
> >> +	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >>  
> >> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp);
> >> +	ctl2 = 0;
> >> +	if (panel->backlight.active_low_pwm)
> >> +		ctl2 |= BLM_POLARITY_I965;
> >> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> >>  	POSTING_READ(VLV_BLC_PWM_CTL2(pipe));
> >> -	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp | BLM_PWM_ENABLE);
> >> -
> >> -	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE);
> >>  }
> >>  
> >>  void intel_panel_enable_backlight(struct intel_connector *connector)
> >
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 0e8f0a3..da088e3 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -719,50 +719,71 @@  static void pch_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	enum transcoder cpu_transcoder =
 		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
-	u32 tmp;
-
-	tmp = I915_READ(BLC_PWM_CPU_CTL2);
+	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
 
-	/* Note that this can also get called through dpms changes. And
-	 * we don't track the backlight dpms state, hence check whether
-	 * we have to do anything first. */
-	if (tmp & BLM_PWM_ENABLE)
-		return;
+	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
+	if (cpu_ctl2 & BLM_PWM_ENABLE) {
+		WARN(1, "cpu backlight already enabled\n");
+		cpu_ctl2 &= ~BLM_PWM_ENABLE;
+		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
+	}
 
-	if (INTEL_INFO(dev)->num_pipes == 3)
-		tmp &= ~BLM_PIPE_SELECT_IVB;
-	else
-		tmp &= ~BLM_PIPE_SELECT;
+	pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
+	if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
+		DRM_DEBUG_KMS("pch backlight already enabled\n");
+		pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
+		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
+	}
 
 	if (cpu_transcoder == TRANSCODER_EDP)
-		tmp |= BLM_TRANSCODER_EDP;
+		cpu_ctl2 = BLM_TRANSCODER_EDP;
 	else
-		tmp |= BLM_PIPE(cpu_transcoder);
-	tmp &= ~BLM_PWM_ENABLE;
-
-	I915_WRITE(BLC_PWM_CPU_CTL2, tmp);
+		cpu_ctl2 = BLM_PIPE(cpu_transcoder);
+	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
 	POSTING_READ(BLC_PWM_CPU_CTL2);
-	I915_WRITE(BLC_PWM_CPU_CTL2, tmp | BLM_PWM_ENABLE);
-
-	if (!(dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)) {
-		tmp = I915_READ(BLC_PWM_PCH_CTL1);
-		tmp |= BLM_PCH_PWM_ENABLE;
-		tmp &= ~BLM_PCH_OVERRIDE_ENABLE;
-		I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
-	}
+	I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
 
-	/*
-	 * Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
-	 * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
-	 * registers are set.
-	 */
+	/* This won't stick until the above enable. */
 	intel_panel_actually_set_backlight(connector, panel->backlight.level);
+
+	pch_ctl2 = panel->backlight.max << 16;
+	I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
+
+	pch_ctl1 = 0;
+	if (panel->backlight.active_low_pwm)
+		pch_ctl1 |= BLM_PCH_POLARITY;
+	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
+	POSTING_READ(BLC_PWM_PCH_CTL1);
+	I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_PWM_ENABLE);
 }
 
 static void i9xx_enable_backlight(struct intel_connector *connector)
 {
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
+	u32 ctl, freq;
 
+	ctl = I915_READ(BLC_PWM_CTL);
+	if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
+		WARN(1, "backlight already enabled\n");
+		I915_WRITE(BLC_PWM_CTL, 0);
+	}
+
+	freq = panel->backlight.max;
+	if (panel->backlight.combination_mode)
+		freq /= 0xff;
+
+	ctl = freq << 17;
+	if (IS_GEN2(dev) && panel->backlight.combination_mode)
+		ctl |= BLM_LEGACY_MODE;
+	if (IS_PINEVIEW(dev) && panel->backlight.active_low_pwm)
+		ctl |= BLM_POLARITY_PNV;
+
+	I915_WRITE(BLC_PWM_CTL, ctl);
+	POSTING_READ(BLC_PWM_CTL);
+
+	/* XXX: combine this into above write? */
 	intel_panel_actually_set_backlight(connector, panel->backlight.level);
 }
 
@@ -772,25 +793,33 @@  static void i965_enable_backlight(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
-	u32 tmp;
-
-	tmp = I915_READ(BLC_PWM_CTL2);
+	u32 ctl, ctl2, freq;
 
-	/* Note that this can also get called through dpms changes. And
-	 * we don't track the backlight dpms state, hence check whether
-	 * we have to do anything first. */
-	if (tmp & BLM_PWM_ENABLE)
-		return;
+	ctl2 = I915_READ(BLC_PWM_CTL2);
+	if (ctl2 & BLM_PWM_ENABLE) {
+		WARN(1, "backlight already enabled\n");
+		ctl2 &= ~BLM_PWM_ENABLE;
+		I915_WRITE(BLC_PWM_CTL2, ctl2);
+	}
 
-	tmp &= ~BLM_PIPE_SELECT;
-	tmp |= BLM_PIPE(pipe);
-	tmp &= ~BLM_PWM_ENABLE;
+	freq = panel->backlight.max;
+	if (panel->backlight.combination_mode)
+		freq /= 0xff;
 
-	I915_WRITE(BLC_PWM_CTL2, tmp);
-	POSTING_READ(BLC_PWM_CTL2);
-	I915_WRITE(BLC_PWM_CTL2, tmp | BLM_PWM_ENABLE);
+	ctl = freq << 16;
+	I915_WRITE(BLC_PWM_CTL, ctl);
 
+	/* XXX: combine this into above write? */
 	intel_panel_actually_set_backlight(connector, panel->backlight.level);
+
+	ctl2 = BLM_PIPE(pipe);
+	if (panel->backlight.combination_mode)
+		ctl2 |= BLM_COMBINATION_MODE;
+	if (panel->backlight.active_low_pwm)
+		ctl2 |= BLM_POLARITY_I965;
+	I915_WRITE(BLC_PWM_CTL2, ctl2);
+	POSTING_READ(BLC_PWM_CTL2);
+	I915_WRITE(BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
 }
 
 static void vlv_enable_backlight(struct intel_connector *connector)
@@ -799,23 +828,27 @@  static void vlv_enable_backlight(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
-	u32 tmp;
+	u32 ctl, ctl2;
 
-	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
+	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
+	if (ctl2 & BLM_PWM_ENABLE) {
+		WARN(1, "backlight already enabled\n");
+		ctl2 &= ~BLM_PWM_ENABLE;
+		I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
+	}
 
-	/* Note that this can also get called through dpms changes. And
-	 * we don't track the backlight dpms state, hence check whether
-	 * we have to do anything first. */
-	if (tmp & BLM_PWM_ENABLE)
-		return;
+	ctl = panel->backlight.max << 16;
+	I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl);
 
-	tmp &= ~BLM_PWM_ENABLE;
+	/* XXX: combine this into above write? */
+	intel_panel_actually_set_backlight(connector, panel->backlight.level);
 
-	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp);
+	ctl2 = 0;
+	if (panel->backlight.active_low_pwm)
+		ctl2 |= BLM_POLARITY_I965;
+	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
 	POSTING_READ(VLV_BLC_PWM_CTL2(pipe));
-	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp | BLM_PWM_ENABLE);
-
-	intel_panel_actually_set_backlight(connector, panel->backlight.level);
+	I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE);
 }
 
 void intel_panel_enable_backlight(struct intel_connector *connector)