2020-09-16 17:32:54

by Lyude Paul

Subject: [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

Currently, every different type of backlight hook that i915 supports is
pretty straight forward - you have a backlight, probably through PWM
(but maybe DPCD), with a single set of platform-specific hooks that are
used for controlling it.

HDR backlights, in particular VESA and Intel's HDR backlight
implementations, can end up being more complicated. With Intel's
proprietary interface, HDR backlight controls always run through the
DPCD. When the backlight is in SDR backlight mode however, the driver
may need to bypass the TCON and control the backlight directly through
PWM.

So, in order to support this we'll need to split our backlight callbacks
into two groups: a set of high-level backlight control callbacks in
intel_panel, and an additional set of pwm-specific backlight control
callbacks. This also implies a functional changes for how these
callbacks are used:

* We now keep track of two separate backlight level ranges, one for the
high-level backlight, and one for the pwm backlight range
* We also keep track of backlight enablement and PWM backlight
enablement separately
* Since the currently set backlight level might not be the same as the
currently programmed PWM backlight level, we stop setting
panel->backlight.level with the currently programmed PWM backlight
level in panel->backlight.pwm_funcs.setup(). Instead, we rely
on the higher level backlight control functions to retrieve the
current PWM backlight level (in this case, intel_pwm_get_backlight()).
Note that there are still a few PWM backlight setup callbacks that
do actually need to retrieve the current PWM backlight level, although
we no longer save this value in panel->backlight.level like before.
* panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
brightness level, unlike their siblings
panel->backlight.enable()/disable(). This is so we can calculate the
actual PWM brightness level we want to set on disable/enable in the
higher level backlight enable()/disable() functions, since this value
might be scaled from a brightness level that doesn't come from PWM.

Signed-off-by: Lyude Paul <[email protected]>
Cc: [email protected]
Cc: Vasily Khoruzhick <[email protected]>
---
.../drm/i915/display/intel_display_types.h | 14 +-
drivers/gpu/drm/i915/display/intel_panel.c | 436 ++++++++++--------
2 files changed, 246 insertions(+), 204 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index b2d0edacc58c9..52a6543df842a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -221,6 +221,9 @@ struct intel_panel {
bool alternate_pwm_increment; /* lpt+ */

/* PWM chip */
+ u32 pwm_min;
+ u32 pwm_max;
+ bool pwm_enabled;
bool util_pin_active_low; /* bxt+ */
u8 controller; /* bxt+ only */
struct pwm_device *pwm;
@@ -229,6 +232,16 @@ struct intel_panel {
/* DPCD backlight */
u8 pwmgen_bit_count;

+ struct {
+ int (*setup)(struct intel_connector *connector, enum pipe pipe);
+ u32 (*get)(struct intel_connector *connector);
+ void (*set)(const struct drm_connector_state *conn_state, u32 level);
+ void (*disable)(const struct drm_connector_state *conn_state, u32 level);
+ void (*enable)(const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state, u32 level);
+ u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);
+ } pwm_funcs;
+
struct backlight_device *device;

/* Connector and platform specific backlight functions */
@@ -238,7 +251,6 @@ struct intel_panel {
void (*disable)(const struct drm_connector_state *conn_state);
void (*enable)(const struct intel_crtc_state *crtc_state,
const struct drm_connector_state *conn_state);
- u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);
void (*power)(struct intel_connector *, bool enable);
} backlight;
};
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index c0e36244bb07d..6d3e9d51d069c 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -511,25 +511,34 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
0, user_max);
}

-static u32 intel_panel_compute_brightness(struct intel_connector *connector,
- u32 val)
+static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;

- drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
+ drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);

if (dev_priv->params.invert_brightness < 0)
return val;

if (dev_priv->params.invert_brightness > 0 ||
dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) {
- return panel->backlight.max - val + panel->backlight.min;
+ return panel->backlight.pwm_max - val + panel->backlight.pwm_min;
}

return val;
}

+static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
+{
+ struct intel_connector *connector = to_intel_connector(conn_state->connector);
+ struct drm_i915_private *i915 = to_i915(connector->base.dev);
+ struct intel_panel *panel = &connector->panel;
+
+ drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", val);
+ panel->backlight.pwm_funcs.set(conn_state, val);
+}
+
static u32 lpt_get_backlight(struct intel_connector *connector)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -624,12 +633,12 @@ static void i9xx_set_backlight(const struct drm_connector_state *conn_state, u32
struct intel_panel *panel = &connector->panel;
u32 tmp, mask;

- drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
+ drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);

if (panel->backlight.combination_mode) {
u8 lbpc;

- lbpc = level * 0xfe / panel->backlight.max + 1;
+ lbpc = level * 0xfe / panel->backlight.pwm_max + 1;
level /= lbpc;
pci_write_config_byte(dev_priv->drm.pdev, LBPC, lbpc);
}
@@ -681,9 +690,8 @@ intel_panel_actually_set_backlight(const struct drm_connector_state *conn_state,
struct drm_i915_private *i915 = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;

- drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", level);
+ drm_dbg_kms(&i915->drm, "set backlight level = %d\n", level);

- level = intel_panel_compute_brightness(connector, level);
panel->backlight.set(conn_state, level);
}

@@ -726,13 +734,13 @@ void intel_panel_set_backlight_acpi(const struct drm_connector_state *conn_state
mutex_unlock(&dev_priv->backlight_lock);
}

-static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
u32 tmp;

- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, level);

/*
* Although we don't support or enable CPU PWM with LPT/SPT based
@@ -754,13 +762,13 @@ static void lpt_disable_backlight(const struct drm_connector_state *old_conn_sta
intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
}

-static void pch_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void pch_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
{
struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
u32 tmp;

- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, val);

tmp = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
@@ -769,44 +777,44 @@ static void pch_disable_backlight(const struct drm_connector_state *old_conn_sta
intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
}

-static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
{
- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, val);
}

-static void i965_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void i965_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
{
struct drm_i915_private *dev_priv = to_i915(old_conn_state->connector->dev);
u32 tmp;

- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, val);

tmp = intel_de_read(dev_priv, BLC_PWM_CTL2);
intel_de_write(dev_priv, BLC_PWM_CTL2, tmp & ~BLM_PWM_ENABLE);
}

-static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
{
struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
enum pipe pipe = to_intel_crtc(old_conn_state->crtc)->pipe;
u32 tmp;

- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, val);

tmp = intel_de_read(dev_priv, VLV_BLC_PWM_CTL2(pipe));
intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe),
tmp & ~BLM_PWM_ENABLE);
}

-static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
{
struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;
- u32 tmp, val;
+ u32 tmp;

- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, val);

tmp = intel_de_read(dev_priv,
BXT_BLC_PWM_CTL(panel->backlight.controller));
@@ -820,14 +828,14 @@ static void bxt_disable_backlight(const struct drm_connector_state *old_conn_sta
}
}

-static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
{
struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;
u32 tmp;

- intel_panel_actually_set_backlight(old_conn_state, 0);
+ intel_panel_set_pwm_level(old_conn_state, val);

tmp = intel_de_read(dev_priv,
BXT_BLC_PWM_CTL(panel->backlight.controller));
@@ -835,7 +843,7 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
tmp & ~BXT_BLC_PWM_ENABLE);
}

-static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
struct intel_panel *panel = &connector->panel;
@@ -876,7 +884,7 @@ void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_st
}

static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -906,7 +914,7 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_write(dev_priv, SOUTH_CHICKEN1, schicken);
}

- pch_ctl2 = panel->backlight.max << 16;
+ pch_ctl2 = panel->backlight.pwm_max << 16;
intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);

pch_ctl1 = 0;
@@ -923,11 +931,11 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
pch_ctl1 | BLM_PCH_PWM_ENABLE);

/* This won't stick until the above enable. */
- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);
}

static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -958,9 +966,9 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);

/* This won't stick until the above enable. */
- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);

- pch_ctl2 = panel->backlight.max << 16;
+ pch_ctl2 = panel->backlight.pwm_max << 16;
intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);

pch_ctl1 = 0;
@@ -974,7 +982,7 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
}

static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -987,7 +995,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_write(dev_priv, BLC_PWM_CTL, 0);
}

- freq = panel->backlight.max;
+ freq = panel->backlight.pwm_max;
if (panel->backlight.combination_mode)
freq /= 0xff;

@@ -1001,7 +1009,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_posting_read(dev_priv, BLC_PWM_CTL);

/* XXX: combine this into above write? */
- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);

/*
* Needed to enable backlight on some 855gm models. BLC_HIST_CTL is
@@ -1013,7 +1021,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
}

static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1028,7 +1036,7 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2);
}

- freq = panel->backlight.max;
+ freq = panel->backlight.pwm_max;
if (panel->backlight.combination_mode)
freq /= 0xff;

@@ -1044,11 +1052,11 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_posting_read(dev_priv, BLC_PWM_CTL2);
intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);

- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);
}

static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1063,11 +1071,11 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe), ctl2);
}

- ctl = panel->backlight.max << 16;
+ ctl = panel->backlight.pwm_max << 16;
intel_de_write(dev_priv, VLV_BLC_PWM_CTL(pipe), ctl);

/* XXX: combine this into above write? */
- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);

ctl2 = 0;
if (panel->backlight.active_low_pwm)
@@ -1079,7 +1087,7 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
}

static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1116,9 +1124,9 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,

intel_de_write(dev_priv,
BXT_BLC_PWM_FREQ(panel->backlight.controller),
- panel->backlight.max);
+ panel->backlight.pwm_max);

- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);

pwm_ctl = 0;
if (panel->backlight.active_low_pwm)
@@ -1133,7 +1141,7 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
}

static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1152,9 +1160,9 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,

intel_de_write(dev_priv,
BXT_BLC_PWM_FREQ(panel->backlight.controller),
- panel->backlight.max);
+ panel->backlight.pwm_max);

- intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+ intel_panel_set_pwm_level(conn_state, level);

pwm_ctl = 0;
if (panel->backlight.active_low_pwm)
@@ -1169,13 +1177,11 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
}

static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
- const struct drm_connector_state *conn_state)
+ const struct drm_connector_state *conn_state, u32 level)
{
struct intel_connector *connector = to_intel_connector(conn_state->connector);
struct intel_panel *panel = &connector->panel;
- int level = panel->backlight.level;

- level = intel_panel_compute_brightness(connector, level);
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
panel->backlight.pwm_state.enabled = true;
pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
@@ -1233,10 +1239,8 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)

mutex_lock(&dev_priv->backlight_lock);

- if (panel->backlight.enabled) {
+ if (panel->backlight.enabled)
val = panel->backlight.get(connector);
- val = intel_panel_compute_brightness(connector, val);
- }

mutex_unlock(&dev_priv->backlight_lock);

@@ -1567,13 +1571,13 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector)
u16 pwm_freq_hz = get_vbt_pwm_freq(dev_priv);
u32 pwm;

- if (!panel->backlight.hz_to_pwm) {
+ if (!panel->backlight.pwm_funcs.hz_to_pwm) {
drm_dbg_kms(&dev_priv->drm,
"backlight frequency conversion not supported\n");
return 0;
}

- pwm = panel->backlight.hz_to_pwm(connector, pwm_freq_hz);
+ pwm = panel->backlight.pwm_funcs.hz_to_pwm(connector, pwm_freq_hz);
if (!pwm) {
drm_dbg_kms(&dev_priv->drm,
"backlight frequency conversion failed\n");
@@ -1592,7 +1596,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
struct intel_panel *panel = &connector->panel;
int min;

- drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
+ drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);

/*
* XXX: If the vbt value is 255, it makes min equal to max, which leads
@@ -1609,7 +1613,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
}

/* vbt value is a coefficient in range [0..255] */
- return scale(min, 0, 255, 0, panel->backlight.max);
+ return scale(min, 0, 255, 0, panel->backlight.pwm_max);
}

static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unused)
@@ -1629,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_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_max)
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

- panel->backlight.min = get_backlight_min_vbt(connector);
+ panel->backlight.pwm_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));
+
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);
intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);

@@ -1674,29 +1673,24 @@ static int pch_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 cpu_ctl2, pch_ctl1, pch_ctl2, val;
+ u32 cpu_ctl2, pch_ctl1, pch_ctl2;

pch_ctl1 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL1);
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_max = pch_ctl2 >> 16;

- if (!panel->backlight.max)
- panel->backlight.max = get_backlight_max_vbt(connector);
+ if (!panel->backlight.pwm_max)
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

- panel->backlight.min = get_backlight_min_vbt(connector);
-
- val = pch_get_backlight(connector);
- val = intel_panel_compute_brightness(connector, val);
- panel->backlight.level = clamp(val, panel->backlight.min,
- panel->backlight.max);
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
- panel->backlight.enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
+ panel->backlight.pwm_enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
(pch_ctl1 & BLM_PCH_PWM_ENABLE);

return 0;
@@ -1716,27 +1710,26 @@ static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unu
if (IS_PINEVIEW(dev_priv))
panel->backlight.active_low_pwm = ctl & BLM_POLARITY_PNV;

- panel->backlight.max = ctl >> 17;
+ panel->backlight.pwm_max = ctl >> 17;

- if (!panel->backlight.max) {
- panel->backlight.max = get_backlight_max_vbt(connector);
- panel->backlight.max >>= 1;
+ if (!panel->backlight.pwm_max) {
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);
+ panel->backlight.pwm_max >>= 1;
}

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

if (panel->backlight.combination_mode)
- panel->backlight.max *= 0xff;
+ panel->backlight.pwm_max *= 0xff;

- panel->backlight.min = get_backlight_min_vbt(connector);
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

val = i9xx_get_backlight(connector);
- val = intel_panel_compute_brightness(connector, val);
- panel->backlight.level = clamp(val, panel->backlight.min,
- panel->backlight.max);
+ val = intel_panel_sanitize_pwm_level(connector, val);
+ val = clamp(val, panel->backlight.pwm_min, panel->backlight.pwm_max);

- panel->backlight.enabled = val != 0;
+ panel->backlight.pwm_enabled = val != 0;

return 0;
}
@@ -1745,32 +1738,27 @@ static int i965_setup_backlight(struct intel_connector *connector, enum pipe unu
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;
- u32 ctl, ctl2, val;
+ u32 ctl, ctl2;

ctl2 = intel_de_read(dev_priv, BLC_PWM_CTL2);
panel->backlight.combination_mode = ctl2 & BLM_COMBINATION_MODE;
panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;

ctl = intel_de_read(dev_priv, BLC_PWM_CTL);
- panel->backlight.max = ctl >> 16;
+ panel->backlight.pwm_max = ctl >> 16;

- if (!panel->backlight.max)
- panel->backlight.max = get_backlight_max_vbt(connector);
+ if (!panel->backlight.pwm_max)
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

if (panel->backlight.combination_mode)
- panel->backlight.max *= 0xff;
+ panel->backlight.pwm_max *= 0xff;

- panel->backlight.min = get_backlight_min_vbt(connector);
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

- val = i9xx_get_backlight(connector);
- val = intel_panel_compute_brightness(connector, val);
- panel->backlight.level = clamp(val, panel->backlight.min,
- panel->backlight.max);
-
- panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
+ panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;

return 0;
}
@@ -1779,7 +1767,7 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;
- u32 ctl, ctl2, val;
+ u32 ctl, ctl2;

if (drm_WARN_ON(&dev_priv->drm, pipe != PIPE_A && pipe != PIPE_B))
return -ENODEV;
@@ -1788,22 +1776,17 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;

ctl = intel_de_read(dev_priv, VLV_BLC_PWM_CTL(pipe));
- panel->backlight.max = ctl >> 16;
+ panel->backlight.pwm_max = ctl >> 16;

- if (!panel->backlight.max)
- panel->backlight.max = get_backlight_max_vbt(connector);
+ if (!panel->backlight.pwm_max)
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

- panel->backlight.min = get_backlight_min_vbt(connector);
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

- val = _vlv_get_backlight(dev_priv, pipe);
- val = intel_panel_compute_brightness(connector, val);
- panel->backlight.level = clamp(val, panel->backlight.min,
- panel->backlight.max);
-
- panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
+ panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;

return 0;
}
@@ -1828,24 +1811,18 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
}

panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
- panel->backlight.max =
- intel_de_read(dev_priv,
- BXT_BLC_PWM_FREQ(panel->backlight.controller));
+ panel->backlight.pwm_max = intel_de_read(dev_priv,
+ BXT_BLC_PWM_FREQ(panel->backlight.controller));

- if (!panel->backlight.max)
- panel->backlight.max = get_backlight_max_vbt(connector);
+ if (!panel->backlight.pwm_max)
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

- panel->backlight.min = get_backlight_min_vbt(connector);
-
- val = bxt_get_backlight(connector);
- val = intel_panel_compute_brightness(connector, val);
- panel->backlight.level = clamp(val, panel->backlight.min,
- panel->backlight.max);
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

- panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
+ panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;

return 0;
}
@@ -1855,7 +1832,7 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;
- u32 pwm_ctl, val;
+ u32 pwm_ctl;

/*
* CNP has the BXT implementation of backlight, but with only one
@@ -1868,24 +1845,18 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
BXT_BLC_PWM_CTL(panel->backlight.controller));

panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
- panel->backlight.max =
- intel_de_read(dev_priv,
- BXT_BLC_PWM_FREQ(panel->backlight.controller));
+ panel->backlight.pwm_max = intel_de_read(dev_priv,
+ BXT_BLC_PWM_FREQ(panel->backlight.controller));

- if (!panel->backlight.max)
- panel->backlight.max = get_backlight_max_vbt(connector);
+ if (!panel->backlight.pwm_max)
+ panel->backlight.pwm_max = get_backlight_max_vbt(connector);

- if (!panel->backlight.max)
+ if (!panel->backlight.pwm_max)
return -ENODEV;

- panel->backlight.min = get_backlight_min_vbt(connector);
-
- val = bxt_get_backlight(connector);
- val = intel_panel_compute_brightness(connector, val);
- panel->backlight.level = clamp(val, panel->backlight.min,
- panel->backlight.max);
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

- panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
+ panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;

return 0;
}
@@ -1915,8 +1886,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
return -ENODEV;
}

- panel->backlight.max = 100; /* 100% */
- panel->backlight.min = get_backlight_min_vbt(connector);
+ panel->backlight.pwm_max = 100; /* 100% */
+ panel->backlight.pwm_min = get_backlight_min_vbt(connector);

if (pwm_is_enabled(panel->backlight.pwm)) {
/* PWM is already enabled, use existing settings */
@@ -1924,10 +1895,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,

level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
100);
- level = intel_panel_compute_brightness(connector, level);
- panel->backlight.level = clamp(level, panel->backlight.min,
- panel->backlight.max);
- panel->backlight.enabled = true;
+ level = intel_panel_sanitize_pwm_level(connector, level);
+ panel->backlight.pwm_enabled = true;

drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n",
NSEC_PER_SEC / (unsigned long)panel->backlight.pwm_state.period,
@@ -1943,6 +1912,60 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
return 0;
}

+static void intel_pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
+{
+ struct intel_connector *connector = to_intel_connector(conn_state->connector);
+ struct intel_panel *panel = &connector->panel;
+
+ panel->backlight.pwm_funcs.set(conn_state,
+ intel_panel_sanitize_pwm_level(connector, level));
+}
+
+static u32 intel_pwm_get_backlight(struct intel_connector *connector)
+{
+ struct intel_panel *panel = &connector->panel;
+
+ return intel_panel_sanitize_pwm_level(connector, panel->backlight.pwm_funcs.get(connector));
+}
+
+static void intel_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state)
+{
+ struct intel_connector *connector = to_intel_connector(conn_state->connector);
+ struct intel_panel *panel = &connector->panel;
+ u32 level = intel_panel_sanitize_pwm_level(connector, panel->backlight.level);
+
+ panel->backlight.pwm_funcs.enable(crtc_state, conn_state, level);
+}
+
+static void intel_pwm_disable_backlight(const struct drm_connector_state *conn_state)
+{
+ struct intel_connector *connector = to_intel_connector(conn_state->connector);
+ struct intel_panel *panel = &connector->panel;
+
+ panel->backlight.pwm_funcs.disable(conn_state,
+ intel_panel_sanitize_pwm_level(connector, 0));
+}
+
+/* The only hook PWM-only backlight setups need to override, the rest of the hooks are filled in
+ * from pwm_funcs
+ */
+static int intel_pwm_setup_backlight(struct intel_connector *connector, enum pipe pipe)
+{
+ struct intel_panel *panel = &connector->panel;
+ int ret = panel->backlight.pwm_funcs.setup(connector, pipe);
+
+ if (ret < 0)
+ return ret;
+
+ panel->backlight.min = panel->backlight.pwm_min;
+ panel->backlight.max = panel->backlight.pwm_max;
+ panel->backlight.level = panel->backlight.get(connector);
+ panel->backlight.enabled = panel->backlight.pwm_enabled;
+
+ return 0;
+}
+
void intel_panel_update_backlight(struct intel_atomic_state *state,
struct intel_encoder *encoder,
const struct intel_crtc_state *crtc_state,
@@ -2024,75 +2047,82 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
container_of(panel, struct intel_connector, panel);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);

- if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
- intel_dp_aux_init_backlight_funcs(connector) == 0)
- return;
-
if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI &&
intel_dsi_dcs_init_backlight_funcs(connector) == 0)
return;

if (IS_GEN9_LP(dev_priv)) {
- panel->backlight.setup = bxt_setup_backlight;
- panel->backlight.enable = bxt_enable_backlight;
- panel->backlight.disable = bxt_disable_backlight;
- panel->backlight.set = bxt_set_backlight;
- panel->backlight.get = bxt_get_backlight;
- panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
+ panel->backlight.pwm_funcs.setup = bxt_setup_backlight;
+ panel->backlight.pwm_funcs.enable = bxt_enable_backlight;
+ panel->backlight.pwm_funcs.disable = bxt_disable_backlight;
+ panel->backlight.pwm_funcs.set = bxt_set_backlight;
+ panel->backlight.pwm_funcs.get = bxt_get_backlight;
+ panel->backlight.pwm_funcs.hz_to_pwm = bxt_hz_to_pwm;
} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
- panel->backlight.setup = cnp_setup_backlight;
- panel->backlight.enable = cnp_enable_backlight;
- panel->backlight.disable = cnp_disable_backlight;
- panel->backlight.set = bxt_set_backlight;
- panel->backlight.get = bxt_get_backlight;
- panel->backlight.hz_to_pwm = cnp_hz_to_pwm;
+ panel->backlight.pwm_funcs.setup = cnp_setup_backlight;
+ panel->backlight.pwm_funcs.enable = cnp_enable_backlight;
+ panel->backlight.pwm_funcs.disable = cnp_disable_backlight;
+ panel->backlight.pwm_funcs.set = bxt_set_backlight;
+ panel->backlight.pwm_funcs.get = bxt_get_backlight;
+ panel->backlight.pwm_funcs.hz_to_pwm = cnp_hz_to_pwm;
} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_LPT) {
- panel->backlight.setup = lpt_setup_backlight;
- panel->backlight.enable = lpt_enable_backlight;
- panel->backlight.disable = lpt_disable_backlight;
- panel->backlight.set = lpt_set_backlight;
- panel->backlight.get = lpt_get_backlight;
+ panel->backlight.pwm_funcs.setup = lpt_setup_backlight;
+ panel->backlight.pwm_funcs.enable = lpt_enable_backlight;
+ panel->backlight.pwm_funcs.disable = lpt_disable_backlight;
+ panel->backlight.pwm_funcs.set = lpt_set_backlight;
+ panel->backlight.pwm_funcs.get = lpt_get_backlight;
if (HAS_PCH_LPT(dev_priv))
- panel->backlight.hz_to_pwm = lpt_hz_to_pwm;
+ panel->backlight.pwm_funcs.hz_to_pwm = lpt_hz_to_pwm;
else
- panel->backlight.hz_to_pwm = spt_hz_to_pwm;
+ panel->backlight.pwm_funcs.hz_to_pwm = spt_hz_to_pwm;
} else if (HAS_PCH_SPLIT(dev_priv)) {
- panel->backlight.setup = pch_setup_backlight;
- panel->backlight.enable = pch_enable_backlight;
- panel->backlight.disable = pch_disable_backlight;
- panel->backlight.set = pch_set_backlight;
- panel->backlight.get = pch_get_backlight;
- panel->backlight.hz_to_pwm = pch_hz_to_pwm;
+ panel->backlight.pwm_funcs.setup = pch_setup_backlight;
+ panel->backlight.pwm_funcs.enable = pch_enable_backlight;
+ panel->backlight.pwm_funcs.disable = pch_disable_backlight;
+ panel->backlight.pwm_funcs.set = pch_set_backlight;
+ panel->backlight.pwm_funcs.get = pch_get_backlight;
+ panel->backlight.pwm_funcs.hz_to_pwm = pch_hz_to_pwm;
} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
- panel->backlight.setup = ext_pwm_setup_backlight;
- panel->backlight.enable = ext_pwm_enable_backlight;
- panel->backlight.disable = ext_pwm_disable_backlight;
- panel->backlight.set = ext_pwm_set_backlight;
- panel->backlight.get = ext_pwm_get_backlight;
+ panel->backlight.pwm_funcs.setup = ext_pwm_setup_backlight;
+ panel->backlight.pwm_funcs.enable = ext_pwm_enable_backlight;
+ panel->backlight.pwm_funcs.disable = ext_pwm_disable_backlight;
+ panel->backlight.pwm_funcs.set = ext_pwm_set_backlight;
+ panel->backlight.pwm_funcs.get = ext_pwm_get_backlight;
} else {
- panel->backlight.setup = vlv_setup_backlight;
- panel->backlight.enable = vlv_enable_backlight;
- panel->backlight.disable = vlv_disable_backlight;
- panel->backlight.set = vlv_set_backlight;
- panel->backlight.get = vlv_get_backlight;
- panel->backlight.hz_to_pwm = vlv_hz_to_pwm;
+ panel->backlight.pwm_funcs.setup = vlv_setup_backlight;
+ panel->backlight.pwm_funcs.enable = vlv_enable_backlight;
+ panel->backlight.pwm_funcs.disable = vlv_disable_backlight;
+ panel->backlight.pwm_funcs.set = vlv_set_backlight;
+ panel->backlight.pwm_funcs.get = vlv_get_backlight;
+ panel->backlight.pwm_funcs.hz_to_pwm = vlv_hz_to_pwm;
}
} else if (IS_GEN(dev_priv, 4)) {
- panel->backlight.setup = i965_setup_backlight;
- panel->backlight.enable = i965_enable_backlight;
- panel->backlight.disable = i965_disable_backlight;
- panel->backlight.set = i9xx_set_backlight;
- panel->backlight.get = i9xx_get_backlight;
- panel->backlight.hz_to_pwm = i965_hz_to_pwm;
+ panel->backlight.pwm_funcs.setup = i965_setup_backlight;
+ panel->backlight.pwm_funcs.enable = i965_enable_backlight;
+ panel->backlight.pwm_funcs.disable = i965_disable_backlight;
+ panel->backlight.pwm_funcs.set = i9xx_set_backlight;
+ panel->backlight.pwm_funcs.get = i9xx_get_backlight;
+ panel->backlight.pwm_funcs.hz_to_pwm = i965_hz_to_pwm;
} else {
- panel->backlight.setup = i9xx_setup_backlight;
- panel->backlight.enable = i9xx_enable_backlight;
- panel->backlight.disable = i9xx_disable_backlight;
- panel->backlight.set = i9xx_set_backlight;
- panel->backlight.get = i9xx_get_backlight;
- panel->backlight.hz_to_pwm = i9xx_hz_to_pwm;
+ panel->backlight.pwm_funcs.setup = i9xx_setup_backlight;
+ panel->backlight.pwm_funcs.enable = i9xx_enable_backlight;
+ panel->backlight.pwm_funcs.disable = i9xx_disable_backlight;
+ panel->backlight.pwm_funcs.set = i9xx_set_backlight;
+ panel->backlight.pwm_funcs.get = i9xx_get_backlight;
+ panel->backlight.pwm_funcs.hz_to_pwm = i9xx_hz_to_pwm;
}
+
+ if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
+ intel_dp_aux_init_backlight_funcs(connector) == 0)
+ return;
+
+ /* We're using a standard PWM backlight interface */
+ panel->backlight.setup = intel_pwm_setup_backlight;
+ panel->backlight.enable = intel_pwm_enable_backlight;
+ panel->backlight.disable = intel_pwm_disable_backlight;
+ panel->backlight.set = intel_pwm_set_backlight;
+ panel->backlight.get = intel_pwm_get_backlight;
}

enum drm_connector_status
--
2.26.2

2020-10-15 21:53:14

by Rodrigo Vivi

Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

On Wed, Sep 16, 2020 at 01:18:50PM -0400, Lyude Paul wrote:
> Currently, every different type of backlight hook that i915 supports is
> pretty straight forward - you have a backlight, probably through PWM
> (but maybe DPCD), with a single set of platform-specific hooks that are
> used for controlling it.
>
> HDR backlights, in particular VESA and Intel's HDR backlight
> implementations, can end up being more complicated. With Intel's
> proprietary interface, HDR backlight controls always run through the
> DPCD. When the backlight is in SDR backlight mode however, the driver
> may need to bypass the TCON and control the backlight directly through
> PWM.
>
> So, in order to support this we'll need to split our backlight callbacks
> into two groups: a set of high-level backlight control callbacks in
> intel_panel, and an additional set of pwm-specific backlight control
> callbacks. This also implies a functional changes for how these
> callbacks are used:
>
> * We now keep track of two separate backlight level ranges, one for the
> high-level backlight, and one for the pwm backlight range
> * We also keep track of backlight enablement and PWM backlight
> enablement separately
> * Since the currently set backlight level might not be the same as the
> currently programmed PWM backlight level, we stop setting
> panel->backlight.level with the currently programmed PWM backlight
> level in panel->backlight.pwm_funcs.setup(). Instead, we rely
> on the higher level backlight control functions to retrieve the
> current PWM backlight level (in this case, intel_pwm_get_backlight()).
> Note that there are still a few PWM backlight setup callbacks that
> do actually need to retrieve the current PWM backlight level, although
> we no longer save this value in panel->backlight.level like before.
> * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
> brightness level, unlike their siblings
> panel->backlight.enable()/disable(). This is so we can calculate the
> actual PWM brightness level we want to set on disable/enable in the
> higher level backlight enable()/disable() functions, since this value
> might be scaled from a brightness level that doesn't come from PWM.
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: [email protected]
> Cc: Vasily Khoruzhick <[email protected]>
> ---
> .../drm/i915/display/intel_display_types.h | 14 +-
> drivers/gpu/drm/i915/display/intel_panel.c | 436 ++++++++++--------
> 2 files changed, 246 insertions(+), 204 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index b2d0edacc58c9..52a6543df842a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -221,6 +221,9 @@ struct intel_panel {
> bool alternate_pwm_increment; /* lpt+ */
>
> /* PWM chip */
> + u32 pwm_min;
> + u32 pwm_max;
> + bool pwm_enabled;
> bool util_pin_active_low; /* bxt+ */
> u8 controller; /* bxt+ only */
> struct pwm_device *pwm;
> @@ -229,6 +232,16 @@ struct intel_panel {
> /* DPCD backlight */
> u8 pwmgen_bit_count;
>
> + struct {
> + int (*setup)(struct intel_connector *connector, enum pipe pipe);
> + u32 (*get)(struct intel_connector *connector);
> + void (*set)(const struct drm_connector_state *conn_state, u32 level);
> + void (*disable)(const struct drm_connector_state *conn_state, u32 level);
> + void (*enable)(const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state, u32 level);
> + u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);
> + } pwm_funcs;
> +
> struct backlight_device *device;
>
> /* Connector and platform specific backlight functions */
> @@ -238,7 +251,6 @@ struct intel_panel {
> void (*disable)(const struct drm_connector_state *conn_state);
> void (*enable)(const struct intel_crtc_state *crtc_state,
> const struct drm_connector_state *conn_state);
> - u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);

I see my poor brain getting confused with these 2 levels with very similar function sets
with same name.
But I currently have no suggestion for better organization or names...

> void (*power)(struct intel_connector *, bool enable);
> } backlight;
> };
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index c0e36244bb07d..6d3e9d51d069c 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -511,25 +511,34 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
> 0, user_max);
> }
>
> -static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> - u32 val)
> +static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
>
> - drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
> + drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
>
> if (dev_priv->params.invert_brightness < 0)
> return val;
>
> if (dev_priv->params.invert_brightness > 0 ||
> dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) {
> - return panel->backlight.max - val + panel->backlight.min;
> + return panel->backlight.pwm_max - val + panel->backlight.pwm_min;
> }
>
> return val;
> }
>
> +static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> + struct intel_panel *panel = &connector->panel;
> +
> + drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", val);
> + panel->backlight.pwm_funcs.set(conn_state, val);
> +}
> +
> static u32 lpt_get_backlight(struct intel_connector *connector)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -624,12 +633,12 @@ static void i9xx_set_backlight(const struct drm_connector_state *conn_state, u32
> struct intel_panel *panel = &connector->panel;
> u32 tmp, mask;
>
> - drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
> + drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
>
> if (panel->backlight.combination_mode) {
> u8 lbpc;
>
> - lbpc = level * 0xfe / panel->backlight.max + 1;
> + lbpc = level * 0xfe / panel->backlight.pwm_max + 1;
> level /= lbpc;
> pci_write_config_byte(dev_priv->drm.pdev, LBPC, lbpc);
> }
> @@ -681,9 +690,8 @@ intel_panel_actually_set_backlight(const struct drm_connector_state *conn_state,
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
>
> - drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", level);
> + drm_dbg_kms(&i915->drm, "set backlight level = %d\n", level);
>
> - level = intel_panel_compute_brightness(connector, level);
> panel->backlight.set(conn_state, level);
> }
>
> @@ -726,13 +734,13 @@ void intel_panel_set_backlight_acpi(const struct drm_connector_state *conn_state
> mutex_unlock(&dev_priv->backlight_lock);
> }
>
> -static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> u32 tmp;
>
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, level);
>
> /*
> * Although we don't support or enable CPU PWM with LPT/SPT based
> @@ -754,13 +762,13 @@ static void lpt_disable_backlight(const struct drm_connector_state *old_conn_sta
> intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
> }
>
> -static void pch_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void pch_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> {
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> u32 tmp;
>
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, val);
>
> tmp = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
> intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
> @@ -769,44 +777,44 @@ static void pch_disable_backlight(const struct drm_connector_state *old_conn_sta
> intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
> }
>
> -static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> {
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, val);
> }
>
> -static void i965_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void i965_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> {
> struct drm_i915_private *dev_priv = to_i915(old_conn_state->connector->dev);
> u32 tmp;
>
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, val);
>
> tmp = intel_de_read(dev_priv, BLC_PWM_CTL2);
> intel_de_write(dev_priv, BLC_PWM_CTL2, tmp & ~BLM_PWM_ENABLE);
> }
>
> -static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> {
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> enum pipe pipe = to_intel_crtc(old_conn_state->crtc)->pipe;
> u32 tmp;
>
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, val);
>
> tmp = intel_de_read(dev_priv, VLV_BLC_PWM_CTL2(pipe));
> intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe),
> tmp & ~BLM_PWM_ENABLE);
> }
>
> -static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> {
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 tmp, val;
> + u32 tmp;
>
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, val);
>
> tmp = intel_de_read(dev_priv,
> BXT_BLC_PWM_CTL(panel->backlight.controller));
> @@ -820,14 +828,14 @@ static void bxt_disable_backlight(const struct drm_connector_state *old_conn_sta
> }
> }
>
> -static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> {
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> u32 tmp;
>
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> + intel_panel_set_pwm_level(old_conn_state, val);
>
> tmp = intel_de_read(dev_priv,
> BXT_BLC_PWM_CTL(panel->backlight.controller));
> @@ -835,7 +843,7 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
> tmp & ~BXT_BLC_PWM_ENABLE);
> }
>
> -static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct intel_panel *panel = &connector->panel;
> @@ -876,7 +884,7 @@ void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_st
> }
>
> static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -906,7 +914,7 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_write(dev_priv, SOUTH_CHICKEN1, schicken);
> }
>
> - pch_ctl2 = panel->backlight.max << 16;
> + pch_ctl2 = panel->backlight.pwm_max << 16;
> intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);
>
> pch_ctl1 = 0;
> @@ -923,11 +931,11 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
> pch_ctl1 | BLM_PCH_PWM_ENABLE);
>
> /* This won't stick until the above enable. */
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
> }
>
> static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -958,9 +966,9 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
>
> /* This won't stick until the above enable. */
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
>
> - pch_ctl2 = panel->backlight.max << 16;
> + pch_ctl2 = panel->backlight.pwm_max << 16;
> intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);
>
> pch_ctl1 = 0;
> @@ -974,7 +982,7 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
> }
>
> static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -987,7 +995,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_write(dev_priv, BLC_PWM_CTL, 0);
> }
>
> - freq = panel->backlight.max;
> + freq = panel->backlight.pwm_max;
> if (panel->backlight.combination_mode)
> freq /= 0xff;
>
> @@ -1001,7 +1009,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_posting_read(dev_priv, BLC_PWM_CTL);
>
> /* XXX: combine this into above write? */
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
>
> /*
> * Needed to enable backlight on some 855gm models. BLC_HIST_CTL is
> @@ -1013,7 +1021,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
> }
>
> static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1028,7 +1036,7 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2);
> }
>
> - freq = panel->backlight.max;
> + freq = panel->backlight.pwm_max;
> if (panel->backlight.combination_mode)
> freq /= 0xff;
>
> @@ -1044,11 +1052,11 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_posting_read(dev_priv, BLC_PWM_CTL2);
> intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
>
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
> }
>
> static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1063,11 +1071,11 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe), ctl2);
> }
>
> - ctl = panel->backlight.max << 16;
> + ctl = panel->backlight.pwm_max << 16;
> intel_de_write(dev_priv, VLV_BLC_PWM_CTL(pipe), ctl);
>
> /* XXX: combine this into above write? */
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
>
> ctl2 = 0;
> if (panel->backlight.active_low_pwm)
> @@ -1079,7 +1087,7 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
> }
>
> static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1116,9 +1124,9 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> intel_de_write(dev_priv,
> BXT_BLC_PWM_FREQ(panel->backlight.controller),
> - panel->backlight.max);
> + panel->backlight.pwm_max);
>
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
>
> pwm_ctl = 0;
> if (panel->backlight.active_low_pwm)
> @@ -1133,7 +1141,7 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
> }
>
> static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1152,9 +1160,9 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> intel_de_write(dev_priv,
> BXT_BLC_PWM_FREQ(panel->backlight.controller),
> - panel->backlight.max);
> + panel->backlight.pwm_max);
>
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + intel_panel_set_pwm_level(conn_state, level);
>
> pwm_ctl = 0;
> if (panel->backlight.active_low_pwm)
> @@ -1169,13 +1177,11 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
> }
>
> static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
> - const struct drm_connector_state *conn_state)
> + const struct drm_connector_state *conn_state, u32 level)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct intel_panel *panel = &connector->panel;
> - int level = panel->backlight.level;
>
> - level = intel_panel_compute_brightness(connector, level);
> pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> panel->backlight.pwm_state.enabled = true;
> pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> @@ -1233,10 +1239,8 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)
>
> mutex_lock(&dev_priv->backlight_lock);
>
> - if (panel->backlight.enabled) {
> + if (panel->backlight.enabled)
> val = panel->backlight.get(connector);
> - val = intel_panel_compute_brightness(connector, val);
> - }
>
> mutex_unlock(&dev_priv->backlight_lock);
>
> @@ -1567,13 +1571,13 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector)
> u16 pwm_freq_hz = get_vbt_pwm_freq(dev_priv);
> u32 pwm;
>
> - if (!panel->backlight.hz_to_pwm) {
> + if (!panel->backlight.pwm_funcs.hz_to_pwm) {
> drm_dbg_kms(&dev_priv->drm,
> "backlight frequency conversion not supported\n");
> return 0;
> }
>
> - pwm = panel->backlight.hz_to_pwm(connector, pwm_freq_hz);
> + pwm = panel->backlight.pwm_funcs.hz_to_pwm(connector, pwm_freq_hz);
> if (!pwm) {
> drm_dbg_kms(&dev_priv->drm,
> "backlight frequency conversion failed\n");
> @@ -1592,7 +1596,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
> struct intel_panel *panel = &connector->panel;
> int min;
>
> - drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
> + drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
>
> /*
> * XXX: If the vbt value is 255, it makes min equal to max, which leads
> @@ -1609,7 +1613,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
> }
>
> /* vbt value is a coefficient in range [0..255] */
> - return scale(min, 0, 255, 0, panel->backlight.max);
> + return scale(min, 0, 255, 0, panel->backlight.pwm_max);
> }
>
> static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unused)
> @@ -1629,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_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_max)
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> + panel->backlight.pwm_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));
> +
> 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);
> intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
> pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
>
> @@ -1674,29 +1673,24 @@ static int pch_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 cpu_ctl2, pch_ctl1, pch_ctl2, val;
> + u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>
> pch_ctl1 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL1);
> 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_max = pch_ctl2 >> 16;
>
> - if (!panel->backlight.max)
> - panel->backlight.max = get_backlight_max_vbt(connector);
> + if (!panel->backlight.pwm_max)
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> -
> - val = pch_get_backlight(connector);
> - val = intel_panel_compute_brightness(connector, val);
> - panel->backlight.level = clamp(val, panel->backlight.min,
> - panel->backlight.max);
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
> - panel->backlight.enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
> + panel->backlight.pwm_enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
> (pch_ctl1 & BLM_PCH_PWM_ENABLE);
>
> return 0;
> @@ -1716,27 +1710,26 @@ static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unu
> if (IS_PINEVIEW(dev_priv))
> panel->backlight.active_low_pwm = ctl & BLM_POLARITY_PNV;
>
> - panel->backlight.max = ctl >> 17;
> + panel->backlight.pwm_max = ctl >> 17;
>
> - if (!panel->backlight.max) {
> - panel->backlight.max = get_backlight_max_vbt(connector);
> - panel->backlight.max >>= 1;
> + if (!panel->backlight.pwm_max) {
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
> + panel->backlight.pwm_max >>= 1;
> }
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> if (panel->backlight.combination_mode)
> - panel->backlight.max *= 0xff;
> + panel->backlight.pwm_max *= 0xff;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> val = i9xx_get_backlight(connector);
> - val = intel_panel_compute_brightness(connector, val);
> - panel->backlight.level = clamp(val, panel->backlight.min,
> - panel->backlight.max);
> + val = intel_panel_sanitize_pwm_level(connector, val);
> + val = clamp(val, panel->backlight.pwm_min, panel->backlight.pwm_max);
>
> - panel->backlight.enabled = val != 0;
> + panel->backlight.pwm_enabled = val != 0;
>
> return 0;
> }
> @@ -1745,32 +1738,27 @@ static int i965_setup_backlight(struct intel_connector *connector, enum pipe unu
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 ctl, ctl2, val;
> + u32 ctl, ctl2;
>
> ctl2 = intel_de_read(dev_priv, BLC_PWM_CTL2);
> panel->backlight.combination_mode = ctl2 & BLM_COMBINATION_MODE;
> panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
>
> ctl = intel_de_read(dev_priv, BLC_PWM_CTL);
> - panel->backlight.max = ctl >> 16;
> + panel->backlight.pwm_max = ctl >> 16;
>
> - if (!panel->backlight.max)
> - panel->backlight.max = get_backlight_max_vbt(connector);
> + if (!panel->backlight.pwm_max)
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> if (panel->backlight.combination_mode)
> - panel->backlight.max *= 0xff;
> + panel->backlight.pwm_max *= 0xff;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> - val = i9xx_get_backlight(connector);
> - val = intel_panel_compute_brightness(connector, val);
> - panel->backlight.level = clamp(val, panel->backlight.min,
> - panel->backlight.max);
> -
> - panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
> + panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
>
> return 0;
> }
> @@ -1779,7 +1767,7 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 ctl, ctl2, val;
> + u32 ctl, ctl2;
>
> if (drm_WARN_ON(&dev_priv->drm, pipe != PIPE_A && pipe != PIPE_B))
> return -ENODEV;
> @@ -1788,22 +1776,17 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
> panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
>
> ctl = intel_de_read(dev_priv, VLV_BLC_PWM_CTL(pipe));
> - panel->backlight.max = ctl >> 16;
> + panel->backlight.pwm_max = ctl >> 16;
>
> - if (!panel->backlight.max)
> - panel->backlight.max = get_backlight_max_vbt(connector);
> + if (!panel->backlight.pwm_max)
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> - val = _vlv_get_backlight(dev_priv, pipe);
> - val = intel_panel_compute_brightness(connector, val);
> - panel->backlight.level = clamp(val, panel->backlight.min,
> - panel->backlight.max);
> -
> - panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
> + panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
>
> return 0;
> }
> @@ -1828,24 +1811,18 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
> }
>
> panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
> - panel->backlight.max =
> - intel_de_read(dev_priv,
> - BXT_BLC_PWM_FREQ(panel->backlight.controller));
> + panel->backlight.pwm_max = intel_de_read(dev_priv,
> + BXT_BLC_PWM_FREQ(panel->backlight.controller));
>
> - if (!panel->backlight.max)
> - panel->backlight.max = get_backlight_max_vbt(connector);
> + if (!panel->backlight.pwm_max)
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> -
> - val = bxt_get_backlight(connector);
> - val = intel_panel_compute_brightness(connector, val);
> - panel->backlight.level = clamp(val, panel->backlight.min,
> - panel->backlight.max);
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> - panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
> + panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
>
> return 0;
> }
> @@ -1855,7 +1832,7 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 pwm_ctl, val;
> + u32 pwm_ctl;
>
> /*
> * CNP has the BXT implementation of backlight, but with only one
> @@ -1868,24 +1845,18 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
> BXT_BLC_PWM_CTL(panel->backlight.controller));
>
> panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
> - panel->backlight.max =
> - intel_de_read(dev_priv,
> - BXT_BLC_PWM_FREQ(panel->backlight.controller));
> + panel->backlight.pwm_max = intel_de_read(dev_priv,
> + BXT_BLC_PWM_FREQ(panel->backlight.controller));
>
> - if (!panel->backlight.max)
> - panel->backlight.max = get_backlight_max_vbt(connector);
> + if (!panel->backlight.pwm_max)
> + panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>
> - if (!panel->backlight.max)
> + if (!panel->backlight.pwm_max)
> return -ENODEV;
>
> - panel->backlight.min = get_backlight_min_vbt(connector);
> -
> - val = bxt_get_backlight(connector);
> - val = intel_panel_compute_brightness(connector, val);
> - panel->backlight.level = clamp(val, panel->backlight.min,
> - panel->backlight.max);
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> - panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
> + panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
>
> return 0;
> }
> @@ -1915,8 +1886,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
> return -ENODEV;
> }
>
> - panel->backlight.max = 100; /* 100% */
> - panel->backlight.min = get_backlight_min_vbt(connector);
> + panel->backlight.pwm_max = 100; /* 100% */
> + panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>
> if (pwm_is_enabled(panel->backlight.pwm)) {
> /* PWM is already enabled, use existing settings */
> @@ -1924,10 +1895,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
>
> level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
> 100);
> - level = intel_panel_compute_brightness(connector, level);
> - panel->backlight.level = clamp(level, panel->backlight.min,
> - panel->backlight.max);
> - panel->backlight.enabled = true;
> + level = intel_panel_sanitize_pwm_level(connector, level);
> + panel->backlight.pwm_enabled = true;
>
> drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n",
> NSEC_PER_SEC / (unsigned long)panel->backlight.pwm_state.period,
> @@ -1943,6 +1912,60 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
> return 0;
> }
>
> +static void intel_pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct intel_panel *panel = &connector->panel;
> +
> + panel->backlight.pwm_funcs.set(conn_state,
> + intel_panel_sanitize_pwm_level(connector, level));
> +}
> +
> +static u32 intel_pwm_get_backlight(struct intel_connector *connector)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + return intel_panel_sanitize_pwm_level(connector, panel->backlight.pwm_funcs.get(connector));
> +}
> +
> +static void intel_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct intel_panel *panel = &connector->panel;
> + u32 level = intel_panel_sanitize_pwm_level(connector, panel->backlight.level);
> +
> + panel->backlight.pwm_funcs.enable(crtc_state, conn_state, level);
> +}
> +
> +static void intel_pwm_disable_backlight(const struct drm_connector_state *conn_state)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct intel_panel *panel = &connector->panel;
> +
> + panel->backlight.pwm_funcs.disable(conn_state,
> + intel_panel_sanitize_pwm_level(connector, 0));
> +}
> +
> +/* The only hook PWM-only backlight setups need to override, the rest of the hooks are filled in
> + * from pwm_funcs
> + */
> +static int intel_pwm_setup_backlight(struct intel_connector *connector, enum pipe pipe)
> +{
> + struct intel_panel *panel = &connector->panel;
> + int ret = panel->backlight.pwm_funcs.setup(connector, pipe);
> +
> + if (ret < 0)
> + return ret;
> +
> + panel->backlight.min = panel->backlight.pwm_min;
> + panel->backlight.max = panel->backlight.pwm_max;
> + panel->backlight.level = panel->backlight.get(connector);
> + panel->backlight.enabled = panel->backlight.pwm_enabled;
> +
> + return 0;
> +}
> +
> void intel_panel_update_backlight(struct intel_atomic_state *state,
> struct intel_encoder *encoder,
> const struct intel_crtc_state *crtc_state,
> @@ -2024,75 +2047,82 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> container_of(panel, struct intel_connector, panel);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>
> - if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
> - intel_dp_aux_init_backlight_funcs(connector) == 0)
> - return;
> -
> if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI &&
> intel_dsi_dcs_init_backlight_funcs(connector) == 0)
> return;
>
> if (IS_GEN9_LP(dev_priv)) {
> - panel->backlight.setup = bxt_setup_backlight;
> - panel->backlight.enable = bxt_enable_backlight;
> - panel->backlight.disable = bxt_disable_backlight;
> - panel->backlight.set = bxt_set_backlight;
> - panel->backlight.get = bxt_get_backlight;
> - panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
> + panel->backlight.pwm_funcs.setup = bxt_setup_backlight;
> + panel->backlight.pwm_funcs.enable = bxt_enable_backlight;
> + panel->backlight.pwm_funcs.disable = bxt_disable_backlight;
> + panel->backlight.pwm_funcs.set = bxt_set_backlight;
> + panel->backlight.pwm_funcs.get = bxt_get_backlight;
> + panel->backlight.pwm_funcs.hz_to_pwm = bxt_hz_to_pwm;
> } else if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
> - panel->backlight.setup = cnp_setup_backlight;
> - panel->backlight.enable = cnp_enable_backlight;
> - panel->backlight.disable = cnp_disable_backlight;
> - panel->backlight.set = bxt_set_backlight;
> - panel->backlight.get = bxt_get_backlight;
> - panel->backlight.hz_to_pwm = cnp_hz_to_pwm;
> + panel->backlight.pwm_funcs.setup = cnp_setup_backlight;
> + panel->backlight.pwm_funcs.enable = cnp_enable_backlight;
> + panel->backlight.pwm_funcs.disable = cnp_disable_backlight;
> + panel->backlight.pwm_funcs.set = bxt_set_backlight;
> + panel->backlight.pwm_funcs.get = bxt_get_backlight;
> + panel->backlight.pwm_funcs.hz_to_pwm = cnp_hz_to_pwm;
> } else if (INTEL_PCH_TYPE(dev_priv) >= PCH_LPT) {
> - panel->backlight.setup = lpt_setup_backlight;
> - panel->backlight.enable = lpt_enable_backlight;
> - panel->backlight.disable = lpt_disable_backlight;
> - panel->backlight.set = lpt_set_backlight;
> - panel->backlight.get = lpt_get_backlight;
> + panel->backlight.pwm_funcs.setup = lpt_setup_backlight;
> + panel->backlight.pwm_funcs.enable = lpt_enable_backlight;
> + panel->backlight.pwm_funcs.disable = lpt_disable_backlight;
> + panel->backlight.pwm_funcs.set = lpt_set_backlight;
> + panel->backlight.pwm_funcs.get = lpt_get_backlight;
> if (HAS_PCH_LPT(dev_priv))
> - panel->backlight.hz_to_pwm = lpt_hz_to_pwm;
> + panel->backlight.pwm_funcs.hz_to_pwm = lpt_hz_to_pwm;
> else
> - panel->backlight.hz_to_pwm = spt_hz_to_pwm;
> + panel->backlight.pwm_funcs.hz_to_pwm = spt_hz_to_pwm;
> } else if (HAS_PCH_SPLIT(dev_priv)) {
> - panel->backlight.setup = pch_setup_backlight;
> - panel->backlight.enable = pch_enable_backlight;
> - panel->backlight.disable = pch_disable_backlight;
> - panel->backlight.set = pch_set_backlight;
> - panel->backlight.get = pch_get_backlight;
> - panel->backlight.hz_to_pwm = pch_hz_to_pwm;
> + panel->backlight.pwm_funcs.setup = pch_setup_backlight;
> + panel->backlight.pwm_funcs.enable = pch_enable_backlight;
> + panel->backlight.pwm_funcs.disable = pch_disable_backlight;
> + panel->backlight.pwm_funcs.set = pch_set_backlight;
> + panel->backlight.pwm_funcs.get = pch_get_backlight;
> + panel->backlight.pwm_funcs.hz_to_pwm = pch_hz_to_pwm;
> } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
> - panel->backlight.setup = ext_pwm_setup_backlight;
> - panel->backlight.enable = ext_pwm_enable_backlight;
> - panel->backlight.disable = ext_pwm_disable_backlight;
> - panel->backlight.set = ext_pwm_set_backlight;
> - panel->backlight.get = ext_pwm_get_backlight;
> + panel->backlight.pwm_funcs.setup = ext_pwm_setup_backlight;
> + panel->backlight.pwm_funcs.enable = ext_pwm_enable_backlight;
> + panel->backlight.pwm_funcs.disable = ext_pwm_disable_backlight;
> + panel->backlight.pwm_funcs.set = ext_pwm_set_backlight;
> + panel->backlight.pwm_funcs.get = ext_pwm_get_backlight;
> } else {
> - panel->backlight.setup = vlv_setup_backlight;
> - panel->backlight.enable = vlv_enable_backlight;
> - panel->backlight.disable = vlv_disable_backlight;
> - panel->backlight.set = vlv_set_backlight;
> - panel->backlight.get = vlv_get_backlight;
> - panel->backlight.hz_to_pwm = vlv_hz_to_pwm;
> + panel->backlight.pwm_funcs.setup = vlv_setup_backlight;
> + panel->backlight.pwm_funcs.enable = vlv_enable_backlight;
> + panel->backlight.pwm_funcs.disable = vlv_disable_backlight;
> + panel->backlight.pwm_funcs.set = vlv_set_backlight;
> + panel->backlight.pwm_funcs.get = vlv_get_backlight;
> + panel->backlight.pwm_funcs.hz_to_pwm = vlv_hz_to_pwm;
> }
> } else if (IS_GEN(dev_priv, 4)) {
> - panel->backlight.setup = i965_setup_backlight;
> - panel->backlight.enable = i965_enable_backlight;
> - panel->backlight.disable = i965_disable_backlight;
> - panel->backlight.set = i9xx_set_backlight;
> - panel->backlight.get = i9xx_get_backlight;
> - panel->backlight.hz_to_pwm = i965_hz_to_pwm;
> + panel->backlight.pwm_funcs.setup = i965_setup_backlight;
> + panel->backlight.pwm_funcs.enable = i965_enable_backlight;
> + panel->backlight.pwm_funcs.disable = i965_disable_backlight;
> + panel->backlight.pwm_funcs.set = i9xx_set_backlight;
> + panel->backlight.pwm_funcs.get = i9xx_get_backlight;
> + panel->backlight.pwm_funcs.hz_to_pwm = i965_hz_to_pwm;
> } else {
> - panel->backlight.setup = i9xx_setup_backlight;
> - panel->backlight.enable = i9xx_enable_backlight;
> - panel->backlight.disable = i9xx_disable_backlight;
> - panel->backlight.set = i9xx_set_backlight;
> - panel->backlight.get = i9xx_get_backlight;
> - panel->backlight.hz_to_pwm = i9xx_hz_to_pwm;
> + panel->backlight.pwm_funcs.setup = i9xx_setup_backlight;
> + panel->backlight.pwm_funcs.enable = i9xx_enable_backlight;
> + panel->backlight.pwm_funcs.disable = i9xx_disable_backlight;
> + panel->backlight.pwm_funcs.set = i9xx_set_backlight;
> + panel->backlight.pwm_funcs.get = i9xx_get_backlight;
> + panel->backlight.pwm_funcs.hz_to_pwm = i9xx_hz_to_pwm;
> }
> +
> + if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
> + intel_dp_aux_init_backlight_funcs(connector) == 0)
> + return;
> +
> + /* We're using a standard PWM backlight interface */
> + panel->backlight.setup = intel_pwm_setup_backlight;
> + panel->backlight.enable = intel_pwm_enable_backlight;
> + panel->backlight.disable = intel_pwm_disable_backlight;
> + panel->backlight.set = intel_pwm_set_backlight;
> + panel->backlight.get = intel_pwm_get_backlight;
> }
>
> enum drm_connector_status
> --
> 2.26.2
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

2020-11-26 08:34:12

by Dave Airlie

Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

On Thu, 17 Sept 2020 at 03:19, Lyude Paul <[email protected]> wrote:
>
> Currently, every different type of backlight hook that i915 supports is
> pretty straight forward - you have a backlight, probably through PWM
> (but maybe DPCD), with a single set of platform-specific hooks that are
> used for controlling it.
>
> HDR backlights, in particular VESA and Intel's HDR backlight
> implementations, can end up being more complicated. With Intel's
> proprietary interface, HDR backlight controls always run through the
> DPCD. When the backlight is in SDR backlight mode however, the driver
> may need to bypass the TCON and control the backlight directly through
> PWM.
>
> So, in order to support this we'll need to split our backlight callbacks
> into two groups: a set of high-level backlight control callbacks in
> intel_panel, and an additional set of pwm-specific backlight control
> callbacks. This also implies a functional changes for how these
> callbacks are used:
>
> * We now keep track of two separate backlight level ranges, one for the
> high-level backlight, and one for the pwm backlight range
> * We also keep track of backlight enablement and PWM backlight
> enablement separately
> * Since the currently set backlight level might not be the same as the
> currently programmed PWM backlight level, we stop setting
> panel->backlight.level with the currently programmed PWM backlight
> level in panel->backlight.pwm_funcs.setup(). Instead, we rely
> on the higher level backlight control functions to retrieve the
> current PWM backlight level (in this case, intel_pwm_get_backlight()).
> Note that there are still a few PWM backlight setup callbacks that
> do actually need to retrieve the current PWM backlight level, although
> we no longer save this value in panel->backlight.level like before.
> * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
> brightness level, unlike their siblings
> panel->backlight.enable()/disable(). This is so we can calculate the
> actual PWM brightness level we want to set on disable/enable in the
> higher level backlight enable()/disable() functions, since this value
> might be scaled from a brightness level that doesn't come from PWM.

Oh this patch is a handful, I can see why people stall out here.

I'm going to be annoying maintainer and see if you can clean this up a
bit in advance
of this patch.

1) move the callbacks out of struct intel_panel.backlight into a separate struct
and use const static object tables, having fn ptrs and data co-located
in a struct
isn't great.

strcut intel_panel_backlight_funcs {

};
struct intel_panel {
struct {
struct intel_panel_backlight_funcs *funcs;
};
};

type of thing.

I think you could reuse the backlight funcs struct for the pwm stuff
as well. (maybe with an assert on hz_to_pwm for the old hooks).

2) change the apis to pass 0 down in a separate patch, this modifies a
bunch of apis to pass in an extra level parameter, do that
first in a separate patch that doesn't change anything but hands 0
down the chain. Then switch over in another patch.

3) One comment in passing below.
>
>
> - 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));
> +
> 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);
> intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
> pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
>
The change here confused me since it no longer calls lpt_get_backlight
in this path, the commit msg might explain this, but it didn't explain
is so I could figure out if that was a mistake or intentional.

Dave.

2020-11-26 12:18:59

by Jani Nikula

Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

On Thu, 26 Nov 2020, Dave Airlie <[email protected]> wrote:
> On Thu, 17 Sept 2020 at 03:19, Lyude Paul <[email protected]> wrote:
>>
>> Currently, every different type of backlight hook that i915 supports is
>> pretty straight forward - you have a backlight, probably through PWM
>> (but maybe DPCD), with a single set of platform-specific hooks that are
>> used for controlling it.
>>
>> HDR backlights, in particular VESA and Intel's HDR backlight
>> implementations, can end up being more complicated. With Intel's
>> proprietary interface, HDR backlight controls always run through the
>> DPCD. When the backlight is in SDR backlight mode however, the driver
>> may need to bypass the TCON and control the backlight directly through
>> PWM.
>>
>> So, in order to support this we'll need to split our backlight callbacks
>> into two groups: a set of high-level backlight control callbacks in
>> intel_panel, and an additional set of pwm-specific backlight control
>> callbacks. This also implies a functional changes for how these
>> callbacks are used:
>>
>> * We now keep track of two separate backlight level ranges, one for the
>> high-level backlight, and one for the pwm backlight range
>> * We also keep track of backlight enablement and PWM backlight
>> enablement separately
>> * Since the currently set backlight level might not be the same as the
>> currently programmed PWM backlight level, we stop setting
>> panel->backlight.level with the currently programmed PWM backlight
>> level in panel->backlight.pwm_funcs.setup(). Instead, we rely
>> on the higher level backlight control functions to retrieve the
>> current PWM backlight level (in this case, intel_pwm_get_backlight()).
>> Note that there are still a few PWM backlight setup callbacks that
>> do actually need to retrieve the current PWM backlight level, although
>> we no longer save this value in panel->backlight.level like before.
>> * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
>> brightness level, unlike their siblings
>> panel->backlight.enable()/disable(). This is so we can calculate the
>> actual PWM brightness level we want to set on disable/enable in the
>> higher level backlight enable()/disable() functions, since this value
>> might be scaled from a brightness level that doesn't come from PWM.
>
> Oh this patch is a handful, I can see why people stall out here.
>
> I'm going to be annoying maintainer and see if you can clean this up a
> bit in advance of this patch.

Agreed. And not looking into and requesting this earlier is on me.

The thing that still keeps bugging me about the DPCD brightness control
in general is that it's a historical mistake to put all of this under
i915. (Again, mea culpa.) The standard DPCD brightness control should
really be under drm core, in one form or another.

I'm not asking to fix that here. But I *am* wondering if the series
makes that harder. What would it look like if we did have that unicorn
of a brightness connector property? How would that tie into the hooks we
have?

Maybe the answer is that the DPCD backlight functions should just be
library code in drm core that the drivers could call. In the long run,
i915 really can't be the only one needing this stuff.

We haven't implemented the mixed modes of DPCD and eDP PWM pin
brightness control. But the point is, the library code can't call into
i915 specific eDP PWM pin control code. The proprietary HDR brightness
code will still be i915 specific, but does it make sense to have a mixed
mode there that will be completely different from what a mixed mode with
the standard VESA DPCD brightness could be?

I.e. what should be the entry points for the hooks, and who calls what?

BR,
Jani.

>
> 1) move the callbacks out of struct intel_panel.backlight into a separate struct
> and use const static object tables, having fn ptrs and data co-located
> in a struct
> isn't great.
>
> strcut intel_panel_backlight_funcs {
>
> };
> struct intel_panel {
> struct {
> struct intel_panel_backlight_funcs *funcs;
> };
> };
>
> type of thing.
>
> I think you could reuse the backlight funcs struct for the pwm stuff
> as well. (maybe with an assert on hz_to_pwm for the old hooks).
>
> 2) change the apis to pass 0 down in a separate patch, this modifies a
> bunch of apis to pass in an extra level parameter, do that
> first in a separate patch that doesn't change anything but hands 0
> down the chain. Then switch over in another patch.
>
> 3) One comment in passing below.
>>
>>
>> - 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));
>> +
>> 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);
>> intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
>> pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
>>
> The change here confused me since it no longer calls lpt_get_backlight
> in this path, the commit msg might explain this, but it didn't explain
> is so I could figure out if that was a mistake or intentional.
>
> Dave.
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Jani Nikula, Intel Open Source Graphics Center

2020-12-01 02:40:30

by Lyude Paul

Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

On Thu, 2020-11-26 at 11:03 +1000, Dave Airlie wrote:
> On Thu, 17 Sept 2020 at 03:19, Lyude Paul <[email protected]> wrote:
> >
> > Currently, every different type of backlight hook that i915 supports is
> > pretty straight forward - you have a backlight, probably through PWM
> > (but maybe DPCD), with a single set of platform-specific hooks that are
> > used for controlling it.
> >
> > HDR backlights, in particular VESA and Intel's HDR backlight
> > implementations, can end up being more complicated. With Intel's
> > proprietary interface, HDR backlight controls always run through the
> > DPCD. When the backlight is in SDR backlight mode however, the driver
> > may need to bypass the TCON and control the backlight directly through
> > PWM.
> >
> > So, in order to support this we'll need to split our backlight callbacks
> > into two groups: a set of high-level backlight control callbacks in
> > intel_panel, and an additional set of pwm-specific backlight control
> > callbacks. This also implies a functional changes for how these
> > callbacks are used:
> >
> > * We now keep track of two separate backlight level ranges, one for the
> >   high-level backlight, and one for the pwm backlight range
> > * We also keep track of backlight enablement and PWM backlight
> >   enablement separately
> > * Since the currently set backlight level might not be the same as the
> >   currently programmed PWM backlight level, we stop setting
> >   panel->backlight.level with the currently programmed PWM backlight
> >   level in panel->backlight.pwm_funcs.setup(). Instead, we rely
> >   on the higher level backlight control functions to retrieve the
> >   current PWM backlight level (in this case, intel_pwm_get_backlight()).
> >   Note that there are still a few PWM backlight setup callbacks that
> >   do actually need to retrieve the current PWM backlight level, although
> >   we no longer save this value in panel->backlight.level like before.
> > * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
> >   brightness level, unlike their siblings
> >   panel->backlight.enable()/disable(). This is so we can calculate the
> >   actual PWM brightness level we want to set on disable/enable in the
> >   higher level backlight enable()/disable() functions, since this value
> >   might be scaled from a brightness level that doesn't come from PWM.
>
> Oh this patch is a handful, I can see why people stall out here.
>
> I'm going to be annoying maintainer and see if you can clean this up a
> bit in advance
> of this patch.
>

Not annoying at all :), I was hoping there'd be a good bit of criticism on
this patch series since it's been hard to figure out if I'm even implementing
things in the right way or not (especially because I really don't know what
the HDR side of this is going to look like, although I assume it's probably
going to be pretty hands-off in the kernel).

JFYI too for folks on the list, any suggestions about the HDR side of this are
super appreciated. I'm barely familiar with such things.

> 1) move the callbacks out of struct intel_panel.backlight into a separate
> struct
> and use const static object tables, having fn ptrs and data co-located
> in a struct
> isn't great.
>
> strcut intel_panel_backlight_funcs {
>
> };
> struct intel_panel {
>     struct {
>         struct intel_panel_backlight_funcs *funcs;
>     };
> };
>
> type of thing.
>
> I think you could reuse the backlight funcs struct for the pwm stuff
> as well. (maybe with an assert on hz_to_pwm for the old hooks).
>
> 2) change the apis to pass 0 down in a separate patch, this modifies a
> bunch of apis to pass in an extra level parameter, do that
> first in a separate patch that doesn't change anything but hands 0
> down the chain. Then switch over in another patch.
>
> 3) One comment in passing below.
> >
> >
> > -       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));
> > +
> >                 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);
> >                 intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
> >                                pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
> >
> The change here confused me since it no longer calls lpt_get_backlight
> in this path, the commit msg might explain this, but it didn't explain
> is so I could figure out if that was a mistake or intentional.

Will address these in the next respin, thanks for the review!

>
> Dave.
>

--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat

2020-12-01 02:45:49

by Lyude Paul

Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

On Thu, 2020-11-26 at 13:57 +0200, Jani Nikula wrote:
> On Thu, 26 Nov 2020, Dave Airlie <[email protected]> wrote:
> > On Thu, 17 Sept 2020 at 03:19, Lyude Paul <[email protected]> wrote:
> > >
> > > Currently, every different type of backlight hook that i915 supports is
> > > pretty straight forward - you have a backlight, probably through PWM
> > > (but maybe DPCD), with a single set of platform-specific hooks that are
> > > used for controlling it.
> > >
> > > HDR backlights, in particular VESA and Intel's HDR backlight
> > > implementations, can end up being more complicated. With Intel's
> > > proprietary interface, HDR backlight controls always run through the
> > > DPCD. When the backlight is in SDR backlight mode however, the driver
> > > may need to bypass the TCON and control the backlight directly through
> > > PWM.
> > >
> > > So, in order to support this we'll need to split our backlight callbacks
> > > into two groups: a set of high-level backlight control callbacks in
> > > intel_panel, and an additional set of pwm-specific backlight control
> > > callbacks. This also implies a functional changes for how these
> > > callbacks are used:
> > >
> > > * We now keep track of two separate backlight level ranges, one for the
> > >   high-level backlight, and one for the pwm backlight range
> > > * We also keep track of backlight enablement and PWM backlight
> > >   enablement separately
> > > * Since the currently set backlight level might not be the same as the
> > >   currently programmed PWM backlight level, we stop setting
> > >   panel->backlight.level with the currently programmed PWM backlight
> > >   level in panel->backlight.pwm_funcs.setup(). Instead, we rely
> > >   on the higher level backlight control functions to retrieve the
> > >   current PWM backlight level (in this case, intel_pwm_get_backlight()).
> > >   Note that there are still a few PWM backlight setup callbacks that
> > >   do actually need to retrieve the current PWM backlight level, although
> > >   we no longer save this value in panel->backlight.level like before.
> > > * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
> > >   brightness level, unlike their siblings
> > >   panel->backlight.enable()/disable(). This is so we can calculate the
> > >   actual PWM brightness level we want to set on disable/enable in the
> > >   higher level backlight enable()/disable() functions, since this value
> > >   might be scaled from a brightness level that doesn't come from PWM.
> >
> > Oh this patch is a handful, I can see why people stall out here.
> >
> > I'm going to be annoying maintainer and see if you can clean this up a
> > bit in advance of this patch.
>
> Agreed. And not looking into and requesting this earlier is on me.
>
> The thing that still keeps bugging me about the DPCD brightness control
> in general is that it's a historical mistake to put all of this under
> i915. (Again, mea culpa.) The standard DPCD brightness control should
> really be under drm core, in one form or another.

JFYI - I already actually have a WIP series to move all of the VESA standard
brightness stuff into the DRM core (especially since I am adding support for
the VESA interface to nouveau). It is pretty important to do so especially
considering some of the ways panel manufacturers seem to have consistently
gotten some portions of the spec wrong (there's currently a bug on almost
every panel I've run into, minus some panels in laptops that run ChromeOS,
where they interpret the brightness value as LSB aligned and not MSB aligned
(which is what the eDP spec actually says), because almost everyone misread
it. So, definitely the kind of stuff we'd want to keep in the drm core to make
maintaining quirks like this easier.

>
> I'm not asking to fix that here. But I *am* wondering if the series
> makes that harder. What would it look like if we did have that unicorn
> of a brightness connector property? How would that tie into the hooks we
> have?

Re: making it harder, not really. But either way I'm planning on doing the
work for this anyway :)

>
> Maybe the answer is that the DPCD backlight functions should just be
> library code in drm core that the drivers could call. In the long run,
> i915 really can't be the only one needing this stuff.
>
> We haven't implemented the mixed modes of DPCD and eDP PWM pin
> brightness control. But the point is, the library code can't call into
> i915 specific eDP PWM pin control code. The proprietary HDR brightness
> code will still be i915 specific, but does it make sense to have a mixed
> mode there that will be completely different from what a mixed mode with
> the standard VESA DPCD brightness could be?
>
> I.e. what should be the entry points for the hooks, and who calls what?

I think i915 is actually exactly where we want the hooks for this particular
backlight interface, mostly because amdgpu already had to implement their own
backlight control interface for similar reasons to Intel. From what I've seen,
the interfaces which a panel supports backlight control through tend to be
tightly tied to the hardware those panels usually accompany (at least in the
x86 world, no idea about elsewhere). For example, I've only seen laptops which
have no support/broken support for the VESA backlight interface, but not
Intel's, on hardware which only had an Intel GPU present. Every single laptop
I've tested with a Intel/Nvidia GPU setup (where the Nvidia GPU could drive the
eDP display, no idea what the situation is like elsewhere) seems to support both
interfaces perfectly. So, I don't think we'll ever see any need for this outside
of i915 at least.

Also in specific regards to the pwm control: I'm actually planning on keeping
that out of the DRM core libraries, because some variants of the VESA interface
actually need to be able to call down to PWM driver functions as well. Thus, I
think the only way of coming up with helpers for this that make sense is to only
add helpers for the DPCD related portions of backlight control that are going to
apply to all drivers. So far the only spot where we ask the driver for PWM
related info is during eDP backlight probing, where we can use it to calculate
the number of supported backlight levels, but this is entirely optional for the
driver to support it.

JFYI, here's a WIP of that:

https://gitlab.freedesktop.org/lyudess/linux/-/commit/a4bbe0d5ad980c12eb776e59a1bd522d74d09006

>
> BR,
> Jani.
>
> >
> > 1) move the callbacks out of struct intel_panel.backlight into a separate
> > struct
> > and use const static object tables, having fn ptrs and data co-located
> > in a struct
> > isn't great.
> >
> > strcut intel_panel_backlight_funcs {
> >
> > };
> > struct intel_panel {
> >     struct {
> >         struct intel_panel_backlight_funcs *funcs;
> >     };
> > };
> >
> > type of thing.
> >
> > I think you could reuse the backlight funcs struct for the pwm stuff
> > as well. (maybe with an assert on hz_to_pwm for the old hooks).
> >
> > 2) change the apis to pass 0 down in a separate patch, this modifies a
> > bunch of apis to pass in an extra level parameter, do that
> > first in a separate patch that doesn't change anything but hands 0
> > down the chain. Then switch over in another patch.
> >
> > 3) One comment in passing below.
> > >
> > >
> > > -       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));
> > > +
> > >                 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);
> > >                 intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
> > >                                pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
> > >
> > The change here confused me since it no longer calls lpt_get_backlight
> > in this path, the commit msg might explain this, but it didn't explain
> > is so I could figure out if that was a mistake or intentional.
> >
> > Dave.
> > _______________________________________________
> > Intel-gfx mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat