Re: [RESEND PATCH 2/5] drm/i915/backlight: Fix backlight takeover on LPT, v2.

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

On Mon, 17 Dec 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> On lynxpoint the bios sometimes sets up the backlight using the CPU
> display, but the driver expects using the PWM PCH override register.
>
> Read the value from the CPU register, then convert it to the other
> units by converting from the old duty cycle, to freq, to the new units.
>
> This value is then programmed in the override register, after which
> we set the override and disable the CPU display control. This allows
> us to switch the source without flickering, and make the backlight
> controls work in the driver.
>
> Changes since v1:
> - Read BLC_PWM_CPU_CTL2 to cpu_ctl2.
> - Clean up cpu_mode if slightly.
> - Always disable BLM_PWM_ENABLE in cpu_ctl2.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108225
> Cc: Basil Eric Rabi <ericbasil.rabi@gmail.com>
> Cc: Hans de Goede <jwrdegoede@fedoraproject.org>
> Cc: Tolga Cakir <cevelnet@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Tolga Cakir <cevelnet@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 40 +++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 799284fcd57d..3e3ce7a77700 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1484,8 +1484,8 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	u32 pch_ctl1, pch_ctl2, val;
> -	bool alt;
> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2, val;
> +	bool alt, cpu_mode;
>  
>  	if (HAS_PCH_LPT(dev_priv))
>  		alt = I915_READ(SOUTH_CHICKEN2) & LPT_PWM_GRANULARITY;
> @@ -1499,6 +1499,8 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus
>  	pch_ctl2 = I915_READ(BLC_PWM_PCH_CTL2);
>  	panel->backlight.max = pch_ctl2 >> 16;
>  
> +	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> +
>  	if (!panel->backlight.max)
>  		panel->backlight.max = get_backlight_max_vbt(connector);
>  
> @@ -1507,12 +1509,42 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus
>  
>  	panel->backlight.min = get_backlight_min_vbt(connector);
>  
> -	val = lpt_get_backlight(connector);
> +	panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
> +
> +	cpu_mode = panel->backlight.enabled && HAS_PCH_LPT(dev_priv) &&
> +		   !(pch_ctl1 & BLM_PCH_OVERRIDE_ENABLE) &&
> +		   (cpu_ctl2 & BLM_PWM_ENABLE);
> +	if (cpu_mode) {
> +		u32 freq;
> +
> +		/*
> +		 * We're in cpu mode, convert to PCH units.
> +		 *
> +		 * Convert CPU pwm tick back to hz, back to new PCH units again.
> +		 * this is the same formula as pch_hz_to_pwm, but the other way
> +		 * around..
> +		 */
> +		val = pch_get_backlight(connector);
> +		freq = DIV_ROUND_CLOSEST(KHz(dev_priv->rawclk_freq), val * 128);
> +
> +		DRM_DEBUG_KMS("Backlight PCH value: %u, converted to freq %u, converted to lpt units %u, minmax: %u/%u\n",
> +			      val, freq, lpt_hz_to_pwm(connector, freq), panel->backlight.min, panel->backlight.max);
> +
> +		val = lpt_hz_to_pwm(connector, freq);

If the CPU register is driving the PCH PWM, I think you're good with
using val = pch_get_backlight(connector) directly. In that case, the
modulation frequency should be set in the PCH register anyway, and it
should be all right. The increments and reference clocks and everything
should be the same. So the duty cycle should be the same.

The CPU register modulation frequency stuff should only be used for
driving the backlight in the utility pin. That's a possibility we should
maybe check, but if this patch fixes anything, that's not the case
here. It's a physically different pin, and we only support that on
BXT. IIRC I've been told nobody uses that on HSW/LPT, and I don't recall
any relevant bug reports either.

Using the utility pin would involve:

* Setting the utility pin mode to PWM, and enabling it in UTIL_PIN_CTL
  (0x48400).

* Setting the PWM pin selects in BLC_MISC_CTL (0x48360).

* Setting the modulation frequency in BLC_PWM_CPU_CTL (0x48254) *or* in
  HSW_BLC_PWM2_CTL (0x48354) depending on BLC_MISC_CTL.

Perhaps we should add WARN_ON()s on util pin pwm mode and non-zero blc
misc, but that's another patch.

TL;DR make that:

	if (cpu_mode)
		val = pch_get_backlight(connector);
	else
		val = lpt_get_backlight(connector);

> +	} else
> +		val = lpt_get_backlight(connector);
>  	val = intel_panel_compute_brightness(connector, val);
>  	panel->backlight.level = clamp(val, panel->backlight.min,
>  				       panel->backlight.max);
>  
> -	panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
> +	if (cpu_mode) {

After the simplicication above, perhaps add a debug log here instead
about switching to PCH override.

> +		/* Write converted CPU PWM value to PCH override register */
> +		lpt_set_backlight(connector->base.state, panel->backlight.level);
> +		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
> +	}
> +
> +	if (cpu_ctl2 & BLM_PWM_ENABLE)
> +		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 & ~BLM_PWM_ENABLE);

Please move that within the cpu_mode block above. I want to minimize the
impact on !cpu_mode.

BR,
Jani.

>  
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx