On Thu, 07 Jan 2021, Lyude Paul lyude@redhat.com wrote:
@@ -1628,37 +1633,32 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
pch_ctl2 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL2);
- panel->backlight.max = pch_ctl2 >> 16;
panel->backlight.pwm_level_max = pch_ctl2 >> 16;
cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
- if (!panel->backlight.max)
panel->backlight.max = get_backlight_max_vbt(connector);
- if (!panel->backlight.pwm_level_max)
panel->backlight.pwm_level_max = get_backlight_max_vbt(connector);
- if (!panel->backlight.max)
- if (!panel->backlight.pwm_level_max) return -ENODEV;
- panel->backlight.min = get_backlight_min_vbt(connector);
- panel->backlight.pwm_level_min = get_backlight_min_vbt(connector);
- panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
- panel->backlight.pwm_enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
- cpu_mode = panel->backlight.enabled && HAS_PCH_LPT(dev_priv) &&
- cpu_mode = panel->backlight.pwm_enabled && HAS_PCH_LPT(dev_priv) && !(pch_ctl1 & BLM_PCH_OVERRIDE_ENABLE) && (cpu_ctl2 & BLM_PWM_ENABLE);
if (cpu_mode)
val = pch_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);if (cpu_mode) {
val = intel_panel_sanitize_pwm_level(connector, pch_get_backlight(connector));
(This really is a PITA to review, not because of how you do it but because of the hardware and the code itself. I'm just pointing out one thing here, but I'm not finished yet.)
I think this sanitize call is wrong here. It should be called only when converting to and from the hw register. Here, we read directly from one hw register and write back to another hw register.
Now, looking at the history, I think it's been wrong all the way since commit 5b1ec9ac7ab5 ("drm/i915/backlight: Fix backlight takeover on LPT, v3."). Probably nobody noticed, because AFAIK inverted brightness control has only ever been an issue on some gen4 platforms...
*facepalm*
BR, Jani.
drm_dbg_kms(&dev_priv->drm, "CPU backlight register was enabled, switching to PCH override\n"); /* Write converted CPU PWM value to PCH override register */
lpt_set_backlight(connector->base.state, panel->backlight.level);
lpt_set_backlight(connector->base.state, val);