Re: [PATCH] drm/i915/backlight: Fix backlight takeover on LPT

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

On Tue, 20 Nov 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.
>
> 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>
> Tested-by: Tolga Cakir <cevelnet@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 34 +++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e6cd7b55c018..3357232f1b0e 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1485,7 +1485,7 @@ 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;
> +	bool alt, cpu_mode = false;
>  
>  	if (HAS_PCH_LPT(dev_priv))
>  		alt = I915_READ(SOUTH_CHICKEN2) & LPT_PWM_GRANULARITY;
> @@ -1507,12 +1507,40 @@ 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;
> +	if (panel->backlight.enabled && HAS_PCH_LPT(dev_priv) &&
> +	    (I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE) &&
> +	    !WARN_ON(pch_ctl1 & BLM_PCH_OVERRIDE_ENABLE)) {

I think there's something wrong with the conditions here, but can't
quite wrap my head around it.

At least it seems wrong to warn about override enable in this case. If
override is set, it'll be used. So the warning should really be about
cpu_ctl2 being enabled when it's not used.

Perhaps read cpu_ctl2 into a variable, and reuse that also later for
disable.

BR,
Jani.

> +		u32 freq;
> +
> +		cpu_mode = true;
> +		/*
> +		 * 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);
> +
> +		val = lpt_hz_to_pwm(connector, freq);
> +	} 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) {
> +		u32 tmp;
> +
> +		/* Use PWM mode, instead of cpu clock */
> +		lpt_set_backlight(connector->base.state, panel->backlight.level);
> +		I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
> +
> +		tmp = I915_READ(BLC_PWM_CPU_CTL2);
> +		I915_WRITE(BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
> +	}
>  
>  	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