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

[Jani Nikula <jani.nikula@intel.com>]

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)
>

-- 
Jani Nikula, Intel Open Source Technology Center