[RFC v3 01/10] drm/nouveau/kms/nv40-/backlight: Assign prop type once

[RFC v3 01/10] drm/nouveau/kms/nv40-/backlight: Assign prop type once

[Lyude Paul]

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@gmail.com>
Cc: greg.depoire@gmail.com
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 72f35a2babcb..42b498e7e2bf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -106,7 +106,6 @@ nv40_backlight_init(struct nouveau_encoder *encoder,
 	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
 		return -ENODEV;
 
-	props->type = BACKLIGHT_RAW;
 	props->max_brightness = 31;
 	*ops = &nv40_bl_ops;
 	return 0;
@@ -212,7 +211,6 @@ nv50_backlight_init(struct nouveau_encoder *nv_encoder,
 	else
 		*ops = &nva3_bl_ops;
 
-	props->type = BACKLIGHT_RAW;
 	props->max_brightness = 100;
 
 	return 0;
@@ -226,7 +224,7 @@ nouveau_backlight_init(struct drm_connector *connector)
 	struct nouveau_encoder *nv_encoder = NULL;
 	struct nvif_device *device = &drm->client.device;
 	char backlight_name[BL_NAME_SIZE];
-	struct backlight_properties props = {0};
+	struct backlight_properties props = { .type = BACKLIGHT_RAW, 0 };
 	const struct backlight_ops *ops;
 	int ret;
 
-- 
2.29.2

[RFC v3 02/10] drm/nouveau/kms: Don't probe eDP connectors more then once

[Lyude Paul]

eDP doesn't do hotplugging, so there's no reason for us to reprobe it (unless a
connection status change is being forced, of course).

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@gmail.com>
Cc: greg.depoire@gmail.com
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 61e6d7412505..55e986b3cad9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -556,6 +556,12 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
 	int ret;
 	enum drm_connector_status conn_status = connector_status_disconnected;
 
+	/* eDP doesn't do hotplugging, never probe more then once */
+	if (nv_connector->type == DCB_CONNECTOR_eDP &&
+	    connector->force == DRM_FORCE_UNSPECIFIED &&
+	    connector->status != connector_status_unknown)
+		return connector->status;
+
 	/* Outputs are only polled while runtime active, so resuming the
 	 * device here is unnecessary (and would deadlock upon runtime suspend
 	 * because it waits for polling to finish). We do however, want to
-- 
2.29.2

[RFC v3 03/10] drm/i915/dpcd_bl: Remove redundant AUX backlight frequency calculations

[Lyude Paul]

Noticed this while moving all of the VESA backlight code in i915 over to
DRM helpers: it would appear that we calculate the frequency value we want
to write to DP_EDP_BACKLIGHT_FREQ_SET twice even though this value never
actually changes during runtime. So, let's simplify things by just caching
this value in intel_panel.backlight, and re-writing it as-needed.

Changes since v1:
* Wrap panel->backlight.edp.vesa.pwm_freq_pre_divider in
  DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP check - Jani

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@gmail.com>
Cc: greg.depoire@gmail.com
---
 .../drm/i915/display/intel_display_types.h    |  1 +
 .../drm/i915/display/intel_dp_aux_backlight.c | 65 ++++++-------------
 2 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 307ff4b771f4..f4b26e1dbaaf 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -264,6 +264,7 @@ struct intel_panel {
 		union {
 			struct {
 				u8 pwmgen_bit_count;
+				u8 pwm_freq_pre_divider;
 			} vesa;
 			struct {
 				bool sdr_uses_aux;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 651884390137..62294967f430 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -373,50 +373,6 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
 	}
 }
 
-/*
- * Set PWM Frequency divider to match desired frequency in vbt.
- * The PWM Frequency is calculated as 27Mhz / (F x P).
- * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
- *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
- * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
- *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
- */
-static bool intel_dp_aux_vesa_set_pwm_freq(struct intel_connector *connector)
-{
-	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	const u8 pn = connector->panel.backlight.edp.vesa.pwmgen_bit_count;
-	int freq, fxp, f, fxp_actual, fxp_min, fxp_max;
-
-	freq = dev_priv->vbt.backlight.pwm_freq_hz;
-	if (!freq) {
-		drm_dbg_kms(&dev_priv->drm,
-			    "Use panel default backlight frequency\n");
-		return false;
-	}
-
-	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
-	f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
-	fxp_actual = f << pn;
-
-	/* Ensure frequency is within 25% of desired value */
-	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
-	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
-
-	if (fxp_min > fxp_actual || fxp_actual > fxp_max) {
-		drm_dbg_kms(&dev_priv->drm, "Actual frequency out of range\n");
-		return false;
-	}
-
-	if (drm_dp_dpcd_writeb(&intel_dp->aux,
-			       DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
-		drm_dbg_kms(&dev_priv->drm,
-			    "Failed to write aux backlight freq\n");
-		return false;
-	}
-	return true;
-}
-
 static void
 intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state, u32 level)
@@ -459,9 +415,13 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 		break;
 	}
 
-	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
-		if (intel_dp_aux_vesa_set_pwm_freq(connector))
+	if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
+		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
+				       panel->backlight.edp.vesa.pwm_freq_pre_divider) == 1)
 			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+		else
+			drm_dbg_kms(&i915->drm, "Failed to write aux backlight frequency\n");
+	}
 
 	if (new_dpcd_buf != dpcd_buf) {
 		if (drm_dp_dpcd_writeb(&intel_dp->aux,
@@ -482,6 +442,14 @@ static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state
 				  false);
 }
 
+/*
+ * Compute PWM frequency divider value based off the frequency provided to us by the vbt.
+ * The PWM Frequency is calculated as 27Mhz / (F x P).
+ * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
+ *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
+ * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
+ *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
+ */
 static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connector)
 {
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
@@ -533,8 +501,10 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
 	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
 
+	/* Ensure frequency is within 25% of desired value */
 	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
 	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
+
 	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
 		drm_dbg_kms(&i915->drm,
 			    "VBT defined backlight frequency out of range\n");
@@ -555,7 +525,10 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 			    "Failed to write aux pwmgen bit count\n");
 		return max_backlight;
 	}
+
 	panel->backlight.edp.vesa.pwmgen_bit_count = pn;
+	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
+		panel->backlight.edp.vesa.pwm_freq_pre_divider = f;
 
 	max_backlight = (1 << pn) - 1;
 
-- 
2.29.2

[RFC v3 04/10] drm/i915/dpcd_bl: Handle drm_dpcd_read/write() return values correctly

[Lyude Paul]

This is kind of an annoying aspect of DRM's DP helpers:
drm_dp_dpcd_readb/writeb() return the size of bytes read/written on
success, thus we want to check against that instead of checking if the
return value is less than 0.

I'll probably be fixing this in the near future once I start doing DP work
again, also because I'd rather not mix a tree-wide refactor like that in
with a patch series intended to be around introducing DP backlight helpers.
So, for now let's just handle the return values from each function
correctly.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 41 +++++++++----------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 62294967f430..c37ccc8538cb 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -107,7 +107,7 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
 	u8 tcon_cap[4];
 
 	ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
-	if (ret < 0)
+	if (ret != sizeof(tcon_cap))
 		return false;
 
 	if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP))
@@ -137,7 +137,7 @@ intel_dp_aux_hdr_get_backlight(struct intel_connector *connector, enum pipe pipe
 	u8 tmp;
 	u8 buf[2] = { 0 };
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) < 0) {
+	if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) != 1) {
 		drm_err(&i915->drm, "Failed to read current backlight mode from DPCD\n");
 		return 0;
 	}
@@ -153,7 +153,8 @@ intel_dp_aux_hdr_get_backlight(struct intel_connector *connector, enum pipe pipe
 		return panel->backlight.max;
 	}
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, sizeof(buf)) < 0) {
+	if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf,
+			     sizeof(buf)) != sizeof(buf)) {
 		drm_err(&i915->drm, "Failed to read brightness from DPCD\n");
 		return 0;
 	}
@@ -172,7 +173,8 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state,
 	buf[0] = level & 0xFF;
 	buf[1] = (level & 0xFF00) >> 8;
 
-	if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, 4) < 0)
+	if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf,
+			      sizeof(buf)) != sizeof(buf))
 		drm_err(dev, "Failed to write brightness level to DPCD\n");
 }
 
@@ -203,7 +205,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
 	u8 old_ctrl, ctrl;
 
 	ret = drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
-	if (ret < 0) {
+	if (ret != 1) {
 		drm_err(&i915->drm, "Failed to read current backlight control mode: %d\n", ret);
 		return;
 	}
@@ -221,7 +223,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
 	}
 
 	if (ctrl != old_ctrl)
-		if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) < 0)
+		if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1)
 			drm_err(&i915->drm, "Failed to configure DPCD brightness controls\n");
 }
 
@@ -277,8 +279,7 @@ static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
 	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
 		return;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
-			      &reg_val) < 0) {
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
 		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
 			    DP_EDP_DISPLAY_CONTROL_REGISTER);
 		return;
@@ -332,8 +333,8 @@ static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, en
 	if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
 		return connector->panel.backlight.max;
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
-			     &read_val, sizeof(read_val)) < 0) {
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
+			     sizeof(read_val)) != sizeof(read_val)) {
 		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
 			    DP_EDP_BACKLIGHT_BRIGHTNESS_MSB);
 		return 0;
@@ -365,8 +366,8 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
 		vals[0] = (level & 0xFF00) >> 8;
 		vals[1] = (level & 0xFF);
 	}
-	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
-			      vals, sizeof(vals)) < 0) {
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
+			      sizeof(vals)) != sizeof(vals)) {
 		drm_dbg_kms(&i915->drm,
 			    "Failed to write aux backlight level\n");
 		return;
@@ -401,9 +402,8 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
 		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 
-		if (drm_dp_dpcd_writeb(&intel_dp->aux,
-				       DP_EDP_PWMGEN_BIT_COUNT,
-				       pwmgen_bit_count) < 0)
+		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
+				       pwmgen_bit_count) != 1)
 			drm_dbg_kms(&i915->drm,
 				    "Failed to write aux pwmgen bit count\n");
 
@@ -424,11 +424,9 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 	}
 
 	if (new_dpcd_buf != dpcd_buf) {
-		if (drm_dp_dpcd_writeb(&intel_dp->aux,
-			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
-			drm_dbg_kms(&i915->drm,
-				    "Failed to write aux backlight mode\n");
-		}
+		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
+				       new_dpcd_buf) != 1)
+			drm_dbg_kms(&i915->drm, "Failed to write aux backlight mode\n");
 	}
 
 	intel_dp_aux_vesa_set_backlight(conn_state, level);
@@ -519,8 +517,7 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 	}
 
 	drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
-	if (drm_dp_dpcd_writeb(&intel_dp->aux,
-			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn) != 1) {
 		drm_dbg_kms(&i915->drm,
 			    "Failed to write aux pwmgen bit count\n");
 		return max_backlight;
-- 
2.29.2

Re: [RFC v3 04/10] drm/i915/dpcd_bl: Handle drm_dpcd_read/write() return values correctly

[Rodrigo Vivi]

On Fri, Feb 05, 2021 at 06:45:08PM -0500, Lyude Paul wrote:
> This is kind of an annoying aspect of DRM's DP helpers:
> drm_dp_dpcd_readb/writeb() return the size of bytes read/written on
> success, thus we want to check against that instead of checking if the
> return value is less than 0.
> 
> I'll probably be fixing this in the near future once I start doing DP work
> again, also because I'd rather not mix a tree-wide refactor like that in
> with a patch series intended to be around introducing DP backlight helpers.
> So, for now let's just handle the return values from each function
> correctly.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  .../drm/i915/display/intel_dp_aux_backlight.c | 41 +++++++++----------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 62294967f430..c37ccc8538cb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -107,7 +107,7 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
>  	u8 tcon_cap[4];
>  
>  	ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
> -	if (ret < 0)
> +	if (ret != sizeof(tcon_cap))
>  		return false;
>  
>  	if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP))
> @@ -137,7 +137,7 @@ intel_dp_aux_hdr_get_backlight(struct intel_connector *connector, enum pipe pipe
>  	u8 tmp;
>  	u8 buf[2] = { 0 };
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) < 0) {
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) != 1) {
>  		drm_err(&i915->drm, "Failed to read current backlight mode from DPCD\n");
>  		return 0;
>  	}
> @@ -153,7 +153,8 @@ intel_dp_aux_hdr_get_backlight(struct intel_connector *connector, enum pipe pipe
>  		return panel->backlight.max;
>  	}
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, sizeof(buf)) < 0) {
> +	if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf,
> +			     sizeof(buf)) != sizeof(buf)) {
>  		drm_err(&i915->drm, "Failed to read brightness from DPCD\n");
>  		return 0;
>  	}
> @@ -172,7 +173,8 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state,
>  	buf[0] = level & 0xFF;
>  	buf[1] = (level & 0xFF00) >> 8;
>  
> -	if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, 4) < 0)
> +	if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf,
> +			      sizeof(buf)) != sizeof(buf))
>  		drm_err(dev, "Failed to write brightness level to DPCD\n");
>  }
>  
> @@ -203,7 +205,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	u8 old_ctrl, ctrl;
>  
>  	ret = drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
> -	if (ret < 0) {
> +	if (ret != 1) {
>  		drm_err(&i915->drm, "Failed to read current backlight control mode: %d\n", ret);
>  		return;
>  	}
> @@ -221,7 +223,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	}
>  
>  	if (ctrl != old_ctrl)
> -		if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) < 0)
> +		if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1)
>  			drm_err(&i915->drm, "Failed to configure DPCD brightness controls\n");
>  }
>  
> @@ -277,8 +279,7 @@ static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
>  		return;
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> -			      &reg_val) < 0) {
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
>  		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
>  			    DP_EDP_DISPLAY_CONTROL_REGISTER);
>  		return;
> @@ -332,8 +333,8 @@ static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, en
>  	if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
>  		return connector->panel.backlight.max;
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> -			     &read_val, sizeof(read_val)) < 0) {
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
> +			     sizeof(read_val)) != sizeof(read_val)) {
>  		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
>  			    DP_EDP_BACKLIGHT_BRIGHTNESS_MSB);
>  		return 0;
> @@ -365,8 +366,8 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
>  		vals[0] = (level & 0xFF00) >> 8;
>  		vals[1] = (level & 0xFF);
>  	}
> -	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> -			      vals, sizeof(vals)) < 0) {
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
> +			      sizeof(vals)) != sizeof(vals)) {
>  		drm_dbg_kms(&i915->drm,
>  			    "Failed to write aux backlight level\n");
>  		return;
> @@ -401,9 +402,8 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>  		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>  
> -		if (drm_dp_dpcd_writeb(&intel_dp->aux,
> -				       DP_EDP_PWMGEN_BIT_COUNT,
> -				       pwmgen_bit_count) < 0)
> +		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
> +				       pwmgen_bit_count) != 1)
>  			drm_dbg_kms(&i915->drm,
>  				    "Failed to write aux pwmgen bit count\n");
>  
> @@ -424,11 +424,9 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	}
>  
>  	if (new_dpcd_buf != dpcd_buf) {
> -		if (drm_dp_dpcd_writeb(&intel_dp->aux,
> -			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
> -			drm_dbg_kms(&i915->drm,
> -				    "Failed to write aux backlight mode\n");
> -		}
> +		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> +				       new_dpcd_buf) != 1)
> +			drm_dbg_kms(&i915->drm, "Failed to write aux backlight mode\n");
>  	}
>  
>  	intel_dp_aux_vesa_set_backlight(conn_state, level);
> @@ -519,8 +517,7 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
>  	}
>  
>  	drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> -			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn) != 1) {
>  		drm_dbg_kms(&i915->drm,
>  			    "Failed to write aux pwmgen bit count\n");
>  		return max_backlight;
> -- 
> 2.29.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v3 05/10] drm/i915/dpcd_bl: Cleanup intel_dp_aux_vesa_enable_backlight() a bit

[Lyude Paul]

Get rid of the extraneous switch case in here, and just open code
edp_backlight_mode as we only ever use it once.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index c37ccc8538cb..95e3e344cf40 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -382,7 +382,7 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_panel *panel = &connector->panel;
-	u8 dpcd_buf, new_dpcd_buf, edp_backlight_mode;
+	u8 dpcd_buf, new_dpcd_buf;
 	u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
@@ -393,12 +393,8 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 	}
 
 	new_dpcd_buf = dpcd_buf;
-	edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
 
-	switch (edp_backlight_mode) {
-	case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
-	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
-	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
+	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {
 		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
 		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 
@@ -406,13 +402,6 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 				       pwmgen_bit_count) != 1)
 			drm_dbg_kms(&i915->drm,
 				    "Failed to write aux pwmgen bit count\n");
-
-		break;
-
-	/* Do nothing when it is already DPCD mode */
-	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
-	default:
-		break;
 	}
 
 	if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
-- 
2.29.2

Re: [Nouveau] [RFC v3 05/10] drm/i915/dpcd_bl: Cleanup intel_dp_aux_vesa_enable_backlight() a bit

[Ilia Mirkin]

On Fri, Feb 5, 2021 at 6:45 PM Lyude Paul <lyude@redhat.com> wrote:
>
> Get rid of the extraneous switch case in here, and just open code
> edp_backlight_mode as we only ever use it once.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index c37ccc8538cb..95e3e344cf40 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -382,7 +382,7 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>         struct intel_dp *intel_dp = intel_attached_dp(connector);
>         struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>         struct intel_panel *panel = &connector->panel;
> -       u8 dpcd_buf, new_dpcd_buf, edp_backlight_mode;
> +       u8 dpcd_buf, new_dpcd_buf;
>         u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
>
>         if (drm_dp_dpcd_readb(&intel_dp->aux,
> @@ -393,12 +393,8 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>         }
>
>         new_dpcd_buf = dpcd_buf;
> -       edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>
> -       switch (edp_backlight_mode) {
> -       case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
> -       case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
> -       case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
> +       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {

You probably meant != MODE_DPCD?

>                 new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>                 new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>
> @@ -406,13 +402,6 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>                                        pwmgen_bit_count) != 1)
>                         drm_dbg_kms(&i915->drm,
>                                     "Failed to write aux pwmgen bit count\n");
> -
> -               break;
> -
> -       /* Do nothing when it is already DPCD mode */
> -       case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
> -       default:
> -               break;
>         }
>
>         if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
> --
> 2.29.2
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC v3 06/10] drm/i915/dpcd_bl: Cache some backlight capabilities in intel_panel.backlight

[Lyude Paul]

Since we're about to be moving this code into shared DRM helpers, we might
as well start to cache certain backlight capabilities that can be
determined from the EDP DPCD, and are likely to be relevant to the majority
of drivers using said helpers. The main purpose of this is just to prevent
every driver from having to check everything against the eDP DPCD using DP
macros, which makes the code slightly easier to read (especially since the
names of some of the eDP capabilities don't exactly match up with what we
actually need to use them for, like DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT
for instance).

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 ++
 .../drm/i915/display/intel_dp_aux_backlight.c | 29 ++++++++++++-------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index f4b26e1dbaaf..16824eb3ef93 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -265,6 +265,8 @@ struct intel_panel {
 			struct {
 				u8 pwmgen_bit_count;
 				u8 pwm_freq_pre_divider;
+				bool lsb_reg_used;
+				bool aux_enable;
 			} vesa;
 			struct {
 				bool sdr_uses_aux;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 95e3e344cf40..f5ae2fb34c1f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -270,13 +270,14 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
 }
 
 /* VESA backlight callbacks */
-static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
+static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
 {
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	u8 reg_val = 0;
 
 	/* Early return when display use other mechanism to enable backlight. */
-	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
+	if (!connector->panel.backlight.edp.vesa.aux_enable)
 		return;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
@@ -339,9 +340,11 @@ static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, en
 			    DP_EDP_BACKLIGHT_BRIGHTNESS_MSB);
 		return 0;
 	}
-	level = read_val[0];
-	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
+
+	if (connector->panel.backlight.edp.vesa.lsb_reg_used)
 		level = (read_val[0] << 8 | read_val[1]);
+	else
+		level = read_val[0];
 
 	return level;
 }
@@ -359,13 +362,14 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	u8 vals[2] = { 0x0 };
 
-	vals[0] = level;
-
 	/* Write the MSB and/or LSB */
-	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) {
+	if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
 		vals[0] = (level & 0xFF00) >> 8;
 		vals[1] = (level & 0xFF);
+	} else {
+		vals[0] = level;
 	}
+
 	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
 			      sizeof(vals)) != sizeof(vals)) {
 		drm_dbg_kms(&i915->drm,
@@ -419,14 +423,13 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 	}
 
 	intel_dp_aux_vesa_set_backlight(conn_state, level);
-	set_vesa_backlight_enable(intel_dp, true);
+	set_vesa_backlight_enable(connector, true);
 }
 
 static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state *old_conn_state,
 						u32 level)
 {
-	set_vesa_backlight_enable(enc_to_intel_dp(to_intel_encoder(old_conn_state->best_encoder)),
-				  false);
+	set_vesa_backlight_enable(to_intel_connector(old_conn_state->connector), false);
 }
 
 /*
@@ -524,8 +527,14 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
 					     enum pipe pipe)
 {
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_panel *panel = &connector->panel;
 
+	if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
+		panel->backlight.edp.vesa.aux_enable = true;
+	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
+		panel->backlight.edp.vesa.lsb_reg_used = true;
+
 	panel->backlight.max = intel_dp_aux_vesa_calc_max_backlight(connector);
 	if (!panel->backlight.max)
 		return -ENODEV;
-- 
2.29.2

Re: [RFC v3 06/10] drm/i915/dpcd_bl: Cache some backlight capabilities in intel_panel.backlight

[Rodrigo Vivi]

On Fri, Feb 05, 2021 at 06:45:10PM -0500, Lyude Paul wrote:
> Since we're about to be moving this code into shared DRM helpers, we might
> as well start to cache certain backlight capabilities that can be
> determined from the EDP DPCD, and are likely to be relevant to the majority
> of drivers using said helpers. The main purpose of this is just to prevent
> every driver from having to check everything against the eDP DPCD using DP
> macros, which makes the code slightly easier to read (especially since the
> names of some of the eDP capabilities don't exactly match up with what we
> actually need to use them for, like DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> for instance).
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  .../drm/i915/display/intel_dp_aux_backlight.c | 29 ++++++++++++-------
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index f4b26e1dbaaf..16824eb3ef93 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -265,6 +265,8 @@ struct intel_panel {
>  			struct {
>  				u8 pwmgen_bit_count;
>  				u8 pwm_freq_pre_divider;
> +				bool lsb_reg_used;
> +				bool aux_enable;
>  			} vesa;
>  			struct {
>  				bool sdr_uses_aux;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 95e3e344cf40..f5ae2fb34c1f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -270,13 +270,14 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
>  }
>  
>  /* VESA backlight callbacks */
> -static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
> +static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
>  {
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	u8 reg_val = 0;
>  
>  	/* Early return when display use other mechanism to enable backlight. */
> -	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> +	if (!connector->panel.backlight.edp.vesa.aux_enable)
>  		return;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
> @@ -339,9 +340,11 @@ static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, en
>  			    DP_EDP_BACKLIGHT_BRIGHTNESS_MSB);
>  		return 0;
>  	}
> -	level = read_val[0];
> -	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> +
> +	if (connector->panel.backlight.edp.vesa.lsb_reg_used)
>  		level = (read_val[0] << 8 | read_val[1]);
> +	else
> +		level = read_val[0];
>  
>  	return level;
>  }
> @@ -359,13 +362,14 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	u8 vals[2] = { 0x0 };
>  
> -	vals[0] = level;
> -
>  	/* Write the MSB and/or LSB */
> -	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) {
> +	if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
>  		vals[0] = (level & 0xFF00) >> 8;
>  		vals[1] = (level & 0xFF);
> +	} else {
> +		vals[0] = level;
>  	}
> +
>  	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
>  			      sizeof(vals)) != sizeof(vals)) {
>  		drm_dbg_kms(&i915->drm,
> @@ -419,14 +423,13 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	}
>  
>  	intel_dp_aux_vesa_set_backlight(conn_state, level);
> -	set_vesa_backlight_enable(intel_dp, true);
> +	set_vesa_backlight_enable(connector, true);
>  }
>  
>  static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state *old_conn_state,
>  						u32 level)
>  {
> -	set_vesa_backlight_enable(enc_to_intel_dp(to_intel_encoder(old_conn_state->best_encoder)),
> -				  false);
> +	set_vesa_backlight_enable(to_intel_connector(old_conn_state->connector), false);
>  }
>  
>  /*
> @@ -524,8 +527,14 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
>  static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
>  					     enum pipe pipe)
>  {
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_panel *panel = &connector->panel;
>  
> +	if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> +		panel->backlight.edp.vesa.aux_enable = true;
> +	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> +		panel->backlight.edp.vesa.lsb_reg_used = true;
> +
>  	panel->backlight.max = intel_dp_aux_vesa_calc_max_backlight(connector);
>  	if (!panel->backlight.max)
>  		return -ENODEV;
> -- 
> 2.29.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v3 07/10] drm/i915/dpcd_bl: Move VESA backlight enabling code closer together

[Lyude Paul]

No functional changes, just move set_vesa_backlight_enable() closer to it's
only caller: intel_dp_aux_vesa_enable_backlight().

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 54 +++++++++----------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index f5ae2fb34c1f..431758058aa0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -270,33 +270,6 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
 }
 
 /* VESA backlight callbacks */
-static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
-{
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	u8 reg_val = 0;
-
-	/* Early return when display use other mechanism to enable backlight. */
-	if (!connector->panel.backlight.edp.vesa.aux_enable)
-		return;
-
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
-			    DP_EDP_DISPLAY_CONTROL_REGISTER);
-		return;
-	}
-	if (enable)
-		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
-	else
-		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
-
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
-			       reg_val) != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight\n",
-			    enable ? "enable" : "disable");
-	}
-}
-
 static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
@@ -378,6 +351,33 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
 	}
 }
 
+static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	u8 reg_val = 0;
+
+	/* Early return when display use other mechanism to enable backlight. */
+	if (!connector->panel.backlight.edp.vesa.aux_enable)
+		return;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
+			    DP_EDP_DISPLAY_CONTROL_REGISTER);
+		return;
+	}
+	if (enable)
+		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
+	else
+		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
+			       reg_val) != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight\n",
+			    enable ? "enable" : "disable");
+	}
+}
+
 static void
 intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state, u32 level)
-- 
2.29.2

Re: [RFC v3 07/10] drm/i915/dpcd_bl: Move VESA backlight enabling code closer together

[Rodrigo Vivi]

On Fri, Feb 05, 2021 at 06:45:11PM -0500, Lyude Paul wrote:
> No functional changes, just move set_vesa_backlight_enable() closer to it's
> only caller: intel_dp_aux_vesa_enable_backlight().
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  .../drm/i915/display/intel_dp_aux_backlight.c | 54 +++++++++----------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index f5ae2fb34c1f..431758058aa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -270,33 +270,6 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
>  }
>  
>  /* VESA backlight callbacks */
> -static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
> -{
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	u8 reg_val = 0;
> -
> -	/* Early return when display use other mechanism to enable backlight. */
> -	if (!connector->panel.backlight.edp.vesa.aux_enable)
> -		return;
> -
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
> -			    DP_EDP_DISPLAY_CONTROL_REGISTER);
> -		return;
> -	}
> -	if (enable)
> -		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
> -	else
> -		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> -
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> -			       reg_val) != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight\n",
> -			    enable ? "enable" : "disable");
> -	}
> -}
> -
>  static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -378,6 +351,33 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
>  	}
>  }
>  
> +static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u8 reg_val = 0;
> +
> +	/* Early return when display use other mechanism to enable backlight. */
> +	if (!connector->panel.backlight.edp.vesa.aux_enable)
> +		return;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
> +		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
> +			    DP_EDP_DISPLAY_CONTROL_REGISTER);
> +		return;
> +	}
> +	if (enable)
> +		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
> +	else
> +		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> +			       reg_val) != 1) {
> +		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight\n",
> +			    enable ? "enable" : "disable");
> +	}
> +}
> +
>  static void
>  intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state, u32 level)
> -- 
> 2.29.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v3 08/10] drm/i915/dpcd_bl: Return early in vesa_calc_max_backlight if we can't read PWMGEN_BIT_COUNT

[Lyude Paul]

If we can't read DP_EDP_PWMGEN_BIT_COUNT in
intel_dp_aux_vesa_calc_max_backlight() but do have a valid PWM frequency
defined in the VBT, we'll keep going in the function until we inevitably
fail on reading DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN. There's not much point in
doing this, so just return early.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 431758058aa0..c66350cdf7e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -449,11 +449,14 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 	int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
 	u8 pn, pn_min, pn_max;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, &pn) == 1) {
-		pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
-		max_backlight = (1 << pn) - 1;
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, &pn) != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap\n");
+		return 0;
 	}
 
+	pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	max_backlight = (1 << pn) - 1;
+
 	/* Find desired value of (F x P)
 	 * Note that, if F x P is out of supported range, the maximum value or
 	 * minimum value will applied automatically. So no need to check that.
-- 
2.29.2

[RFC v3 09/10] drm/i915/dpcd_bl: Print return codes for VESA backlight failures

[Lyude Paul]

Also, stop printing the DPCD register that failed, and just describe it
instead. Saves us from having to look up each register offset when reading
through kernel logs (plus, DPCD dumping with drm.debug |= 0x100 will give
us that anyway).

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 101 +++++++++---------
 1 file changed, 52 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index c66350cdf7e9..813f6c553156 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -274,14 +274,12 @@ static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector *connec
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	int ret;
 	u8 mode_reg;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux,
-			      DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
-			      &mode_reg) != 1) {
-		drm_dbg_kms(&i915->drm,
-			    "Failed to read the DPCD register 0x%x\n",
-			    DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read backlight mode: %d\n", ret);
 		return false;
 	}
 
@@ -297,6 +295,7 @@ static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, en
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	int ret;
 	u8 read_val[2] = { 0x0 };
 	u16 level = 0;
 
@@ -307,10 +306,10 @@ static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, en
 	if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
 		return connector->panel.backlight.max;
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
-			     sizeof(read_val)) != sizeof(read_val)) {
-		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
-			    DP_EDP_BACKLIGHT_BRIGHTNESS_MSB);
+	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
+			       sizeof(read_val));
+	if (ret != sizeof(read_val)) {
+		drm_dbg_kms(&i915->drm, "Failed to read brightness level: %d\n", ret);
 		return 0;
 	}
 
@@ -333,6 +332,7 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	int ret;
 	u8 vals[2] = { 0x0 };
 
 	/* Write the MSB and/or LSB */
@@ -343,10 +343,10 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
 		vals[0] = level;
 	}
 
-	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
-			      sizeof(vals)) != sizeof(vals)) {
-		drm_dbg_kms(&i915->drm,
-			    "Failed to write aux backlight level\n");
+	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
+				sizeof(vals));
+	if (ret != sizeof(vals)) {
+		drm_dbg_kms(&i915->drm, "Failed to write aux backlight level: %d\n", ret);
 		return;
 	}
 }
@@ -355,26 +355,28 @@ static void set_vesa_backlight_enable(struct intel_connector *connector, bool en
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	int ret;
 	u8 reg_val = 0;
 
 	/* Early return when display use other mechanism to enable backlight. */
 	if (!connector->panel.backlight.edp.vesa.aux_enable)
 		return;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val) != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
-			    DP_EDP_DISPLAY_CONTROL_REGISTER);
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read eDP display control register: %d\n", ret);
 		return;
 	}
+
 	if (enable)
 		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
 	else
 		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
 
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
-			       reg_val) != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight\n",
-			    enable ? "enable" : "disable");
+	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight: %d\n",
+			    enable ? "enable" : "disable", ret);
 	}
 }
 
@@ -386,13 +388,13 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_panel *panel = &connector->panel;
+	int ret;
 	u8 dpcd_buf, new_dpcd_buf;
 	u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux,
-			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read DPCD register 0x%x\n",
-			    DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read backlight mode: %d\n", ret);
 		return;
 	}
 
@@ -402,24 +404,26 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
 		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 
-		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
-				       pwmgen_bit_count) != 1)
-			drm_dbg_kms(&i915->drm,
-				    "Failed to write aux pwmgen bit count\n");
+		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pwmgen_bit_count);
+		if (ret != 1)
+			drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit count: %d\n", ret);
 	}
 
 	if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
-		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
-				       panel->backlight.edp.vesa.pwm_freq_pre_divider) == 1)
+		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
+					 panel->backlight.edp.vesa.pwm_freq_pre_divider);
+		if (ret == 1)
 			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
 		else
-			drm_dbg_kms(&i915->drm, "Failed to write aux backlight frequency\n");
+			drm_dbg_kms(&i915->drm, "Failed to write aux backlight frequency: %d\n",
+				    ret);
 	}
 
 	if (new_dpcd_buf != dpcd_buf) {
-		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
-				       new_dpcd_buf) != 1)
-			drm_dbg_kms(&i915->drm, "Failed to write aux backlight mode\n");
+		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
+					 new_dpcd_buf);
+		if (ret != 1)
+			drm_dbg_kms(&i915->drm, "Failed to write aux backlight mode: %d\n", ret);
 	}
 
 	intel_dp_aux_vesa_set_backlight(conn_state, level);
@@ -446,11 +450,12 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_panel *panel = &connector->panel;
 	u32 max_backlight = 0;
-	int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
+	int ret, freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
 	u8 pn, pn_min, pn_max;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, &pn) != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap\n");
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap: %d\n", ret);
 		return 0;
 	}
 
@@ -479,16 +484,14 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 	 * - FxP is within 25% of desired value.
 	 *   Note: 25% is arbitrary value and may need some tweak.
 	 */
-	if (drm_dp_dpcd_readb(&intel_dp->aux,
-			      DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) {
-		drm_dbg_kms(&i915->drm,
-			    "Failed to read pwmgen bit count cap min\n");
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap min: %d\n", ret);
 		return max_backlight;
 	}
-	if (drm_dp_dpcd_readb(&intel_dp->aux,
-			      DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
-		drm_dbg_kms(&i915->drm,
-			    "Failed to read pwmgen bit count cap max\n");
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap max: %d\n", ret);
 		return max_backlight;
 	}
 	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
@@ -512,9 +515,9 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 	}
 
 	drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn) != 1) {
-		drm_dbg_kms(&i915->drm,
-			    "Failed to write aux pwmgen bit count\n");
+	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
+	if (ret != 1) {
+		drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit count: %d\n", ret);
 		return max_backlight;
 	}
 
-- 
2.29.2

[RFC v3 10/10] drm/dp: Extract i915's eDP backlight code into DRM helpers

[Lyude Paul]

Since we're about to implement eDP backlight support in nouveau using the
standard protocol from VESA, we might as well just take the code that's
already written for this and move it into a set of shared DRM helpers.

Note that these helpers are intended to handle DPCD related backlight
control bits such as setting the brightness level over AUX, probing the
backlight's TCON, enabling/disabling the backlight over AUX if supported,
etc. Any PWM-related portions of backlight control are explicitly left up
to the driver, as these will vary from platform to platform.

The only exception to this is the calculation of the PWM frequency
pre-divider value. This is because the only platform-specific information
required for this is the PWM frequency of the panel, which the driver is
expected to provide if available. The actual algorithm for calculating this
value is standard and is defined in the eDP specification from VESA.

Note that these helpers do not yet implement the full range of features
the VESA backlight interface provides, and only provide the following
functionality (all of which was already present in i915's DPCD backlight
support):

* Basic control of brightness levels
* Basic probing of backlight capabilities
* Helpers for enabling and disabling the backlight

v3:
* Split out changes to i915's backlight code to separate patches to make it
  easier to review

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@gmail.com>
Cc: greg.depoire@gmail.com
---
 drivers/gpu/drm/drm_dp_helper.c               | 332 ++++++++++++++++++
 .../drm/i915/display/intel_display_types.h    |   5 +-
 .../drm/i915/display/intel_dp_aux_backlight.c | 285 ++-------------
 include/drm/drm_dp_helper.h                   |  48 +++
 4 files changed, 412 insertions(+), 258 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index eedbb48815b7..04cb2b6970a8 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3082,3 +3082,335 @@ int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc)
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
+
+/**
+ * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel via AUX
+ * @aux: The DP AUX channel to use
+ * @bl: Backlight capability info from drm_edp_backlight_init()
+ * @level: The brightness level to set
+ *
+ * Sets the brightness level of an eDP panel's backlight. Note that the panel's backlight must
+ * already have been enabled by the driver by calling drm_edp_backlight_enable().
+ *
+ * Returns: %0 on success, negative error code on failure
+ */
+int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
+				u16 level)
+{
+	int ret;
+	u8 buf[2] = { 0 };
+
+	if (bl->lsb_reg_used) {
+		buf[0] = (level & 0xFF00) >> 8;
+		buf[1] = (level & 0x00FF);
+	} else {
+		buf[0] = level;
+	}
+
+	ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		DRM_ERROR("%s: Failed to write aux backlight level: %d\n", aux->name, ret);
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_edp_backlight_set_level);
+
+static int
+drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
+			     bool enable)
+{
+	int ret;
+	u8 buf;
+
+	/* The panel uses something other then DPCD for enabling it's backlight */
+	if (!bl->aux_enable)
+		return 0;
+
+	ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &buf);
+	if (ret != 1) {
+		DRM_ERROR("%s: Failed to read eDP display control register: %d\n", aux->name, ret);
+		return ret < 0 ? ret : -EIO;
+	}
+	if (enable)
+		buf |= DP_EDP_BACKLIGHT_ENABLE;
+	else
+		buf &= ~DP_EDP_BACKLIGHT_ENABLE;
+
+	ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
+	if (ret != 1) {
+		DRM_ERROR("%s: Failed to write eDP display control register: %d\n", aux->name, ret);
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return 0;
+}
+
+/**
+ * drm_edp_backlight_enable() - Enable an eDP panel's backlight using DPCD
+ * @aux: The DP AUX channel to use
+ * @bl: Backlight capability info from drm_edp_backlight_init()
+ * @level: The initial backlight level to set via AUX, if there is one
+ *
+ * This function handles enabling DPCD backlight controls on a panel over DPCD, while additionally
+ * restoring any important backlight state such as the given backlight level, the brightness byte
+ * count, backlight frequency, etc.
+ *
+ * Note that certain panels, while supporting brightness level controls over DPCD, may not support
+ * having their backlights enabled via the standard %DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
+ * &drm_edp_backlight_info.aux_enable will be set to %false, this function will skip the step of
+ * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must perform the required
+ * implementation specific step for enabling the backlight after calling this function.
+ *
+ * Returns: %0 on success, negative error code on failure.
+ */
+int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
+			     const u16 level)
+{
+	int ret;
+	u8 dpcd_buf, new_dpcd_buf;
+
+	ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
+	if (ret != 1) {
+		DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n", aux->name, ret);
+		return ret < 0 ? ret : -EIO;
+	}
+
+	new_dpcd_buf = dpcd_buf;
+
+	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
+		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
+		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+
+		ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, bl->pwmgen_bit_count);
+		if (ret != 1)
+			DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count: %d\n",
+				      aux->name, ret);
+	}
+
+	if (bl->pwm_freq_pre_divider) {
+		ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_FREQ_SET, bl->pwm_freq_pre_divider);
+		if (ret != 1)
+			DRM_DEBUG_KMS("%s: Failed to write aux backlight frequency: %d\n",
+				      aux->name, ret);
+		else
+			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+	}
+
+	if (new_dpcd_buf != dpcd_buf) {
+		ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
+		if (ret != 1) {
+			DRM_DEBUG_KMS("%s: Failed to write aux backlight mode: %d\n",
+				      aux->name, ret);
+			return ret < 0 ? ret : -EIO;
+		}
+	}
+
+	ret = drm_edp_backlight_set_level(aux, bl, level);
+	if (ret < 0)
+		return ret;
+	ret = drm_edp_backlight_set_enable(aux, bl, true);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_edp_backlight_enable);
+
+/**
+ * drm_edp_backlight_disable() - Disable an eDP backlight using DPCD, if supported
+ * @aux: The DP AUX channel to use
+ * @bl: Backlight capability info from drm_edp_backlight_init()
+ *
+ * This function handles disabling DPCD backlight controls on a panel over AUX. Note that some
+ * panels have backlights that are enabled/disabled by other means, despite having their brightness
+ * values controlled through DPCD. On such panels &drm_edp_backlight_info.aux_enable will be set to
+ * %false, this function will become a no-op (and we will skip updating
+ * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform it's own
+ * implementation specific step for disabling the backlight.
+ *
+ * Returns: %0 on success or no-op, negative error code on failure.
+ */
+int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl)
+{
+	int ret;
+
+	ret = drm_edp_backlight_set_enable(aux, bl, false);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_edp_backlight_disable);
+
+static inline int
+drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
+			    u16 driver_pwm_freq_hz, const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
+{
+	int fxp, fxp_min, fxp_max, fxp_actual, f = 1;
+	int ret;
+	u8 pn, pn_min, pn_max;
+
+	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
+	if (ret != 1) {
+		DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap: %d\n", aux->name, ret);
+		return -ENODEV;
+	}
+
+	pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	bl->max = (1 << pn) - 1;
+	if (!driver_pwm_freq_hz)
+		return 0;
+
+	/*
+	 * Set PWM Frequency divider to match desired frequency provided by the driver.
+	 * The PWM Frequency is calculated as 27Mhz / (F x P).
+	 * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
+	 *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
+	 * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
+	 *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
+	 */
+
+	/* Find desired value of (F x P)
+	 * Note that, if F x P is out of supported range, the maximum value or minimum value will
+	 * applied automatically. So no need to check that.
+	 */
+	fxp = DIV_ROUND_CLOSEST(1000 * DP_EDP_BACKLIGHT_FREQ_BASE_KHZ, driver_pwm_freq_hz);
+
+	/* Use highest possible value of Pn for more granularity of brightness adjustment while
+	 * satifying the conditions below.
+	 * - Pn is in the range of Pn_min and Pn_max
+	 * - F is in the range of 1 and 255
+	 * - FxP is within 25% of desired value.
+	 *   Note: 25% is arbitrary value and may need some tweak.
+	 */
+	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
+	if (ret != 1) {
+		DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap min: %d\n", aux->name, ret);
+		return 0;
+	}
+	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
+	if (ret != 1) {
+		DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap max: %d\n", aux->name, ret);
+		return 0;
+	}
+	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+
+	/* Ensure frequency is within 25% of desired value */
+	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
+	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
+	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
+		DRM_DEBUG_KMS("%s: Driver defined backlight frequency (%d) out of range\n",
+			      aux->name, driver_pwm_freq_hz);
+		return 0;
+	}
+
+	for (pn = pn_max; pn >= pn_min; pn--) {
+		f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
+		fxp_actual = f << pn;
+		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
+			break;
+	}
+
+	ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
+	if (ret != 1) {
+		DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count: %d\n", aux->name, ret);
+		return 0;
+	}
+	bl->pwmgen_bit_count = pn;
+	bl->max = (1 << pn) - 1;
+
+	if (edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) {
+		bl->pwm_freq_pre_divider = f;
+		DRM_DEBUG_KMS("%s: Using backlight frequency from driver (%dHz)\n",
+			      aux->name, driver_pwm_freq_hz);
+	}
+
+	return 0;
+}
+
+static inline int
+drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
+			      u8 *current_mode)
+{
+	int ret;
+	u8 buf[2];
+	u8 mode_reg;
+
+	ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
+	if (ret != 1) {
+		DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n", aux->name, ret);
+		return ret < 0 ? ret : -EIO;
+	}
+
+	*current_mode = (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK);
+	if (*current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
+		int size = 1 + bl->lsb_reg_used;
+
+		ret = drm_dp_dpcd_read(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf, size);
+		if (ret != size) {
+			DRM_DEBUG_KMS("%s: Failed to read backlight level: %d\n", aux->name, ret);
+			return ret < 0 ? ret : -EIO;
+		}
+
+		if (bl->lsb_reg_used)
+			return (buf[0] << 8) | buf[1];
+		else
+			return buf[0];
+	}
+
+	/*
+	 * If we're not in DPCD control mode yet, the programmed brightness value is meaningless and
+	 * the driver should assume max brightness
+	 */
+	return bl->max;
+}
+
+/**
+ * drm_edp_backlight_init() - Probe a display panel's TCON using the standard VESA eDP backlight
+ * interface.
+ * @aux: The DP aux device to use for probing
+ * @bl: The &drm_edp_backlight_info struct to fill out with information on the backlight
+ * @driver_pwm_freq_hz: Optional PWM frequency from the driver in hz
+ * @edp_dpcd: A cached copy of the eDP DPCD
+ * @current_level: Where to store the probed brightness level
+ * @current_mode: Where to store the currently set backlight control mode
+ *
+ * Initializes a &drm_edp_backlight_info struct by probing @aux for it's backlight capabilities,
+ * along with also probing the current and maximum supported brightness levels.
+ *
+ * If @driver_pwm_freq_hz is non-zero, this will be used as the backlight frequency. Otherwise, the
+ * default frequency from the panel is used.
+ *
+ * Returns: %0 on success, negative error code on failure.
+ */
+int
+drm_edp_backlight_init(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
+		       u16 driver_pwm_freq_hz, const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
+		       u16 *current_level, u8 *current_mode)
+{
+	int ret;
+
+	if (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
+		bl->aux_enable = true;
+	if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
+		bl->lsb_reg_used = true;
+
+	ret = drm_edp_backlight_probe_max(aux, bl, driver_pwm_freq_hz, edp_dpcd);
+	if (ret < 0)
+		return ret;
+
+	ret = drm_edp_backlight_probe_level(aux, bl, current_mode);
+	if (ret < 0)
+		return ret;
+	*current_level = ret;
+
+	DRM_DEBUG_KMS("%s: Found backlight level=%d/%d pwm_freq_pre_divider=%d mode=%x\n",
+		      aux->name, *current_level, bl->max, bl->pwm_freq_pre_divider, *current_mode);
+	DRM_DEBUG_KMS("%s: Backlight caps: pwmgen_bit_count=%d lsb_reg_used=%d aux_enable=%d\n",
+		      aux->name, bl->pwmgen_bit_count, bl->lsb_reg_used, bl->aux_enable);
+	return 0;
+}
+EXPORT_SYMBOL(drm_edp_backlight_init);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 16824eb3ef93..03051ab75d30 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -263,10 +263,7 @@ struct intel_panel {
 		/* DPCD backlight */
 		union {
 			struct {
-				u8 pwmgen_bit_count;
-				u8 pwm_freq_pre_divider;
-				bool lsb_reg_used;
-				bool aux_enable;
+				struct drm_edp_backlight_info info;
 			} vesa;
 			struct {
 				bool sdr_uses_aux;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 813f6c553156..286eb337448e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -270,114 +270,19 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
 }
 
 /* VESA backlight callbacks */
-static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector *connector)
-{
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	int ret;
-	u8 mode_reg;
-
-	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read backlight mode: %d\n", ret);
-		return false;
-	}
-
-	return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
-	       DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-}
-
-/*
- * Read the current backlight value from DPCD register(s) based
- * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
- */
 static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, enum pipe unused)
 {
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	int ret;
-	u8 read_val[2] = { 0x0 };
-	u16 level = 0;
-
-	/*
-	 * If we're not in DPCD control mode yet, the programmed brightness
-	 * value is meaningless and we should assume max brightness
-	 */
-	if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
-		return connector->panel.backlight.max;
-
-	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
-			       sizeof(read_val));
-	if (ret != sizeof(read_val)) {
-		drm_dbg_kms(&i915->drm, "Failed to read brightness level: %d\n", ret);
-		return 0;
-	}
-
-	if (connector->panel.backlight.edp.vesa.lsb_reg_used)
-		level = (read_val[0] << 8 | read_val[1]);
-	else
-		level = read_val[0];
-
-	return level;
+	return connector->panel.backlight.level;
 }
 
-/*
- * Sends the current backlight level over the aux channel, checking if its using
- * 8-bit or 16 bit value (MSB and LSB)
- */
 static void
-intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
-				u32 level)
+intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	int ret;
-	u8 vals[2] = { 0x0 };
-
-	/* Write the MSB and/or LSB */
-	if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
-		vals[0] = (level & 0xFF00) >> 8;
-		vals[1] = (level & 0xFF);
-	} else {
-		vals[0] = level;
-	}
-
-	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
-				sizeof(vals));
-	if (ret != sizeof(vals)) {
-		drm_dbg_kms(&i915->drm, "Failed to write aux backlight level: %d\n", ret);
-		return;
-	}
-}
-
-static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
-{
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	int ret;
-	u8 reg_val = 0;
-
-	/* Early return when display use other mechanism to enable backlight. */
-	if (!connector->panel.backlight.edp.vesa.aux_enable)
-		return;
-
-	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read eDP display control register: %d\n", ret);
-		return;
-	}
-
-	if (enable)
-		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
-	else
-		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
+	struct intel_panel *panel = &connector->panel;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
-	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight: %d\n",
-			    enable ? "enable" : "disable", ret);
-	}
+	drm_edp_backlight_set_level(&intel_dp->aux, &panel->backlight.edp.vesa.info, level);
 }
 
 static void
@@ -385,170 +290,46 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_panel *panel = &connector->panel;
-	int ret;
-	u8 dpcd_buf, new_dpcd_buf;
-	u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
-
-	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read backlight mode: %d\n", ret);
-		return;
-	}
-
-	new_dpcd_buf = dpcd_buf;
-
-	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {
-		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
-		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-
-		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pwmgen_bit_count);
-		if (ret != 1)
-			drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit count: %d\n", ret);
-	}
-
-	if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
-		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
-					 panel->backlight.edp.vesa.pwm_freq_pre_divider);
-		if (ret == 1)
-			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
-		else
-			drm_dbg_kms(&i915->drm, "Failed to write aux backlight frequency: %d\n",
-				    ret);
-	}
-
-	if (new_dpcd_buf != dpcd_buf) {
-		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
-					 new_dpcd_buf);
-		if (ret != 1)
-			drm_dbg_kms(&i915->drm, "Failed to write aux backlight mode: %d\n", ret);
-	}
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
-	intel_dp_aux_vesa_set_backlight(conn_state, level);
-	set_vesa_backlight_enable(connector, true);
+	drm_edp_backlight_enable(&intel_dp->aux, &panel->backlight.edp.vesa.info, level);
 }
 
 static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state *old_conn_state,
 						u32 level)
 {
-	set_vesa_backlight_enable(to_intel_connector(old_conn_state->connector), false);
-}
-
-/*
- * Compute PWM frequency divider value based off the frequency provided to us by the vbt.
- * The PWM Frequency is calculated as 27Mhz / (F x P).
- * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
- *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
- * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
- *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
- */
-static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connector)
-{
-	struct drm_i915_private *i915 = to_i915(connector->base.dev);
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct intel_panel *panel = &connector->panel;
-	u32 max_backlight = 0;
-	int ret, freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
-	u8 pn, pn_min, pn_max;
-
-	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap: %d\n", ret);
-		return 0;
-	}
-
-	pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
-	max_backlight = (1 << pn) - 1;
-
-	/* Find desired value of (F x P)
-	 * Note that, if F x P is out of supported range, the maximum value or
-	 * minimum value will applied automatically. So no need to check that.
-	 */
-	freq = i915->vbt.backlight.pwm_freq_hz;
-	drm_dbg_kms(&i915->drm, "VBT defined backlight frequency %u Hz\n",
-		    freq);
-	if (!freq) {
-		drm_dbg_kms(&i915->drm,
-			    "Use panel default backlight frequency\n");
-		return max_backlight;
-	}
-
-	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
-
-	/* Use highest possible value of Pn for more granularity of brightness
-	 * adjustment while satifying the conditions below.
-	 * - Pn is in the range of Pn_min and Pn_max
-	 * - F is in the range of 1 and 255
-	 * - FxP is within 25% of desired value.
-	 *   Note: 25% is arbitrary value and may need some tweak.
-	 */
-	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap min: %d\n", ret);
-		return max_backlight;
-	}
-	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap max: %d\n", ret);
-		return max_backlight;
-	}
-	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
-	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
-
-	/* Ensure frequency is within 25% of desired value */
-	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
-	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
-
-	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
-		drm_dbg_kms(&i915->drm,
-			    "VBT defined backlight frequency out of range\n");
-		return max_backlight;
-	}
-
-	for (pn = pn_max; pn >= pn_min; pn--) {
-		f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
-		fxp_actual = f << pn;
-		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
-			break;
-	}
-
-	drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
-	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
-	if (ret != 1) {
-		drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit count: %d\n", ret);
-		return max_backlight;
-	}
-
-	panel->backlight.edp.vesa.pwmgen_bit_count = pn;
-	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
-		panel->backlight.edp.vesa.pwm_freq_pre_divider = f;
-
-	max_backlight = (1 << pn) - 1;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
-	return max_backlight;
+	drm_edp_backlight_disable(&intel_dp->aux, &panel->backlight.edp.vesa.info);
 }
 
-static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
-					     enum pipe pipe)
+static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector, enum pipe pipe)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_panel *panel = &connector->panel;
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	u16 current_level;
+	u8 current_mode;
+	int ret;
 
-	if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
-		panel->backlight.edp.vesa.aux_enable = true;
-	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
-		panel->backlight.edp.vesa.lsb_reg_used = true;
-
-	panel->backlight.max = intel_dp_aux_vesa_calc_max_backlight(connector);
-	if (!panel->backlight.max)
-		return -ENODEV;
+	ret = drm_edp_backlight_init(&intel_dp->aux, &panel->backlight.edp.vesa.info,
+				     i915->vbt.backlight.pwm_freq_hz, intel_dp->edp_dpcd,
+				     &current_level, &current_mode);
+	if (ret < 0)
+		return ret;
 
+	panel->backlight.max = panel->backlight.edp.vesa.info.max;
 	panel->backlight.min = 0;
-	panel->backlight.level = intel_dp_aux_vesa_get_backlight(connector, pipe);
-	panel->backlight.enabled = intel_dp_aux_vesa_backlight_dpcd_mode(connector) &&
-				   panel->backlight.level != 0;
+	if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
+		panel->backlight.level = current_level;
+		panel->backlight.enabled = panel->backlight.level != 0;
+	} else {
+		panel->backlight.level = panel->backlight.max;
+		panel->backlight.enabled = false;
+	}
 
 	return 0;
 }
@@ -559,16 +340,12 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 
-	/* Check the eDP Display control capabilities registers to determine if
-	 * the panel can support backlight control over the aux channel.
-	 *
-	 * TODO: We currently only support AUX only backlight configurations, not backlights which
+	/* TODO: We currently only support AUX only backlight configurations, not backlights which
 	 * require a mix of PWM and AUX controls to work. In the mean time, these machines typically
 	 * work just fine using normal PWM controls anyway.
 	 */
-	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
-	    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
-	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
+	if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+	    drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
 		drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n");
 		return true;
 	}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index edffd1dcca3e..1eca0b42fc45 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1790,6 +1790,24 @@ drm_dp_sink_can_do_video_without_timing_msa(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 		DP_MSA_TIMING_PAR_IGNORED;
 }
 
+/**
+ * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support
+ * @edp_dpcd: The DPCD to check
+ *
+ * Note that currently this function will return %false for panels which support various DPCD
+ * backlight features but which require the brightness be set through PWM, and don't support setting
+ * the brightness level via the DPCD. This is a TODO.
+ *
+ * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false
+ * otherwise
+ */
+static inline bool
+drm_edp_backlight_supported(const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
+{
+	return (edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+		(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
+}
+
 /*
  * DisplayPort AUX channel
  */
@@ -2089,6 +2107,36 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
 	return desc->quirks & BIT(quirk);
 }
 
+/**
+ * struct drm_edp_backlight_info - Probed eDP backlight info struct
+ * @pwmgen_bit_count: The pwmgen bit count
+ * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used for this backlight, if any
+ * @max: The maximum backlight level that may be set
+ * @lsb_reg_used: Do we also write values to the DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register?
+ * @aux_enable: Does the panel support the AUX enable cap?
+ *
+ * This structure contains various data about an eDP backlight, which can be populated by using
+ * drm_edp_backlight_init().
+ */
+struct drm_edp_backlight_info {
+	u8 pwmgen_bit_count;
+	u8 pwm_freq_pre_divider;
+	u16 max;
+
+	bool lsb_reg_used : 1;
+	bool aux_enable : 1;
+};
+
+int
+drm_edp_backlight_init(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
+		       u16 driver_pwm_freq_hz, const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
+		       u16 *current_level, u8 *current_mode);
+int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
+				u16 level);
+int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
+			     u16 level);
+int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl);
+
 #ifdef CONFIG_DRM_DP_CEC
 void drm_dp_cec_irq(struct drm_dp_aux *aux);
 void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
-- 
2.29.2

Re: [RFC v3 10/10] drm/dp: Extract i915's eDP backlight code into DRM helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 32704 bytes --]

Hi

Am 06.02.21 um 00:45 schrieb Lyude Paul:
> Since we're about to implement eDP backlight support in nouveau using the
> standard protocol from VESA, we might as well just take the code that's
> already written for this and move it into a set of shared DRM helpers.
> 
> Note that these helpers are intended to handle DPCD related backlight
> control bits such as setting the brightness level over AUX, probing the
> backlight's TCON, enabling/disabling the backlight over AUX if supported,
> etc. Any PWM-related portions of backlight control are explicitly left up
> to the driver, as these will vary from platform to platform.
> 
> The only exception to this is the calculation of the PWM frequency
> pre-divider value. This is because the only platform-specific information
> required for this is the PWM frequency of the panel, which the driver is
> expected to provide if available. The actual algorithm for calculating this
> value is standard and is defined in the eDP specification from VESA.
> 
> Note that these helpers do not yet implement the full range of features
> the VESA backlight interface provides, and only provide the following
> functionality (all of which was already present in i915's DPCD backlight
> support):
> 
> * Basic control of brightness levels
> * Basic probing of backlight capabilities
> * Helpers for enabling and disabling the backlight
> 
> v3:
> * Split out changes to i915's backlight code to separate patches to make it
>    easier to review
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: greg.depoire@gmail.com
> ---
>   drivers/gpu/drm/drm_dp_helper.c               | 332 ++++++++++++++++++
>   .../drm/i915/display/intel_display_types.h    |   5 +-
>   .../drm/i915/display/intel_dp_aux_backlight.c | 285 ++-------------
>   include/drm/drm_dp_helper.h                   |  48 +++
>   4 files changed, 412 insertions(+), 258 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eedbb48815b7..04cb2b6970a8 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -3082,3 +3082,335 @@ int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc)
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
> +
> +/**
> + * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel via AUX
> + * @aux: The DP AUX channel to use
> + * @bl: Backlight capability info from drm_edp_backlight_init()
> + * @level: The brightness level to set
> + *
> + * Sets the brightness level of an eDP panel's backlight. Note that the panel's backlight must
> + * already have been enabled by the driver by calling drm_edp_backlight_enable().
> + *
> + * Returns: %0 on success, negative error code on failure
> + */
> +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
> +				u16 level)
> +{
> +	int ret;
> +	u8 buf[2] = { 0 };
> +
> +	if (bl->lsb_reg_used) {
> +		buf[0] = (level & 0xFF00) >> 8;
> +		buf[1] = (level & 0x00FF);

Maybe 0x00ff and 0xff00 for aesthetic reasons.

> +	} else {
> +		buf[0] = level;
> +	}
> +
> +	ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf, sizeof(buf));
> +	if (ret != sizeof(buf)) {
> +		DRM_ERROR("%s: Failed to write aux backlight level: %d\n", aux->name, ret);

Since you're adding this code, you should probably convert to drm_err() 
helpers as well. Here and elsewhere.

> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_edp_backlight_set_level);
> +
> +static int
> +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
> +			     bool enable)
> +{
> +	int ret;
> +	u8 buf;
> +
> +	/* The panel uses something other then DPCD for enabling it's backlight */

'its'

Best regards
Thomas

> +	if (!bl->aux_enable)
> +		return 0;
> +
> +	ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &buf);
> +	if (ret != 1) {
> +		DRM_ERROR("%s: Failed to read eDP display control register: %d\n", aux->name, ret);
> +		return ret < 0 ? ret : -EIO;
> +	}
> +	if (enable)
> +		buf |= DP_EDP_BACKLIGHT_ENABLE;
> +	else
> +		buf &= ~DP_EDP_BACKLIGHT_ENABLE;
> +
> +	ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
> +	if (ret != 1) {
> +		DRM_ERROR("%s: Failed to write eDP display control register: %d\n", aux->name, ret);
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * drm_edp_backlight_enable() - Enable an eDP panel's backlight using DPCD
> + * @aux: The DP AUX channel to use
> + * @bl: Backlight capability info from drm_edp_backlight_init()
> + * @level: The initial backlight level to set via AUX, if there is one
> + *
> + * This function handles enabling DPCD backlight controls on a panel over DPCD, while additionally
> + * restoring any important backlight state such as the given backlight level, the brightness byte
> + * count, backlight frequency, etc.
> + *
> + * Note that certain panels, while supporting brightness level controls over DPCD, may not support
> + * having their backlights enabled via the standard %DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
> + * &drm_edp_backlight_info.aux_enable will be set to %false, this function will skip the step of
> + * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must perform the required
> + * implementation specific step for enabling the backlight after calling this function.
> + *
> + * Returns: %0 on success, negative error code on failure.
> + */
> +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
> +			     const u16 level)
> +{
> +	int ret;
> +	u8 dpcd_buf, new_dpcd_buf;
> +
> +	ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
> +	if (ret != 1) {
> +		DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n", aux->name, ret);
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	new_dpcd_buf = dpcd_buf;
> +
> +	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> +		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> +
> +		ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, bl->pwmgen_bit_count);
> +		if (ret != 1)
> +			DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count: %d\n",
> +				      aux->name, ret);
> +	}
> +
> +	if (bl->pwm_freq_pre_divider) {
> +		ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_FREQ_SET, bl->pwm_freq_pre_divider);
> +		if (ret != 1)
> +			DRM_DEBUG_KMS("%s: Failed to write aux backlight frequency: %d\n",
> +				      aux->name, ret);
> +		else
> +			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> +	}
> +
> +	if (new_dpcd_buf != dpcd_buf) {
> +		ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> +		if (ret != 1) {
> +			DRM_DEBUG_KMS("%s: Failed to write aux backlight mode: %d\n",
> +				      aux->name, ret);
> +			return ret < 0 ? ret : -EIO;
> +		}
> +	}
> +
> +	ret = drm_edp_backlight_set_level(aux, bl, level);
> +	if (ret < 0)
> +		return ret;
> +	ret = drm_edp_backlight_set_enable(aux, bl, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_edp_backlight_enable);
> +
> +/**
> + * drm_edp_backlight_disable() - Disable an eDP backlight using DPCD, if supported
> + * @aux: The DP AUX channel to use
> + * @bl: Backlight capability info from drm_edp_backlight_init()
> + *
> + * This function handles disabling DPCD backlight controls on a panel over AUX. Note that some
> + * panels have backlights that are enabled/disabled by other means, despite having their brightness
> + * values controlled through DPCD. On such panels &drm_edp_backlight_info.aux_enable will be set to
> + * %false, this function will become a no-op (and we will skip updating
> + * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform it's own
> + * implementation specific step for disabling the backlight.
> + *
> + * Returns: %0 on success or no-op, negative error code on failure.
> + */
> +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl)
> +{
> +	int ret;
> +
> +	ret = drm_edp_backlight_set_enable(aux, bl, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_edp_backlight_disable);
> +
> +static inline int
> +drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
> +			    u16 driver_pwm_freq_hz, const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
> +{
> +	int fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> +	int ret;
> +	u8 pn, pn_min, pn_max;
> +
> +	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
> +	if (ret != 1) {
> +		DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap: %d\n", aux->name, ret);
> +		return -ENODEV;
> +	}
> +
> +	pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	bl->max = (1 << pn) - 1;
> +	if (!driver_pwm_freq_hz)
> +		return 0;
> +
> +	/*
> +	 * Set PWM Frequency divider to match desired frequency provided by the driver.
> +	 * The PWM Frequency is calculated as 27Mhz / (F x P).
> +	 * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
> +	 *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> +	 * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
> +	 *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> +	 */
> +
> +	/* Find desired value of (F x P)
> +	 * Note that, if F x P is out of supported range, the maximum value or minimum value will
> +	 * applied automatically. So no need to check that.
> +	 */
> +	fxp = DIV_ROUND_CLOSEST(1000 * DP_EDP_BACKLIGHT_FREQ_BASE_KHZ, driver_pwm_freq_hz);
> +
> +	/* Use highest possible value of Pn for more granularity of brightness adjustment while
> +	 * satifying the conditions below.
> +	 * - Pn is in the range of Pn_min and Pn_max
> +	 * - F is in the range of 1 and 255
> +	 * - FxP is within 25% of desired value.
> +	 *   Note: 25% is arbitrary value and may need some tweak.
> +	 */
> +	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
> +	if (ret != 1) {
> +		DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap min: %d\n", aux->name, ret);
> +		return 0;
> +	}
> +	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
> +	if (ret != 1) {
> +		DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap max: %d\n", aux->name, ret);
> +		return 0;
> +	}
> +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> +	/* Ensure frequency is within 25% of desired value */
> +	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> +	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> +	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> +		DRM_DEBUG_KMS("%s: Driver defined backlight frequency (%d) out of range\n",
> +			      aux->name, driver_pwm_freq_hz);
> +		return 0;
> +	}
> +
> +	for (pn = pn_max; pn >= pn_min; pn--) {
> +		f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> +		fxp_actual = f << pn;
> +		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> +			break;
> +	}
> +
> +	ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> +	if (ret != 1) {
> +		DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count: %d\n", aux->name, ret);
> +		return 0;
> +	}
> +	bl->pwmgen_bit_count = pn;
> +	bl->max = (1 << pn) - 1;
> +
> +	if (edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) {
> +		bl->pwm_freq_pre_divider = f;
> +		DRM_DEBUG_KMS("%s: Using backlight frequency from driver (%dHz)\n",
> +			      aux->name, driver_pwm_freq_hz);
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int
> +drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
> +			      u8 *current_mode)
> +{
> +	int ret;
> +	u8 buf[2];
> +	u8 mode_reg;
> +
> +	ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
> +	if (ret != 1) {
> +		DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n", aux->name, ret);
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	*current_mode = (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK);
> +	if (*current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> +		int size = 1 + bl->lsb_reg_used;
> +
> +		ret = drm_dp_dpcd_read(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf, size);
> +		if (ret != size) {
> +			DRM_DEBUG_KMS("%s: Failed to read backlight level: %d\n", aux->name, ret);
> +			return ret < 0 ? ret : -EIO;
> +		}
> +
> +		if (bl->lsb_reg_used)
> +			return (buf[0] << 8) | buf[1];
> +		else
> +			return buf[0];
> +	}
> +
> +	/*
> +	 * If we're not in DPCD control mode yet, the programmed brightness value is meaningless and
> +	 * the driver should assume max brightness
> +	 */
> +	return bl->max;
> +}
> +
> +/**
> + * drm_edp_backlight_init() - Probe a display panel's TCON using the standard VESA eDP backlight
> + * interface.
> + * @aux: The DP aux device to use for probing
> + * @bl: The &drm_edp_backlight_info struct to fill out with information on the backlight
> + * @driver_pwm_freq_hz: Optional PWM frequency from the driver in hz
> + * @edp_dpcd: A cached copy of the eDP DPCD
> + * @current_level: Where to store the probed brightness level
> + * @current_mode: Where to store the currently set backlight control mode
> + *
> + * Initializes a &drm_edp_backlight_info struct by probing @aux for it's backlight capabilities,
> + * along with also probing the current and maximum supported brightness levels.
> + *
> + * If @driver_pwm_freq_hz is non-zero, this will be used as the backlight frequency. Otherwise, the
> + * default frequency from the panel is used.
> + *
> + * Returns: %0 on success, negative error code on failure.
> + */
> +int
> +drm_edp_backlight_init(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
> +		       u16 driver_pwm_freq_hz, const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> +		       u16 *current_level, u8 *current_mode)
> +{
> +	int ret;
> +
> +	if (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> +		bl->aux_enable = true;
> +	if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> +		bl->lsb_reg_used = true;
> +
> +	ret = drm_edp_backlight_probe_max(aux, bl, driver_pwm_freq_hz, edp_dpcd);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = drm_edp_backlight_probe_level(aux, bl, current_mode);
> +	if (ret < 0)
> +		return ret;
> +	*current_level = ret;
> +
> +	DRM_DEBUG_KMS("%s: Found backlight level=%d/%d pwm_freq_pre_divider=%d mode=%x\n",
> +		      aux->name, *current_level, bl->max, bl->pwm_freq_pre_divider, *current_mode);
> +	DRM_DEBUG_KMS("%s: Backlight caps: pwmgen_bit_count=%d lsb_reg_used=%d aux_enable=%d\n",
> +		      aux->name, bl->pwmgen_bit_count, bl->lsb_reg_used, bl->aux_enable);
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_edp_backlight_init);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 16824eb3ef93..03051ab75d30 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -263,10 +263,7 @@ struct intel_panel {
>   		/* DPCD backlight */
>   		union {
>   			struct {
> -				u8 pwmgen_bit_count;
> -				u8 pwm_freq_pre_divider;
> -				bool lsb_reg_used;
> -				bool aux_enable;
> +				struct drm_edp_backlight_info info;
>   			} vesa;
>   			struct {
>   				bool sdr_uses_aux;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 813f6c553156..286eb337448e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -270,114 +270,19 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
>   }
>   
>   /* VESA backlight callbacks */
> -static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector *connector)
> -{
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	int ret;
> -	u8 mode_reg;
> -
> -	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read backlight mode: %d\n", ret);
> -		return false;
> -	}
> -
> -	return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> -	       DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> -}
> -
> -/*
> - * Read the current backlight value from DPCD register(s) based
> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> - */
>   static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector, enum pipe unused)
>   {
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	int ret;
> -	u8 read_val[2] = { 0x0 };
> -	u16 level = 0;
> -
> -	/*
> -	 * If we're not in DPCD control mode yet, the programmed brightness
> -	 * value is meaningless and we should assume max brightness
> -	 */
> -	if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
> -		return connector->panel.backlight.max;
> -
> -	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
> -			       sizeof(read_val));
> -	if (ret != sizeof(read_val)) {
> -		drm_dbg_kms(&i915->drm, "Failed to read brightness level: %d\n", ret);
> -		return 0;
> -	}
> -
> -	if (connector->panel.backlight.edp.vesa.lsb_reg_used)
> -		level = (read_val[0] << 8 | read_val[1]);
> -	else
> -		level = read_val[0];
> -
> -	return level;
> +	return connector->panel.backlight.level;
>   }
>   
> -/*
> - * Sends the current backlight level over the aux channel, checking if its using
> - * 8-bit or 16 bit value (MSB and LSB)
> - */
>   static void
> -intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
> -				u32 level)
> +intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state, u32 level)
>   {
>   	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	int ret;
> -	u8 vals[2] = { 0x0 };
> -
> -	/* Write the MSB and/or LSB */
> -	if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
> -		vals[0] = (level & 0xFF00) >> 8;
> -		vals[1] = (level & 0xFF);
> -	} else {
> -		vals[0] = level;
> -	}
> -
> -	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
> -				sizeof(vals));
> -	if (ret != sizeof(vals)) {
> -		drm_dbg_kms(&i915->drm, "Failed to write aux backlight level: %d\n", ret);
> -		return;
> -	}
> -}
> -
> -static void set_vesa_backlight_enable(struct intel_connector *connector, bool enable)
> -{
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	int ret;
> -	u8 reg_val = 0;
> -
> -	/* Early return when display use other mechanism to enable backlight. */
> -	if (!connector->panel.backlight.edp.vesa.aux_enable)
> -		return;
> -
> -	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read eDP display control register: %d\n", ret);
> -		return;
> -	}
> -
> -	if (enable)
> -		reg_val |= DP_EDP_BACKLIGHT_ENABLE;
> -	else
> -		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> +	struct intel_panel *panel = &connector->panel;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>   
> -	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to %s aux backlight: %d\n",
> -			    enable ? "enable" : "disable", ret);
> -	}
> +	drm_edp_backlight_set_level(&intel_dp->aux, &panel->backlight.edp.vesa.info, level);
>   }
>   
>   static void
> @@ -385,170 +290,46 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>   				   const struct drm_connector_state *conn_state, u32 level)
>   {
>   	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>   	struct intel_panel *panel = &connector->panel;
> -	int ret;
> -	u8 dpcd_buf, new_dpcd_buf;
> -	u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
> -
> -	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read backlight mode: %d\n", ret);
> -		return;
> -	}
> -
> -	new_dpcd_buf = dpcd_buf;
> -
> -	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {
> -		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> -		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> -
> -		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pwmgen_bit_count);
> -		if (ret != 1)
> -			drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit count: %d\n", ret);
> -	}
> -
> -	if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
> -		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
> -					 panel->backlight.edp.vesa.pwm_freq_pre_divider);
> -		if (ret == 1)
> -			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> -		else
> -			drm_dbg_kms(&i915->drm, "Failed to write aux backlight frequency: %d\n",
> -				    ret);
> -	}
> -
> -	if (new_dpcd_buf != dpcd_buf) {
> -		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> -					 new_dpcd_buf);
> -		if (ret != 1)
> -			drm_dbg_kms(&i915->drm, "Failed to write aux backlight mode: %d\n", ret);
> -	}
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>   
> -	intel_dp_aux_vesa_set_backlight(conn_state, level);
> -	set_vesa_backlight_enable(connector, true);
> +	drm_edp_backlight_enable(&intel_dp->aux, &panel->backlight.edp.vesa.info, level);
>   }
>   
>   static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state *old_conn_state,
>   						u32 level)
>   {
> -	set_vesa_backlight_enable(to_intel_connector(old_conn_state->connector), false);
> -}
> -
> -/*
> - * Compute PWM frequency divider value based off the frequency provided to us by the vbt.
> - * The PWM Frequency is calculated as 27Mhz / (F x P).
> - * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
> - *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> - * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
> - *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> - */
> -static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connector)
> -{
> -	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>   	struct intel_panel *panel = &connector->panel;
> -	u32 max_backlight = 0;
> -	int ret, freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> -	u8 pn, pn_min, pn_max;
> -
> -	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap: %d\n", ret);
> -		return 0;
> -	}
> -
> -	pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> -	max_backlight = (1 << pn) - 1;
> -
> -	/* Find desired value of (F x P)
> -	 * Note that, if F x P is out of supported range, the maximum value or
> -	 * minimum value will applied automatically. So no need to check that.
> -	 */
> -	freq = i915->vbt.backlight.pwm_freq_hz;
> -	drm_dbg_kms(&i915->drm, "VBT defined backlight frequency %u Hz\n",
> -		    freq);
> -	if (!freq) {
> -		drm_dbg_kms(&i915->drm,
> -			    "Use panel default backlight frequency\n");
> -		return max_backlight;
> -	}
> -
> -	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
> -
> -	/* Use highest possible value of Pn for more granularity of brightness
> -	 * adjustment while satifying the conditions below.
> -	 * - Pn is in the range of Pn_min and Pn_max
> -	 * - F is in the range of 1 and 255
> -	 * - FxP is within 25% of desired value.
> -	 *   Note: 25% is arbitrary value and may need some tweak.
> -	 */
> -	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap min: %d\n", ret);
> -		return max_backlight;
> -	}
> -	ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap max: %d\n", ret);
> -		return max_backlight;
> -	}
> -	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> -	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> -
> -	/* Ensure frequency is within 25% of desired value */
> -	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> -	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> -
> -	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> -		drm_dbg_kms(&i915->drm,
> -			    "VBT defined backlight frequency out of range\n");
> -		return max_backlight;
> -	}
> -
> -	for (pn = pn_max; pn >= pn_min; pn--) {
> -		f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> -		fxp_actual = f << pn;
> -		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> -			break;
> -	}
> -
> -	drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
> -	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> -	if (ret != 1) {
> -		drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit count: %d\n", ret);
> -		return max_backlight;
> -	}
> -
> -	panel->backlight.edp.vesa.pwmgen_bit_count = pn;
> -	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> -		panel->backlight.edp.vesa.pwm_freq_pre_divider = f;
> -
> -	max_backlight = (1 << pn) - 1;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>   
> -	return max_backlight;
> +	drm_edp_backlight_disable(&intel_dp->aux, &panel->backlight.edp.vesa.info);
>   }
>   
> -static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
> -					     enum pipe pipe)
> +static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector, enum pipe pipe)
>   {
>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>   	struct intel_panel *panel = &connector->panel;
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u16 current_level;
> +	u8 current_mode;
> +	int ret;
>   
> -	if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> -		panel->backlight.edp.vesa.aux_enable = true;
> -	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> -		panel->backlight.edp.vesa.lsb_reg_used = true;
> -
> -	panel->backlight.max = intel_dp_aux_vesa_calc_max_backlight(connector);
> -	if (!panel->backlight.max)
> -		return -ENODEV;
> +	ret = drm_edp_backlight_init(&intel_dp->aux, &panel->backlight.edp.vesa.info,
> +				     i915->vbt.backlight.pwm_freq_hz, intel_dp->edp_dpcd,
> +				     &current_level, &current_mode);
> +	if (ret < 0)
> +		return ret;
>   
> +	panel->backlight.max = panel->backlight.edp.vesa.info.max;
>   	panel->backlight.min = 0;
> -	panel->backlight.level = intel_dp_aux_vesa_get_backlight(connector, pipe);
> -	panel->backlight.enabled = intel_dp_aux_vesa_backlight_dpcd_mode(connector) &&
> -				   panel->backlight.level != 0;
> +	if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> +		panel->backlight.level = current_level;
> +		panel->backlight.enabled = panel->backlight.level != 0;
> +	} else {
> +		panel->backlight.level = panel->backlight.max;
> +		panel->backlight.enabled = false;
> +	}
>   
>   	return 0;
>   }
> @@ -559,16 +340,12 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>   	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>   
> -	/* Check the eDP Display control capabilities registers to determine if
> -	 * the panel can support backlight control over the aux channel.
> -	 *
> -	 * TODO: We currently only support AUX only backlight configurations, not backlights which
> +	/* TODO: We currently only support AUX only backlight configurations, not backlights which
>   	 * require a mix of PWM and AUX controls to work. In the mean time, these machines typically
>   	 * work just fine using normal PWM controls anyway.
>   	 */
> -	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> -	    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> -	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
> +	if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> +	    drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
>   		drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n");
>   		return true;
>   	}
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index edffd1dcca3e..1eca0b42fc45 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1790,6 +1790,24 @@ drm_dp_sink_can_do_video_without_timing_msa(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>   		DP_MSA_TIMING_PAR_IGNORED;
>   }
>   
> +/**
> + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support
> + * @edp_dpcd: The DPCD to check
> + *
> + * Note that currently this function will return %false for panels which support various DPCD
> + * backlight features but which require the brightness be set through PWM, and don't support setting
> + * the brightness level via the DPCD. This is a TODO.
> + *
> + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false
> + * otherwise
> + */
> +static inline bool
> +drm_edp_backlight_supported(const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
> +{
> +	return (edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +		(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> +}
> +
>   /*
>    * DisplayPort AUX channel
>    */
> @@ -2089,6 +2107,36 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
>   	return desc->quirks & BIT(quirk);
>   }
>   
> +/**
> + * struct drm_edp_backlight_info - Probed eDP backlight info struct
> + * @pwmgen_bit_count: The pwmgen bit count
> + * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used for this backlight, if any
> + * @max: The maximum backlight level that may be set
> + * @lsb_reg_used: Do we also write values to the DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register?
> + * @aux_enable: Does the panel support the AUX enable cap?
> + *
> + * This structure contains various data about an eDP backlight, which can be populated by using
> + * drm_edp_backlight_init().
> + */
> +struct drm_edp_backlight_info {
> +	u8 pwmgen_bit_count;
> +	u8 pwm_freq_pre_divider;
> +	u16 max;
> +
> +	bool lsb_reg_used : 1;
> +	bool aux_enable : 1;
> +};
> +
> +int
> +drm_edp_backlight_init(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
> +		       u16 driver_pwm_freq_hz, const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> +		       u16 *current_level, u8 *current_mode);
> +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
> +				u16 level);
> +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl,
> +			     u16 level);
> +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct drm_edp_backlight_info *bl);
> +
>   #ifdef CONFIG_DRM_DP_CEC
>   void drm_dp_cec_irq(struct drm_dp_aux *aux);
>   void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC v3 10/10] drm/dp: Extract i915's eDP backlight code into DRM helpers

[Lyude Paul]

On Mon, 2021-02-08 at 09:46 +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 06.02.21 um 00:45 schrieb Lyude Paul:
> > Since we're about to implement eDP backlight support in nouveau using the
> > standard protocol from VESA, we might as well just take the code that's
> > already written for this and move it into a set of shared DRM helpers.
> > 
> > Note that these helpers are intended to handle DPCD related backlight
> > control bits such as setting the brightness level over AUX, probing the
> > backlight's TCON, enabling/disabling the backlight over AUX if supported,
> > etc. Any PWM-related portions of backlight control are explicitly left up
> > to the driver, as these will vary from platform to platform.
> > 
> > The only exception to this is the calculation of the PWM frequency
> > pre-divider value. This is because the only platform-specific information
> > required for this is the PWM frequency of the panel, which the driver is
> > expected to provide if available. The actual algorithm for calculating this
> > value is standard and is defined in the eDP specification from VESA.
> > 
> > Note that these helpers do not yet implement the full range of features
> > the VESA backlight interface provides, and only provide the following
> > functionality (all of which was already present in i915's DPCD backlight
> > support):
> > 
> > * Basic control of brightness levels
> > * Basic probing of backlight capabilities
> > * Helpers for enabling and disabling the backlight
> > 
> > v3:
> > * Split out changes to i915's backlight code to separate patches to make it
> >    easier to review
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Dave Airlie <airlied@gmail.com>
> > Cc: greg.depoire@gmail.com
> > ---
> >   drivers/gpu/drm/drm_dp_helper.c               | 332 ++++++++++++++++++
> >   .../drm/i915/display/intel_display_types.h    |   5 +-
> >   .../drm/i915/display/intel_dp_aux_backlight.c | 285 ++-------------
> >   include/drm/drm_dp_helper.h                   |  48 +++
> >   4 files changed, 412 insertions(+), 258 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index eedbb48815b7..04cb2b6970a8 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -3082,3 +3082,335 @@ int drm_dp_pcon_convert_rgb_to_ycbcr(struct
> > drm_dp_aux *aux, u8 color_spc)
> >         return 0;
> >   }
> >   EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
> > +
> > +/**
> > + * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel
> > via AUX
> > + * @aux: The DP AUX channel to use
> > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > + * @level: The brightness level to set
> > + *
> > + * Sets the brightness level of an eDP panel's backlight. Note that the
> > panel's backlight must
> > + * already have been enabled by the driver by calling
> > drm_edp_backlight_enable().
> > + *
> > + * Returns: %0 on success, negative error code on failure
> > + */
> > +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > +                               u16 level)
> > +{
> > +       int ret;
> > +       u8 buf[2] = { 0 };
> > +
> > +       if (bl->lsb_reg_used) {
> > +               buf[0] = (level & 0xFF00) >> 8;
> > +               buf[1] = (level & 0x00FF);
> 
> Maybe 0x00ff and 0xff00 for aesthetic reasons.
> 
> > +       } else {
> > +               buf[0] = level;
> > +       }
> > +
> > +       ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf,
> > sizeof(buf));
> > +       if (ret != sizeof(buf)) {
> > +               DRM_ERROR("%s: Failed to write aux backlight level: %d\n",
> > aux->name, ret);
> 
> Since you're adding this code, you should probably convert to drm_err() 
> helpers as well. Here and elsewhere.

this is next up on my todo list JFYI-I don't do it right now because there isn't
actually any backpointer to the drm driver (and you can't just use the parent of
the aux device, since that technically doesn't need to be the drm driver).

I'd add it in this series, but that's going to involve updating functions across
the tree like drm_dp_aux_init() so I'd like to do it in a different patch series

> 
> > +               return ret < 0 ? ret : -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(drm_edp_backlight_set_level);
> > +
> > +static int
> > +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > +                            bool enable)
> > +{
> > +       int ret;
> > +       u8 buf;
> > +
> > +       /* The panel uses something other then DPCD for enabling it's
> > backlight */
> 
> 'its'
> 
> Best regards
> Thomas
> 
> > +       if (!bl->aux_enable)
> > +               return 0;
> > +
> > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &buf);
> > +       if (ret != 1) {
> > +               DRM_ERROR("%s: Failed to read eDP display control register:
> > %d\n", aux->name, ret);
> > +               return ret < 0 ? ret : -EIO;
> > +       }
> > +       if (enable)
> > +               buf |= DP_EDP_BACKLIGHT_ENABLE;
> > +       else
> > +               buf &= ~DP_EDP_BACKLIGHT_ENABLE;
> > +
> > +       ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
> > +       if (ret != 1) {
> > +               DRM_ERROR("%s: Failed to write eDP display control register:
> > %d\n", aux->name, ret);
> > +               return ret < 0 ? ret : -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * drm_edp_backlight_enable() - Enable an eDP panel's backlight using DPCD
> > + * @aux: The DP AUX channel to use
> > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > + * @level: The initial backlight level to set via AUX, if there is one
> > + *
> > + * This function handles enabling DPCD backlight controls on a panel over
> > DPCD, while additionally
> > + * restoring any important backlight state such as the given backlight
> > level, the brightness byte
> > + * count, backlight frequency, etc.
> > + *
> > + * Note that certain panels, while supporting brightness level controls
> > over DPCD, may not support
> > + * having their backlights enabled via the standard
> > %DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
> > + * &drm_edp_backlight_info.aux_enable will be set to %false, this function
> > will skip the step of
> > + * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must
> > perform the required
> > + * implementation specific step for enabling the backlight after calling
> > this function.
> > + *
> > + * Returns: %0 on success, negative error code on failure.
> > + */
> > +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > +                            const u16 level)
> > +{
> > +       int ret;
> > +       u8 dpcd_buf, new_dpcd_buf;
> > +
> > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > &dpcd_buf);
> > +       if (ret != 1) {
> > +               DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n",
> > aux->name, ret);
> > +               return ret < 0 ? ret : -EIO;
> > +       }
> > +
> > +       new_dpcd_buf = dpcd_buf;
> > +
> > +       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > +               new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > +               new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > +
> > +               ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, bl-
> > >pwmgen_bit_count);
> > +               if (ret != 1)
> > +                       DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit
> > count: %d\n",
> > +                                     aux->name, ret);
> > +       }
> > +
> > +       if (bl->pwm_freq_pre_divider) {
> > +               ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_FREQ_SET, bl-
> > >pwm_freq_pre_divider);
> > +               if (ret != 1)
> > +                       DRM_DEBUG_KMS("%s: Failed to write aux backlight
> > frequency: %d\n",
> > +                                     aux->name, ret);
> > +               else
> > +                       new_dpcd_buf |=
> > DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> > +       }
> > +
> > +       if (new_dpcd_buf != dpcd_buf) {
> > +               ret = drm_dp_dpcd_writeb(aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> > +               if (ret != 1) {
> > +                       DRM_DEBUG_KMS("%s: Failed to write aux backlight
> > mode: %d\n",
> > +                                     aux->name, ret);
> > +                       return ret < 0 ? ret : -EIO;
> > +               }
> > +       }
> > +
> > +       ret = drm_edp_backlight_set_level(aux, bl, level);
> > +       if (ret < 0)
> > +               return ret;
> > +       ret = drm_edp_backlight_set_enable(aux, bl, true);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(drm_edp_backlight_enable);
> > +
> > +/**
> > + * drm_edp_backlight_disable() - Disable an eDP backlight using DPCD, if
> > supported
> > + * @aux: The DP AUX channel to use
> > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > + *
> > + * This function handles disabling DPCD backlight controls on a panel over
> > AUX. Note that some
> > + * panels have backlights that are enabled/disabled by other means, despite
> > having their brightness
> > + * values controlled through DPCD. On such panels
> > &drm_edp_backlight_info.aux_enable will be set to
> > + * %false, this function will become a no-op (and we will skip updating
> > + * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to
> > perform it's own
> > + * implementation specific step for disabling the backlight.
> > + *
> > + * Returns: %0 on success or no-op, negative error code on failure.
> > + */
> > +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl)
> > +{
> > +       int ret;
> > +
> > +       ret = drm_edp_backlight_set_enable(aux, bl, false);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(drm_edp_backlight_disable);
> > +
> > +static inline int
> > +drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct
> > drm_edp_backlight_info *bl,
> > +                           u16 driver_pwm_freq_hz, const u8
> > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
> > +{
> > +       int fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> > +       int ret;
> > +       u8 pn, pn_min, pn_max;
> > +
> > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
> > +       if (ret != 1) {
> > +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap:
> > %d\n", aux->name, ret);
> > +               return -ENODEV;
> > +       }
> > +
> > +       pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +       bl->max = (1 << pn) - 1;
> > +       if (!driver_pwm_freq_hz)
> > +               return 0;
> > +
> > +       /*
> > +        * Set PWM Frequency divider to match desired frequency provided by
> > the driver.
> > +        * The PWM Frequency is calculated as 27Mhz / (F x P).
> > +        * - Where F = PWM Frequency Pre-Divider value programmed by field
> > 7:0 of the
> > +        *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > +        * - Where P = 2^Pn, where Pn is the value programmed by field 4:0
> > of the
> > +        *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > +        */
> > +
> > +       /* Find desired value of (F x P)
> > +        * Note that, if F x P is out of supported range, the maximum value
> > or minimum value will
> > +        * applied automatically. So no need to check that.
> > +        */
> > +       fxp = DIV_ROUND_CLOSEST(1000 * DP_EDP_BACKLIGHT_FREQ_BASE_KHZ,
> > driver_pwm_freq_hz);
> > +
> > +       /* Use highest possible value of Pn for more granularity of
> > brightness adjustment while
> > +        * satifying the conditions below.
> > +        * - Pn is in the range of Pn_min and Pn_max
> > +        * - F is in the range of 1 and 255
> > +        * - FxP is within 25% of desired value.
> > +        *   Note: 25% is arbitrary value and may need some tweak.
> > +        */
> > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> > &pn_min);
> > +       if (ret != 1) {
> > +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap min:
> > %d\n", aux->name, ret);
> > +               return 0;
> > +       }
> > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
> > &pn_max);
> > +       if (ret != 1) {
> > +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap max:
> > %d\n", aux->name, ret);
> > +               return 0;
> > +       }
> > +       pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +       pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +
> > +       /* Ensure frequency is within 25% of desired value */
> > +       fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > +       fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > +       if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > +               DRM_DEBUG_KMS("%s: Driver defined backlight frequency (%d)
> > out of range\n",
> > +                             aux->name, driver_pwm_freq_hz);
> > +               return 0;
> > +       }
> > +
> > +       for (pn = pn_max; pn >= pn_min; pn--) {
> > +               f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> > +               fxp_actual = f << pn;
> > +               if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > +                       break;
> > +       }
> > +
> > +       ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> > +       if (ret != 1) {
> > +               DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count:
> > %d\n", aux->name, ret);
> > +               return 0;
> > +       }
> > +       bl->pwmgen_bit_count = pn;
> > +       bl->max = (1 << pn) - 1;
> > +
> > +       if (edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) {
> > +               bl->pwm_freq_pre_divider = f;
> > +               DRM_DEBUG_KMS("%s: Using backlight frequency from driver
> > (%dHz)\n",
> > +                             aux->name, driver_pwm_freq_hz);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static inline int
> > +drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct
> > drm_edp_backlight_info *bl,
> > +                             u8 *current_mode)
> > +{
> > +       int ret;
> > +       u8 buf[2];
> > +       u8 mode_reg;
> > +
> > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > &mode_reg);
> > +       if (ret != 1) {
> > +               DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n",
> > aux->name, ret);
> > +               return ret < 0 ? ret : -EIO;
> > +       }
> > +
> > +       *current_mode = (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK);
> > +       if (*current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > +               int size = 1 + bl->lsb_reg_used;
> > +
> > +               ret = drm_dp_dpcd_read(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > buf, size);
> > +               if (ret != size) {
> > +                       DRM_DEBUG_KMS("%s: Failed to read backlight level:
> > %d\n", aux->name, ret);
> > +                       return ret < 0 ? ret : -EIO;
> > +               }
> > +
> > +               if (bl->lsb_reg_used)
> > +                       return (buf[0] << 8) | buf[1];
> > +               else
> > +                       return buf[0];
> > +       }
> > +
> > +       /*
> > +        * If we're not in DPCD control mode yet, the programmed brightness
> > value is meaningless and
> > +        * the driver should assume max brightness
> > +        */
> > +       return bl->max;
> > +}
> > +
> > +/**
> > + * drm_edp_backlight_init() - Probe a display panel's TCON using the
> > standard VESA eDP backlight
> > + * interface.
> > + * @aux: The DP aux device to use for probing
> > + * @bl: The &drm_edp_backlight_info struct to fill out with information on
> > the backlight
> > + * @driver_pwm_freq_hz: Optional PWM frequency from the driver in hz
> > + * @edp_dpcd: A cached copy of the eDP DPCD
> > + * @current_level: Where to store the probed brightness level
> > + * @current_mode: Where to store the currently set backlight control mode
> > + *
> > + * Initializes a &drm_edp_backlight_info struct by probing @aux for it's
> > backlight capabilities,
> > + * along with also probing the current and maximum supported brightness
> > levels.
> > + *
> > + * If @driver_pwm_freq_hz is non-zero, this will be used as the backlight
> > frequency. Otherwise, the
> > + * default frequency from the panel is used.
> > + *
> > + * Returns: %0 on success, negative error code on failure.
> > + */
> > +int
> > +drm_edp_backlight_init(struct drm_dp_aux *aux, struct
> > drm_edp_backlight_info *bl,
> > +                      u16 driver_pwm_freq_hz, const u8
> > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> > +                      u16 *current_level, u8 *current_mode)
> > +{
> > +       int ret;
> > +
> > +       if (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> > +               bl->aux_enable = true;
> > +       if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> > +               bl->lsb_reg_used = true;
> > +
> > +       ret = drm_edp_backlight_probe_max(aux, bl, driver_pwm_freq_hz,
> > edp_dpcd);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = drm_edp_backlight_probe_level(aux, bl, current_mode);
> > +       if (ret < 0)
> > +               return ret;
> > +       *current_level = ret;
> > +
> > +       DRM_DEBUG_KMS("%s: Found backlight level=%d/%d
> > pwm_freq_pre_divider=%d mode=%x\n",
> > +                     aux->name, *current_level, bl->max, bl-
> > >pwm_freq_pre_divider, *current_mode);
> > +       DRM_DEBUG_KMS("%s: Backlight caps: pwmgen_bit_count=%d
> > lsb_reg_used=%d aux_enable=%d\n",
> > +                     aux->name, bl->pwmgen_bit_count, bl->lsb_reg_used, bl-
> > >aux_enable);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(drm_edp_backlight_init);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 16824eb3ef93..03051ab75d30 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -263,10 +263,7 @@ struct intel_panel {
> >                 /* DPCD backlight */
> >                 union {
> >                         struct {
> > -                               u8 pwmgen_bit_count;
> > -                               u8 pwm_freq_pre_divider;
> > -                               bool lsb_reg_used;
> > -                               bool aux_enable;
> > +                               struct drm_edp_backlight_info info;
> >                         } vesa;
> >                         struct {
> >                                 bool sdr_uses_aux;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > index 813f6c553156..286eb337448e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > @@ -270,114 +270,19 @@ intel_dp_aux_hdr_setup_backlight(struct
> > intel_connector *connector, enum pipe pi
> >   }
> >   
> >   /* VESA backlight callbacks */
> > -static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector
> > *connector)
> > -{
> > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -       int ret;
> > -       u8 mode_reg;
> > -
> > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read backlight mode:
> > %d\n", ret);
> > -               return false;
> > -       }
> > -
> > -       return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > -              DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > -}
> > -
> > -/*
> > - * Read the current backlight value from DPCD register(s) based
> > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > - */
> >   static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector
> > *connector, enum pipe unused)
> >   {
> > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -       int ret;
> > -       u8 read_val[2] = { 0x0 };
> > -       u16 level = 0;
> > -
> > -       /*
> > -        * If we're not in DPCD control mode yet, the programmed brightness
> > -        * value is meaningless and we should assume max brightness
> > -        */
> > -       if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
> > -               return connector->panel.backlight.max;
> > -
> > -       ret = drm_dp_dpcd_read(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
> > -                              sizeof(read_val));
> > -       if (ret != sizeof(read_val)) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read brightness level:
> > %d\n", ret);
> > -               return 0;
> > -       }
> > -
> > -       if (connector->panel.backlight.edp.vesa.lsb_reg_used)
> > -               level = (read_val[0] << 8 | read_val[1]);
> > -       else
> > -               level = read_val[0];
> > -
> > -       return level;
> > +       return connector->panel.backlight.level;
> >   }
> >   
> > -/*
> > - * Sends the current backlight level over the aux channel, checking if its
> > using
> > - * 8-bit or 16 bit value (MSB and LSB)
> > - */
> >   static void
> > -intel_dp_aux_vesa_set_backlight(const struct drm_connector_state
> > *conn_state,
> > -                               u32 level)
> > +intel_dp_aux_vesa_set_backlight(const struct drm_connector_state
> > *conn_state, u32 level)
> >   {
> >         struct intel_connector *connector = to_intel_connector(conn_state-
> > >connector);
> > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -       int ret;
> > -       u8 vals[2] = { 0x0 };
> > -
> > -       /* Write the MSB and/or LSB */
> > -       if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
> > -               vals[0] = (level & 0xFF00) >> 8;
> > -               vals[1] = (level & 0xFF);
> > -       } else {
> > -               vals[0] = level;
> > -       }
> > -
> > -       ret = drm_dp_dpcd_write(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
> > -                               sizeof(vals));
> > -       if (ret != sizeof(vals)) {
> > -               drm_dbg_kms(&i915->drm, "Failed to write aux backlight
> > level: %d\n", ret);
> > -               return;
> > -       }
> > -}
> > -
> > -static void set_vesa_backlight_enable(struct intel_connector *connector,
> > bool enable)
> > -{
> > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -       int ret;
> > -       u8 reg_val = 0;
> > -
> > -       /* Early return when display use other mechanism to enable
> > backlight. */
> > -       if (!connector->panel.backlight.edp.vesa.aux_enable)
> > -               return;
> > -
> > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read eDP display control
> > register: %d\n", ret);
> > -               return;
> > -       }
> > -
> > -       if (enable)
> > -               reg_val |= DP_EDP_BACKLIGHT_ENABLE;
> > -       else
> > -               reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> > +       struct intel_panel *panel = &connector->panel;
> > +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> >   
> > -       ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to %s aux backlight: %d\n",
> > -                           enable ? "enable" : "disable", ret);
> > -       }
> > +       drm_edp_backlight_set_level(&intel_dp->aux, &panel-
> > >backlight.edp.vesa.info, level);
> >   }
> >   
> >   static void
> > @@ -385,170 +290,46 @@ intel_dp_aux_vesa_enable_backlight(const struct
> > intel_crtc_state *crtc_state,
> >                                    const struct drm_connector_state
> > *conn_state, u32 level)
> >   {
> >         struct intel_connector *connector = to_intel_connector(conn_state-
> > >connector);
> > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >         struct intel_panel *panel = &connector->panel;
> > -       int ret;
> > -       u8 dpcd_buf, new_dpcd_buf;
> > -       u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
> > -
> > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read backlight mode:
> > %d\n", ret);
> > -               return;
> > -       }
> > -
> > -       new_dpcd_buf = dpcd_buf;
> > -
> > -       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {
> > -               new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > -               new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > -
> > -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_EDP_PWMGEN_BIT_COUNT, pwmgen_bit_count);
> > -               if (ret != 1)
> > -                       drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen
> > bit count: %d\n", ret);
> > -       }
> > -
> > -       if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
> > -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_FREQ_SET,
> > -                                        panel-
> > >backlight.edp.vesa.pwm_freq_pre_divider);
> > -               if (ret == 1)
> > -                       new_dpcd_buf |=
> > DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> > -               else
> > -                       drm_dbg_kms(&i915->drm, "Failed to write aux
> > backlight frequency: %d\n",
> > -                                   ret);
> > -       }
> > -
> > -       if (new_dpcd_buf != dpcd_buf) {
> > -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > -                                        new_dpcd_buf);
> > -               if (ret != 1)
> > -                       drm_dbg_kms(&i915->drm, "Failed to write aux
> > backlight mode: %d\n", ret);
> > -       }
> > +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> >   
> > -       intel_dp_aux_vesa_set_backlight(conn_state, level);
> > -       set_vesa_backlight_enable(connector, true);
> > +       drm_edp_backlight_enable(&intel_dp->aux, &panel-
> > >backlight.edp.vesa.info, level);
> >   }
> >   
> >   static void intel_dp_aux_vesa_disable_backlight(const struct
> > drm_connector_state *old_conn_state,
> >                                                 u32 level)
> >   {
> > -       set_vesa_backlight_enable(to_intel_connector(old_conn_state-
> > >connector), false);
> > -}
> > -
> > -/*
> > - * Compute PWM frequency divider value based off the frequency provided to
> > us by the vbt.
> > - * The PWM Frequency is calculated as 27Mhz / (F x P).
> > - * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of
> > the
> > - *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > - * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
> > - *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > - */
> > -static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector
> > *connector)
> > -{
> > -       struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > +       struct intel_connector *connector =
> > to_intel_connector(old_conn_state->connector);
> >         struct intel_panel *panel = &connector->panel;
> > -       u32 max_backlight = 0;
> > -       int ret, freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> > -       u8 pn, pn_min, pn_max;
> > -
> > -       ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
> > &pn);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count
> > cap: %d\n", ret);
> > -               return 0;
> > -       }
> > -
> > -       pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > -       max_backlight = (1 << pn) - 1;
> > -
> > -       /* Find desired value of (F x P)
> > -        * Note that, if F x P is out of supported range, the maximum value
> > or
> > -        * minimum value will applied automatically. So no need to check
> > that.
> > -        */
> > -       freq = i915->vbt.backlight.pwm_freq_hz;
> > -       drm_dbg_kms(&i915->drm, "VBT defined backlight frequency %u Hz\n",
> > -                   freq);
> > -       if (!freq) {
> > -               drm_dbg_kms(&i915->drm,
> > -                           "Use panel default backlight frequency\n");
> > -               return max_backlight;
> > -       }
> > -
> > -       fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
> > -
> > -       /* Use highest possible value of Pn for more granularity of
> > brightness
> > -        * adjustment while satifying the conditions below.
> > -        * - Pn is in the range of Pn_min and Pn_max
> > -        * - F is in the range of 1 and 255
> > -        * - FxP is within 25% of desired value.
> > -        *   Note: 25% is arbitrary value and may need some tweak.
> > -        */
> > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap
> > min: %d\n", ret);
> > -               return max_backlight;
> > -       }
> > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap
> > max: %d\n", ret);
> > -               return max_backlight;
> > -       }
> > -       pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > -       pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > -
> > -       /* Ensure frequency is within 25% of desired value */
> > -       fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > -       fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > -
> > -       if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > -               drm_dbg_kms(&i915->drm,
> > -                           "VBT defined backlight frequency out of
> > range\n");
> > -               return max_backlight;
> > -       }
> > -
> > -       for (pn = pn_max; pn >= pn_min; pn--) {
> > -               f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> > -               fxp_actual = f << pn;
> > -               if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > -                       break;
> > -       }
> > -
> > -       drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
> > -       ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
> > pn);
> > -       if (ret != 1) {
> > -               drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit
> > count: %d\n", ret);
> > -               return max_backlight;
> > -       }
> > -
> > -       panel->backlight.edp.vesa.pwmgen_bit_count = pn;
> > -       if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> > -               panel->backlight.edp.vesa.pwm_freq_pre_divider = f;
> > -
> > -       max_backlight = (1 << pn) - 1;
> > +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> >   
> > -       return max_backlight;
> > +       drm_edp_backlight_disable(&intel_dp->aux, &panel-
> > >backlight.edp.vesa.info);
> >   }
> >   
> > -static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
> > *connector,
> > -                                            enum pipe pipe)
> > +static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
> > *connector, enum pipe pipe)
> >   {
> >         struct intel_dp *intel_dp = intel_attached_dp(connector);
> >         struct intel_panel *panel = &connector->panel;
> > +       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +       u16 current_level;
> > +       u8 current_mode;
> > +       int ret;
> >   
> > -       if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> > -               panel->backlight.edp.vesa.aux_enable = true;
> > -       if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> > -               panel->backlight.edp.vesa.lsb_reg_used = true;
> > -
> > -       panel->backlight.max =
> > intel_dp_aux_vesa_calc_max_backlight(connector);
> > -       if (!panel->backlight.max)
> > -               return -ENODEV;
> > +       ret = drm_edp_backlight_init(&intel_dp->aux, &panel-
> > >backlight.edp.vesa.info,
> > +                                    i915->vbt.backlight.pwm_freq_hz,
> > intel_dp->edp_dpcd,
> > +                                    &current_level, &current_mode);
> > +       if (ret < 0)
> > +               return ret;
> >   
> > +       panel->backlight.max = panel->backlight.edp.vesa.info.max;
> >         panel->backlight.min = 0;
> > -       panel->backlight.level = intel_dp_aux_vesa_get_backlight(connector,
> > pipe);
> > -       panel->backlight.enabled =
> > intel_dp_aux_vesa_backlight_dpcd_mode(connector) &&
> > -                                  panel->backlight.level != 0;
> > +       if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > +               panel->backlight.level = current_level;
> > +               panel->backlight.enabled = panel->backlight.level != 0;
> > +       } else {
> > +               panel->backlight.level = panel->backlight.max;
> > +               panel->backlight.enabled = false;
> > +       }
> >   
> >         return 0;
> >   }
> > @@ -559,16 +340,12 @@ intel_dp_aux_supports_vesa_backlight(struct
> > intel_connector *connector)
> >         struct intel_dp *intel_dp = intel_attached_dp(connector);
> >         struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >   
> > -       /* Check the eDP Display control capabilities registers to determine
> > if
> > -        * the panel can support backlight control over the aux channel.
> > -        *
> > -        * TODO: We currently only support AUX only backlight
> > configurations, not backlights which
> > +       /* TODO: We currently only support AUX only backlight
> > configurations, not backlights which
> >          * require a mix of PWM and AUX controls to work. In the mean time,
> > these machines typically
> >          * work just fine using normal PWM controls anyway.
> >          */
> > -       if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> > -           (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > -           (intel_dp->edp_dpcd[2] &
> > DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
> > +       if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > +           drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
> >                 drm_dbg_kms(&i915->drm, "AUX Backlight Control
> > Supported!\n");
> >                 return true;
> >         }
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index edffd1dcca3e..1eca0b42fc45 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1790,6 +1790,24 @@ drm_dp_sink_can_do_video_without_timing_msa(const u8
> > dpcd[DP_RECEIVER_CAP_SIZE])
> >                 DP_MSA_TIMING_PAR_IGNORED;
> >   }
> >   
> > +/**
> > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight
> > support
> > + * @edp_dpcd: The DPCD to check
> > + *
> > + * Note that currently this function will return %false for panels which
> > support various DPCD
> > + * backlight features but which require the brightness be set through PWM,
> > and don't support setting
> > + * the brightness level via the DPCD. This is a TODO.
> > + *
> > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are
> > supported, %false
> > + * otherwise
> > + */
> > +static inline bool
> > +drm_edp_backlight_supported(const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
> > +{
> > +       return (edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> > +               (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> > +}
> > +
> >   /*
> >    * DisplayPort AUX channel
> >    */
> > @@ -2089,6 +2107,36 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum
> > drm_dp_quirk quirk)
> >         return desc->quirks & BIT(quirk);
> >   }
> >   
> > +/**
> > + * struct drm_edp_backlight_info - Probed eDP backlight info struct
> > + * @pwmgen_bit_count: The pwmgen bit count
> > + * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used
> > for this backlight, if any
> > + * @max: The maximum backlight level that may be set
> > + * @lsb_reg_used: Do we also write values to the
> > DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register?
> > + * @aux_enable: Does the panel support the AUX enable cap?
> > + *
> > + * This structure contains various data about an eDP backlight, which can
> > be populated by using
> > + * drm_edp_backlight_init().
> > + */
> > +struct drm_edp_backlight_info {
> > +       u8 pwmgen_bit_count;
> > +       u8 pwm_freq_pre_divider;
> > +       u16 max;
> > +
> > +       bool lsb_reg_used : 1;
> > +       bool aux_enable : 1;
> > +};
> > +
> > +int
> > +drm_edp_backlight_init(struct drm_dp_aux *aux, struct
> > drm_edp_backlight_info *bl,
> > +                      u16 driver_pwm_freq_hz, const u8
> > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> > +                      u16 *current_level, u8 *current_mode);
> > +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > +                               u16 level);
> > +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > +                            u16 level);
> > +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl);
> > +
> >   #ifdef CONFIG_DRM_DP_CEC
> >   void drm_dp_cec_irq(struct drm_dp_aux *aux);
> >   void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > 
> 

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

Re: [RFC v3 10/10] drm/dp: Extract i915's eDP backlight code into DRM helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 42143 bytes --]

Hi

Am 09.02.21 um 00:03 schrieb Lyude Paul:
>>
>>> +       } else {
>>> +               buf[0] = level;
>>> +       }
>>> +
>>> +       ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf,
>>> sizeof(buf));
>>> +       if (ret != sizeof(buf)) {
>>> +               DRM_ERROR("%s: Failed to write aux backlight level: %d\n",
>>> aux->name, ret);
>>
>> Since you're adding this code, you should probably convert to drm_err()
>> helpers as well. Here and elsewhere.
> 
> this is next up on my todo list JFYI-I don't do it right now because there isn't
> actually any backpointer to the drm driver (and you can't just use the parent of
> the aux device, since that technically doesn't need to be the drm driver).
> 
> I'd add it in this series, but that's going to involve updating functions across
> the tree like drm_dp_aux_init() so I'd like to do it in a different patch series

Ok, sure. Makes sense.

Best regards
Thomas

> 
>>
>>> +               return ret < 0 ? ret : -EIO;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_edp_backlight_set_level);
>>> +
>>> +static int
>>> +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct
>>> drm_edp_backlight_info *bl,
>>> +                            bool enable)
>>> +{
>>> +       int ret;
>>> +       u8 buf;
>>> +
>>> +       /* The panel uses something other then DPCD for enabling it's
>>> backlight */
>>
>> 'its'
>>
>> Best regards
>> Thomas
>>
>>> +       if (!bl->aux_enable)
>>> +               return 0;
>>> +
>>> +       ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &buf);
>>> +       if (ret != 1) {
>>> +               DRM_ERROR("%s: Failed to read eDP display control register:
>>> %d\n", aux->name, ret);
>>> +               return ret < 0 ? ret : -EIO;
>>> +       }
>>> +       if (enable)
>>> +               buf |= DP_EDP_BACKLIGHT_ENABLE;
>>> +       else
>>> +               buf &= ~DP_EDP_BACKLIGHT_ENABLE;
>>> +
>>> +       ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
>>> +       if (ret != 1) {
>>> +               DRM_ERROR("%s: Failed to write eDP display control register:
>>> %d\n", aux->name, ret);
>>> +               return ret < 0 ? ret : -EIO;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +/**
>>> + * drm_edp_backlight_enable() - Enable an eDP panel's backlight using DPCD
>>> + * @aux: The DP AUX channel to use
>>> + * @bl: Backlight capability info from drm_edp_backlight_init()
>>> + * @level: The initial backlight level to set via AUX, if there is one
>>> + *
>>> + * This function handles enabling DPCD backlight controls on a panel over
>>> DPCD, while additionally
>>> + * restoring any important backlight state such as the given backlight
>>> level, the brightness byte
>>> + * count, backlight frequency, etc.
>>> + *
>>> + * Note that certain panels, while supporting brightness level controls
>>> over DPCD, may not support
>>> + * having their backlights enabled via the standard
>>> %DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
>>> + * &drm_edp_backlight_info.aux_enable will be set to %false, this function
>>> will skip the step of
>>> + * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must
>>> perform the required
>>> + * implementation specific step for enabling the backlight after calling
>>> this function.
>>> + *
>>> + * Returns: %0 on success, negative error code on failure.
>>> + */
>>> +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct
>>> drm_edp_backlight_info *bl,
>>> +                            const u16 level)
>>> +{
>>> +       int ret;
>>> +       u8 dpcd_buf, new_dpcd_buf;
>>> +
>>> +       ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>>> &dpcd_buf);
>>> +       if (ret != 1) {
>>> +               DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n",
>>> aux->name, ret);
>>> +               return ret < 0 ? ret : -EIO;
>>> +       }
>>> +
>>> +       new_dpcd_buf = dpcd_buf;
>>> +
>>> +       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
>>> DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
>>> +               new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>>> +               new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>>> +
>>> +               ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, bl-
>>>> pwmgen_bit_count);
>>> +               if (ret != 1)
>>> +                       DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit
>>> count: %d\n",
>>> +                                     aux->name, ret);
>>> +       }
>>> +
>>> +       if (bl->pwm_freq_pre_divider) {
>>> +               ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_FREQ_SET, bl-
>>>> pwm_freq_pre_divider);
>>> +               if (ret != 1)
>>> +                       DRM_DEBUG_KMS("%s: Failed to write aux backlight
>>> frequency: %d\n",
>>> +                                     aux->name, ret);
>>> +               else
>>> +                       new_dpcd_buf |=
>>> DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
>>> +       }
>>> +
>>> +       if (new_dpcd_buf != dpcd_buf) {
>>> +               ret = drm_dp_dpcd_writeb(aux,
>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>>> +               if (ret != 1) {
>>> +                       DRM_DEBUG_KMS("%s: Failed to write aux backlight
>>> mode: %d\n",
>>> +                                     aux->name, ret);
>>> +                       return ret < 0 ? ret : -EIO;
>>> +               }
>>> +       }
>>> +
>>> +       ret = drm_edp_backlight_set_level(aux, bl, level);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +       ret = drm_edp_backlight_set_enable(aux, bl, true);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_edp_backlight_enable);
>>> +
>>> +/**
>>> + * drm_edp_backlight_disable() - Disable an eDP backlight using DPCD, if
>>> supported
>>> + * @aux: The DP AUX channel to use
>>> + * @bl: Backlight capability info from drm_edp_backlight_init()
>>> + *
>>> + * This function handles disabling DPCD backlight controls on a panel over
>>> AUX. Note that some
>>> + * panels have backlights that are enabled/disabled by other means, despite
>>> having their brightness
>>> + * values controlled through DPCD. On such panels
>>> &drm_edp_backlight_info.aux_enable will be set to
>>> + * %false, this function will become a no-op (and we will skip updating
>>> + * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to
>>> perform it's own
>>> + * implementation specific step for disabling the backlight.
>>> + *
>>> + * Returns: %0 on success or no-op, negative error code on failure.
>>> + */
>>> +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
>>> drm_edp_backlight_info *bl)
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = drm_edp_backlight_set_enable(aux, bl, false);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_edp_backlight_disable);
>>> +
>>> +static inline int
>>> +drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct
>>> drm_edp_backlight_info *bl,
>>> +                           u16 driver_pwm_freq_hz, const u8
>>> edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
>>> +{
>>> +       int fxp, fxp_min, fxp_max, fxp_actual, f = 1;
>>> +       int ret;
>>> +       u8 pn, pn_min, pn_max;
>>> +
>>> +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
>>> +       if (ret != 1) {
>>> +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap:
>>> %d\n", aux->name, ret);
>>> +               return -ENODEV;
>>> +       }
>>> +
>>> +       pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>>> +       bl->max = (1 << pn) - 1;
>>> +       if (!driver_pwm_freq_hz)
>>> +               return 0;
>>> +
>>> +       /*
>>> +        * Set PWM Frequency divider to match desired frequency provided by
>>> the driver.
>>> +        * The PWM Frequency is calculated as 27Mhz / (F x P).
>>> +        * - Where F = PWM Frequency Pre-Divider value programmed by field
>>> 7:0 of the
>>> +        *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
>>> +        * - Where P = 2^Pn, where Pn is the value programmed by field 4:0
>>> of the
>>> +        *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
>>> +        */
>>> +
>>> +       /* Find desired value of (F x P)
>>> +        * Note that, if F x P is out of supported range, the maximum value
>>> or minimum value will
>>> +        * applied automatically. So no need to check that.
>>> +        */
>>> +       fxp = DIV_ROUND_CLOSEST(1000 * DP_EDP_BACKLIGHT_FREQ_BASE_KHZ,
>>> driver_pwm_freq_hz);
>>> +
>>> +       /* Use highest possible value of Pn for more granularity of
>>> brightness adjustment while
>>> +        * satifying the conditions below.
>>> +        * - Pn is in the range of Pn_min and Pn_max
>>> +        * - F is in the range of 1 and 255
>>> +        * - FxP is within 25% of desired value.
>>> +        *   Note: 25% is arbitrary value and may need some tweak.
>>> +        */
>>> +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
>>> &pn_min);
>>> +       if (ret != 1) {
>>> +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap min:
>>> %d\n", aux->name, ret);
>>> +               return 0;
>>> +       }
>>> +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
>>> &pn_max);
>>> +       if (ret != 1) {
>>> +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap max:
>>> %d\n", aux->name, ret);
>>> +               return 0;
>>> +       }
>>> +       pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>>> +       pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>>> +
>>> +       /* Ensure frequency is within 25% of desired value */
>>> +       fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
>>> +       fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
>>> +       if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
>>> +               DRM_DEBUG_KMS("%s: Driver defined backlight frequency (%d)
>>> out of range\n",
>>> +                             aux->name, driver_pwm_freq_hz);
>>> +               return 0;
>>> +       }
>>> +
>>> +       for (pn = pn_max; pn >= pn_min; pn--) {
>>> +               f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
>>> +               fxp_actual = f << pn;
>>> +               if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
>>> +                       break;
>>> +       }
>>> +
>>> +       ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
>>> +       if (ret != 1) {
>>> +               DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count:
>>> %d\n", aux->name, ret);
>>> +               return 0;
>>> +       }
>>> +       bl->pwmgen_bit_count = pn;
>>> +       bl->max = (1 << pn) - 1;
>>> +
>>> +       if (edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) {
>>> +               bl->pwm_freq_pre_divider = f;
>>> +               DRM_DEBUG_KMS("%s: Using backlight frequency from driver
>>> (%dHz)\n",
>>> +                             aux->name, driver_pwm_freq_hz);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static inline int
>>> +drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct
>>> drm_edp_backlight_info *bl,
>>> +                             u8 *current_mode)
>>> +{
>>> +       int ret;
>>> +       u8 buf[2];
>>> +       u8 mode_reg;
>>> +
>>> +       ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>>> &mode_reg);
>>> +       if (ret != 1) {
>>> +               DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n",
>>> aux->name, ret);
>>> +               return ret < 0 ? ret : -EIO;
>>> +       }
>>> +
>>> +       *current_mode = (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK);
>>> +       if (*current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
>>> +               int size = 1 + bl->lsb_reg_used;
>>> +
>>> +               ret = drm_dp_dpcd_read(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
>>> buf, size);
>>> +               if (ret != size) {
>>> +                       DRM_DEBUG_KMS("%s: Failed to read backlight level:
>>> %d\n", aux->name, ret);
>>> +                       return ret < 0 ? ret : -EIO;
>>> +               }
>>> +
>>> +               if (bl->lsb_reg_used)
>>> +                       return (buf[0] << 8) | buf[1];
>>> +               else
>>> +                       return buf[0];
>>> +       }
>>> +
>>> +       /*
>>> +        * If we're not in DPCD control mode yet, the programmed brightness
>>> value is meaningless and
>>> +        * the driver should assume max brightness
>>> +        */
>>> +       return bl->max;
>>> +}
>>> +
>>> +/**
>>> + * drm_edp_backlight_init() - Probe a display panel's TCON using the
>>> standard VESA eDP backlight
>>> + * interface.
>>> + * @aux: The DP aux device to use for probing
>>> + * @bl: The &drm_edp_backlight_info struct to fill out with information on
>>> the backlight
>>> + * @driver_pwm_freq_hz: Optional PWM frequency from the driver in hz
>>> + * @edp_dpcd: A cached copy of the eDP DPCD
>>> + * @current_level: Where to store the probed brightness level
>>> + * @current_mode: Where to store the currently set backlight control mode
>>> + *
>>> + * Initializes a &drm_edp_backlight_info struct by probing @aux for it's
>>> backlight capabilities,
>>> + * along with also probing the current and maximum supported brightness
>>> levels.
>>> + *
>>> + * If @driver_pwm_freq_hz is non-zero, this will be used as the backlight
>>> frequency. Otherwise, the
>>> + * default frequency from the panel is used.
>>> + *
>>> + * Returns: %0 on success, negative error code on failure.
>>> + */
>>> +int
>>> +drm_edp_backlight_init(struct drm_dp_aux *aux, struct
>>> drm_edp_backlight_info *bl,
>>> +                      u16 driver_pwm_freq_hz, const u8
>>> edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
>>> +                      u16 *current_level, u8 *current_mode)
>>> +{
>>> +       int ret;
>>> +
>>> +       if (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
>>> +               bl->aux_enable = true;
>>> +       if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>>> +               bl->lsb_reg_used = true;
>>> +
>>> +       ret = drm_edp_backlight_probe_max(aux, bl, driver_pwm_freq_hz,
>>> edp_dpcd);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +
>>> +       ret = drm_edp_backlight_probe_level(aux, bl, current_mode);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +       *current_level = ret;
>>> +
>>> +       DRM_DEBUG_KMS("%s: Found backlight level=%d/%d
>>> pwm_freq_pre_divider=%d mode=%x\n",
>>> +                     aux->name, *current_level, bl->max, bl-
>>>> pwm_freq_pre_divider, *current_mode);
>>> +       DRM_DEBUG_KMS("%s: Backlight caps: pwmgen_bit_count=%d
>>> lsb_reg_used=%d aux_enable=%d\n",
>>> +                     aux->name, bl->pwmgen_bit_count, bl->lsb_reg_used, bl-
>>>> aux_enable);
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_edp_backlight_init);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
>>> b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> index 16824eb3ef93..03051ab75d30 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> @@ -263,10 +263,7 @@ struct intel_panel {
>>>                  /* DPCD backlight */
>>>                  union {
>>>                          struct {
>>> -                               u8 pwmgen_bit_count;
>>> -                               u8 pwm_freq_pre_divider;
>>> -                               bool lsb_reg_used;
>>> -                               bool aux_enable;
>>> +                               struct drm_edp_backlight_info info;
>>>                          } vesa;
>>>                          struct {
>>>                                  bool sdr_uses_aux;
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> index 813f6c553156..286eb337448e 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> @@ -270,114 +270,19 @@ intel_dp_aux_hdr_setup_backlight(struct
>>> intel_connector *connector, enum pipe pi
>>>    }
>>>    
>>>    /* VESA backlight callbacks */
>>> -static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct intel_connector
>>> *connector)
>>> -{
>>> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> -       int ret;
>>> -       u8 mode_reg;
>>> -
>>> -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read backlight mode:
>>> %d\n", ret);
>>> -               return false;
>>> -       }
>>> -
>>> -       return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
>>> -              DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>>> -}
>>> -
>>> -/*
>>> - * Read the current backlight value from DPCD register(s) based
>>> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>> - */
>>>    static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector
>>> *connector, enum pipe unused)
>>>    {
>>> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> -       int ret;
>>> -       u8 read_val[2] = { 0x0 };
>>> -       u16 level = 0;
>>> -
>>> -       /*
>>> -        * If we're not in DPCD control mode yet, the programmed brightness
>>> -        * value is meaningless and we should assume max brightness
>>> -        */
>>> -       if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
>>> -               return connector->panel.backlight.max;
>>> -
>>> -       ret = drm_dp_dpcd_read(&intel_dp->aux,
>>> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
>>> -                              sizeof(read_val));
>>> -       if (ret != sizeof(read_val)) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read brightness level:
>>> %d\n", ret);
>>> -               return 0;
>>> -       }
>>> -
>>> -       if (connector->panel.backlight.edp.vesa.lsb_reg_used)
>>> -               level = (read_val[0] << 8 | read_val[1]);
>>> -       else
>>> -               level = read_val[0];
>>> -
>>> -       return level;
>>> +       return connector->panel.backlight.level;
>>>    }
>>>    
>>> -/*
>>> - * Sends the current backlight level over the aux channel, checking if its
>>> using
>>> - * 8-bit or 16 bit value (MSB and LSB)
>>> - */
>>>    static void
>>> -intel_dp_aux_vesa_set_backlight(const struct drm_connector_state
>>> *conn_state,
>>> -                               u32 level)
>>> +intel_dp_aux_vesa_set_backlight(const struct drm_connector_state
>>> *conn_state, u32 level)
>>>    {
>>>          struct intel_connector *connector = to_intel_connector(conn_state-
>>>> connector);
>>> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> -       int ret;
>>> -       u8 vals[2] = { 0x0 };
>>> -
>>> -       /* Write the MSB and/or LSB */
>>> -       if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
>>> -               vals[0] = (level & 0xFF00) >> 8;
>>> -               vals[1] = (level & 0xFF);
>>> -       } else {
>>> -               vals[0] = level;
>>> -       }
>>> -
>>> -       ret = drm_dp_dpcd_write(&intel_dp->aux,
>>> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
>>> -                               sizeof(vals));
>>> -       if (ret != sizeof(vals)) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to write aux backlight
>>> level: %d\n", ret);
>>> -               return;
>>> -       }
>>> -}
>>> -
>>> -static void set_vesa_backlight_enable(struct intel_connector *connector,
>>> bool enable)
>>> -{
>>> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> -       int ret;
>>> -       u8 reg_val = 0;
>>> -
>>> -       /* Early return when display use other mechanism to enable
>>> backlight. */
>>> -       if (!connector->panel.backlight.edp.vesa.aux_enable)
>>> -               return;
>>> -
>>> -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
>>> DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read eDP display control
>>> register: %d\n", ret);
>>> -               return;
>>> -       }
>>> -
>>> -       if (enable)
>>> -               reg_val |= DP_EDP_BACKLIGHT_ENABLE;
>>> -       else
>>> -               reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
>>> +       struct intel_panel *panel = &connector->panel;
>>> +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>>>    
>>> -       ret = drm_dp_dpcd_writeb(&intel_dp->aux,
>>> DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to %s aux backlight: %d\n",
>>> -                           enable ? "enable" : "disable", ret);
>>> -       }
>>> +       drm_edp_backlight_set_level(&intel_dp->aux, &panel-
>>>> backlight.edp.vesa.info, level);
>>>    }
>>>    
>>>    static void
>>> @@ -385,170 +290,46 @@ intel_dp_aux_vesa_enable_backlight(const struct
>>> intel_crtc_state *crtc_state,
>>>                                     const struct drm_connector_state
>>> *conn_state, u32 level)
>>>    {
>>>          struct intel_connector *connector = to_intel_connector(conn_state-
>>>> connector);
>>> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>          struct intel_panel *panel = &connector->panel;
>>> -       int ret;
>>> -       u8 dpcd_buf, new_dpcd_buf;
>>> -       u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
>>> -
>>> -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read backlight mode:
>>> %d\n", ret);
>>> -               return;
>>> -       }
>>> -
>>> -       new_dpcd_buf = dpcd_buf;
>>> -
>>> -       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
>>> DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {
>>> -               new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>>> -               new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>>> -
>>> -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
>>> DP_EDP_PWMGEN_BIT_COUNT, pwmgen_bit_count);
>>> -               if (ret != 1)
>>> -                       drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen
>>> bit count: %d\n", ret);
>>> -       }
>>> -
>>> -       if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
>>> -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
>>> DP_EDP_BACKLIGHT_FREQ_SET,
>>> -                                        panel-
>>>> backlight.edp.vesa.pwm_freq_pre_divider);
>>> -               if (ret == 1)
>>> -                       new_dpcd_buf |=
>>> DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
>>> -               else
>>> -                       drm_dbg_kms(&i915->drm, "Failed to write aux
>>> backlight frequency: %d\n",
>>> -                                   ret);
>>> -       }
>>> -
>>> -       if (new_dpcd_buf != dpcd_buf) {
>>> -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>>> -                                        new_dpcd_buf);
>>> -               if (ret != 1)
>>> -                       drm_dbg_kms(&i915->drm, "Failed to write aux
>>> backlight mode: %d\n", ret);
>>> -       }
>>> +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>>>    
>>> -       intel_dp_aux_vesa_set_backlight(conn_state, level);
>>> -       set_vesa_backlight_enable(connector, true);
>>> +       drm_edp_backlight_enable(&intel_dp->aux, &panel-
>>>> backlight.edp.vesa.info, level);
>>>    }
>>>    
>>>    static void intel_dp_aux_vesa_disable_backlight(const struct
>>> drm_connector_state *old_conn_state,
>>>                                                  u32 level)
>>>    {
>>> -       set_vesa_backlight_enable(to_intel_connector(old_conn_state-
>>>> connector), false);
>>> -}
>>> -
>>> -/*
>>> - * Compute PWM frequency divider value based off the frequency provided to
>>> us by the vbt.
>>> - * The PWM Frequency is calculated as 27Mhz / (F x P).
>>> - * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of
>>> the
>>> - *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
>>> - * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
>>> - *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
>>> - */
>>> -static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector
>>> *connector)
>>> -{
>>> -       struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>> -       struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> +       struct intel_connector *connector =
>>> to_intel_connector(old_conn_state->connector);
>>>          struct intel_panel *panel = &connector->panel;
>>> -       u32 max_backlight = 0;
>>> -       int ret, freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
>>> -       u8 pn, pn_min, pn_max;
>>> -
>>> -       ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
>>> &pn);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count
>>> cap: %d\n", ret);
>>> -               return 0;
>>> -       }
>>> -
>>> -       pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>>> -       max_backlight = (1 << pn) - 1;
>>> -
>>> -       /* Find desired value of (F x P)
>>> -        * Note that, if F x P is out of supported range, the maximum value
>>> or
>>> -        * minimum value will applied automatically. So no need to check
>>> that.
>>> -        */
>>> -       freq = i915->vbt.backlight.pwm_freq_hz;
>>> -       drm_dbg_kms(&i915->drm, "VBT defined backlight frequency %u Hz\n",
>>> -                   freq);
>>> -       if (!freq) {
>>> -               drm_dbg_kms(&i915->drm,
>>> -                           "Use panel default backlight frequency\n");
>>> -               return max_backlight;
>>> -       }
>>> -
>>> -       fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
>>> -
>>> -       /* Use highest possible value of Pn for more granularity of
>>> brightness
>>> -        * adjustment while satifying the conditions below.
>>> -        * - Pn is in the range of Pn_min and Pn_max
>>> -        * - F is in the range of 1 and 255
>>> -        * - FxP is within 25% of desired value.
>>> -        *   Note: 25% is arbitrary value and may need some tweak.
>>> -        */
>>> -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
>>> DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap
>>> min: %d\n", ret);
>>> -               return max_backlight;
>>> -       }
>>> -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
>>> DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count cap
>>> max: %d\n", ret);
>>> -               return max_backlight;
>>> -       }
>>> -       pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>>> -       pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>>> -
>>> -       /* Ensure frequency is within 25% of desired value */
>>> -       fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
>>> -       fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
>>> -
>>> -       if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
>>> -               drm_dbg_kms(&i915->drm,
>>> -                           "VBT defined backlight frequency out of
>>> range\n");
>>> -               return max_backlight;
>>> -       }
>>> -
>>> -       for (pn = pn_max; pn >= pn_min; pn--) {
>>> -               f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
>>> -               fxp_actual = f << pn;
>>> -               if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
>>> -                       break;
>>> -       }
>>> -
>>> -       drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n", pn);
>>> -       ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
>>> pn);
>>> -       if (ret != 1) {
>>> -               drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit
>>> count: %d\n", ret);
>>> -               return max_backlight;
>>> -       }
>>> -
>>> -       panel->backlight.edp.vesa.pwmgen_bit_count = pn;
>>> -       if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
>>> -               panel->backlight.edp.vesa.pwm_freq_pre_divider = f;
>>> -
>>> -       max_backlight = (1 << pn) - 1;
>>> +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>>>    
>>> -       return max_backlight;
>>> +       drm_edp_backlight_disable(&intel_dp->aux, &panel-
>>>> backlight.edp.vesa.info);
>>>    }
>>>    
>>> -static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
>>> *connector,
>>> -                                            enum pipe pipe)
>>> +static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
>>> *connector, enum pipe pipe)
>>>    {
>>>          struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>          struct intel_panel *panel = &connector->panel;
>>> +       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> +       u16 current_level;
>>> +       u8 current_mode;
>>> +       int ret;
>>>    
>>> -       if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
>>> -               panel->backlight.edp.vesa.aux_enable = true;
>>> -       if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>>> -               panel->backlight.edp.vesa.lsb_reg_used = true;
>>> -
>>> -       panel->backlight.max =
>>> intel_dp_aux_vesa_calc_max_backlight(connector);
>>> -       if (!panel->backlight.max)
>>> -               return -ENODEV;
>>> +       ret = drm_edp_backlight_init(&intel_dp->aux, &panel-
>>>> backlight.edp.vesa.info,
>>> +                                    i915->vbt.backlight.pwm_freq_hz,
>>> intel_dp->edp_dpcd,
>>> +                                    &current_level, &current_mode);
>>> +       if (ret < 0)
>>> +               return ret;
>>>    
>>> +       panel->backlight.max = panel->backlight.edp.vesa.info.max;
>>>          panel->backlight.min = 0;
>>> -       panel->backlight.level = intel_dp_aux_vesa_get_backlight(connector,
>>> pipe);
>>> -       panel->backlight.enabled =
>>> intel_dp_aux_vesa_backlight_dpcd_mode(connector) &&
>>> -                                  panel->backlight.level != 0;
>>> +       if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
>>> +               panel->backlight.level = current_level;
>>> +               panel->backlight.enabled = panel->backlight.level != 0;
>>> +       } else {
>>> +               panel->backlight.level = panel->backlight.max;
>>> +               panel->backlight.enabled = false;
>>> +       }
>>>    
>>>          return 0;
>>>    }
>>> @@ -559,16 +340,12 @@ intel_dp_aux_supports_vesa_backlight(struct
>>> intel_connector *connector)
>>>          struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>          struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>    
>>> -       /* Check the eDP Display control capabilities registers to determine
>>> if
>>> -        * the panel can support backlight control over the aux channel.
>>> -        *
>>> -        * TODO: We currently only support AUX only backlight
>>> configurations, not backlights which
>>> +       /* TODO: We currently only support AUX only backlight
>>> configurations, not backlights which
>>>           * require a mix of PWM and AUX controls to work. In the mean time,
>>> these machines typically
>>>           * work just fine using normal PWM controls anyway.
>>>           */
>>> -       if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
>>> -           (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>>> -           (intel_dp->edp_dpcd[2] &
>>> DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>>> +       if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>>> +           drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
>>>                  drm_dbg_kms(&i915->drm, "AUX Backlight Control
>>> Supported!\n");
>>>                  return true;
>>>          }
>>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>>> index edffd1dcca3e..1eca0b42fc45 100644
>>> --- a/include/drm/drm_dp_helper.h
>>> +++ b/include/drm/drm_dp_helper.h
>>> @@ -1790,6 +1790,24 @@ drm_dp_sink_can_do_video_without_timing_msa(const u8
>>> dpcd[DP_RECEIVER_CAP_SIZE])
>>>                  DP_MSA_TIMING_PAR_IGNORED;
>>>    }
>>>    
>>> +/**
>>> + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight
>>> support
>>> + * @edp_dpcd: The DPCD to check
>>> + *
>>> + * Note that currently this function will return %false for panels which
>>> support various DPCD
>>> + * backlight features but which require the brightness be set through PWM,
>>> and don't support setting
>>> + * the brightness level via the DPCD. This is a TODO.
>>> + *
>>> + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are
>>> supported, %false
>>> + * otherwise
>>> + */
>>> +static inline bool
>>> +drm_edp_backlight_supported(const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
>>> +{
>>> +       return (edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
>>> +               (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
>>> +}
>>> +
>>>    /*
>>>     * DisplayPort AUX channel
>>>     */
>>> @@ -2089,6 +2107,36 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum
>>> drm_dp_quirk quirk)
>>>          return desc->quirks & BIT(quirk);
>>>    }
>>>    
>>> +/**
>>> + * struct drm_edp_backlight_info - Probed eDP backlight info struct
>>> + * @pwmgen_bit_count: The pwmgen bit count
>>> + * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used
>>> for this backlight, if any
>>> + * @max: The maximum backlight level that may be set
>>> + * @lsb_reg_used: Do we also write values to the
>>> DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register?
>>> + * @aux_enable: Does the panel support the AUX enable cap?
>>> + *
>>> + * This structure contains various data about an eDP backlight, which can
>>> be populated by using
>>> + * drm_edp_backlight_init().
>>> + */
>>> +struct drm_edp_backlight_info {
>>> +       u8 pwmgen_bit_count;
>>> +       u8 pwm_freq_pre_divider;
>>> +       u16 max;
>>> +
>>> +       bool lsb_reg_used : 1;
>>> +       bool aux_enable : 1;
>>> +};
>>> +
>>> +int
>>> +drm_edp_backlight_init(struct drm_dp_aux *aux, struct
>>> drm_edp_backlight_info *bl,
>>> +                      u16 driver_pwm_freq_hz, const u8
>>> edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
>>> +                      u16 *current_level, u8 *current_mode);
>>> +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
>>> drm_edp_backlight_info *bl,
>>> +                               u16 level);
>>> +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct
>>> drm_edp_backlight_info *bl,
>>> +                            u16 level);
>>> +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
>>> drm_edp_backlight_info *bl);
>>> +
>>>    #ifdef CONFIG_DRM_DP_CEC
>>>    void drm_dp_cec_irq(struct drm_dp_aux *aux);
>>>    void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
>>>
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC v3 10/10] drm/dp: Extract i915's eDP backlight code into DRM helpers

[Lyude Paul]

On Mon, 2021-02-15 at 19:34 +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 09.02.21 um 00:03 schrieb Lyude Paul:
> > > 
> > > > +       } else {
> > > > +               buf[0] = level;
> > > > +       }
> > > > +
> > > > +       ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > buf,
> > > > sizeof(buf));
> > > > +       if (ret != sizeof(buf)) {
> > > > +               DRM_ERROR("%s: Failed to write aux backlight level:
> > > > %d\n",
> > > > aux->name, ret);
> > > 
> > > Since you're adding this code, you should probably convert to drm_err()
> > > helpers as well. Here and elsewhere.
> > 
> > this is next up on my todo list JFYI-I don't do it right now because there
> > isn't
> > actually any backpointer to the drm driver (and you can't just use the
> > parent of
> > the aux device, since that technically doesn't need to be the drm driver).
> > 
> > I'd add it in this series, but that's going to involve updating functions
> > across
> > the tree like drm_dp_aux_init() so I'd like to do it in a different patch
> > series
> 
> Ok, sure. Makes sense.

I'm actually probably just going to do it anyway, since it seems like a want of
folks really want to keep the drm_dbg_*() prints around
> 
> Best regards
> Thomas
> 
> > 
> > > 
> > > > +               return ret < 0 ? ret : -EIO;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_edp_backlight_set_level);
> > > > +
> > > > +static int
> > > > +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct
> > > > drm_edp_backlight_info *bl,
> > > > +                            bool enable)
> > > > +{
> > > > +       int ret;
> > > > +       u8 buf;
> > > > +
> > > > +       /* The panel uses something other then DPCD for enabling it's
> > > > backlight */
> > > 
> > > 'its'
> > > 
> > > Best regards
> > > Thomas
> > > 
> > > > +       if (!bl->aux_enable)
> > > > +               return 0;
> > > > +
> > > > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> > > > &buf);
> > > > +       if (ret != 1) {
> > > > +               DRM_ERROR("%s: Failed to read eDP display control
> > > > register:
> > > > %d\n", aux->name, ret);
> > > > +               return ret < 0 ? ret : -EIO;
> > > > +       }
> > > > +       if (enable)
> > > > +               buf |= DP_EDP_BACKLIGHT_ENABLE;
> > > > +       else
> > > > +               buf &= ~DP_EDP_BACKLIGHT_ENABLE;
> > > > +
> > > > +       ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> > > > buf);
> > > > +       if (ret != 1) {
> > > > +               DRM_ERROR("%s: Failed to write eDP display control
> > > > register:
> > > > %d\n", aux->name, ret);
> > > > +               return ret < 0 ? ret : -EIO;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_edp_backlight_enable() - Enable an eDP panel's backlight using
> > > > DPCD
> > > > + * @aux: The DP AUX channel to use
> > > > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > > > + * @level: The initial backlight level to set via AUX, if there is one
> > > > + *
> > > > + * This function handles enabling DPCD backlight controls on a panel
> > > > over
> > > > DPCD, while additionally
> > > > + * restoring any important backlight state such as the given backlight
> > > > level, the brightness byte
> > > > + * count, backlight frequency, etc.
> > > > + *
> > > > + * Note that certain panels, while supporting brightness level controls
> > > > over DPCD, may not support
> > > > + * having their backlights enabled via the standard
> > > > %DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
> > > > + * &drm_edp_backlight_info.aux_enable will be set to %false, this
> > > > function
> > > > will skip the step of
> > > > + * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver
> > > > must
> > > > perform the required
> > > > + * implementation specific step for enabling the backlight after
> > > > calling
> > > > this function.
> > > > + *
> > > > + * Returns: %0 on success, negative error code on failure.
> > > > + */
> > > > +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct
> > > > drm_edp_backlight_info *bl,
> > > > +                            const u16 level)
> > > > +{
> > > > +       int ret;
> > > > +       u8 dpcd_buf, new_dpcd_buf;
> > > > +
> > > > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > &dpcd_buf);
> > > > +       if (ret != 1) {
> > > > +               DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n",
> > > > aux->name, ret);
> > > > +               return ret < 0 ? ret : -EIO;
> > > > +       }
> > > > +
> > > > +       new_dpcd_buf = dpcd_buf;
> > > > +
> > > > +       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > > > +               new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > > > +               new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > +
> > > > +               ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT,
> > > > bl-
> > > > > pwmgen_bit_count);
> > > > +               if (ret != 1)
> > > > +                       DRM_DEBUG_KMS("%s: Failed to write aux pwmgen
> > > > bit
> > > > count: %d\n",
> > > > +                                     aux->name, ret);
> > > > +       }
> > > > +
> > > > +       if (bl->pwm_freq_pre_divider) {
> > > > +               ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_FREQ_SET,
> > > > bl-
> > > > > pwm_freq_pre_divider);
> > > > +               if (ret != 1)
> > > > +                       DRM_DEBUG_KMS("%s: Failed to write aux backlight
> > > > frequency: %d\n",
> > > > +                                     aux->name, ret);
> > > > +               else
> > > > +                       new_dpcd_buf |=
> > > > DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> > > > +       }
> > > > +
> > > > +       if (new_dpcd_buf != dpcd_buf) {
> > > > +               ret = drm_dp_dpcd_writeb(aux,
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> > > > +               if (ret != 1) {
> > > > +                       DRM_DEBUG_KMS("%s: Failed to write aux backlight
> > > > mode: %d\n",
> > > > +                                     aux->name, ret);
> > > > +                       return ret < 0 ? ret : -EIO;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       ret = drm_edp_backlight_set_level(aux, bl, level);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +       ret = drm_edp_backlight_set_enable(aux, bl, true);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_edp_backlight_enable);
> > > > +
> > > > +/**
> > > > + * drm_edp_backlight_disable() - Disable an eDP backlight using DPCD,
> > > > if
> > > > supported
> > > > + * @aux: The DP AUX channel to use
> > > > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > > > + *
> > > > + * This function handles disabling DPCD backlight controls on a panel
> > > > over
> > > > AUX. Note that some
> > > > + * panels have backlights that are enabled/disabled by other means,
> > > > despite
> > > > having their brightness
> > > > + * values controlled through DPCD. On such panels
> > > > &drm_edp_backlight_info.aux_enable will be set to
> > > > + * %false, this function will become a no-op (and we will skip updating
> > > > + * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to
> > > > perform it's own
> > > > + * implementation specific step for disabling the backlight.
> > > > + *
> > > > + * Returns: %0 on success or no-op, negative error code on failure.
> > > > + */
> > > > +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
> > > > drm_edp_backlight_info *bl)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       ret = drm_edp_backlight_set_enable(aux, bl, false);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_edp_backlight_disable);
> > > > +
> > > > +static inline int
> > > > +drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct
> > > > drm_edp_backlight_info *bl,
> > > > +                           u16 driver_pwm_freq_hz, const u8
> > > > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
> > > > +{
> > > > +       int fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> > > > +       int ret;
> > > > +       u8 pn, pn_min, pn_max;
> > > > +
> > > > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
> > > > +       if (ret != 1) {
> > > > +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap:
> > > > %d\n", aux->name, ret);
> > > > +               return -ENODEV;
> > > > +       }
> > > > +
> > > > +       pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > > > +       bl->max = (1 << pn) - 1;
> > > > +       if (!driver_pwm_freq_hz)
> > > > +               return 0;
> > > > +
> > > > +       /*
> > > > +        * Set PWM Frequency divider to match desired frequency provided
> > > > by
> > > > the driver.
> > > > +        * The PWM Frequency is calculated as 27Mhz / (F x P).
> > > > +        * - Where F = PWM Frequency Pre-Divider value programmed by
> > > > field
> > > > 7:0 of the
> > > > +        *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address
> > > > 00728h)
> > > > +        * - Where P = 2^Pn, where Pn is the value programmed by field
> > > > 4:0
> > > > of the
> > > > +        *             EDP_PWMGEN_BIT_COUNT register (DPCD Address
> > > > 00724h)
> > > > +        */
> > > > +
> > > > +       /* Find desired value of (F x P)
> > > > +        * Note that, if F x P is out of supported range, the maximum
> > > > value
> > > > or minimum value will
> > > > +        * applied automatically. So no need to check that.
> > > > +        */
> > > > +       fxp = DIV_ROUND_CLOSEST(1000 * DP_EDP_BACKLIGHT_FREQ_BASE_KHZ,
> > > > driver_pwm_freq_hz);
> > > > +
> > > > +       /* Use highest possible value of Pn for more granularity of
> > > > brightness adjustment while
> > > > +        * satifying the conditions below.
> > > > +        * - Pn is in the range of Pn_min and Pn_max
> > > > +        * - F is in the range of 1 and 255
> > > > +        * - FxP is within 25% of desired value.
> > > > +        *   Note: 25% is arbitrary value and may need some tweak.
> > > > +        */
> > > > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> > > > &pn_min);
> > > > +       if (ret != 1) {
> > > > +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap
> > > > min:
> > > > %d\n", aux->name, ret);
> > > > +               return 0;
> > > > +       }
> > > > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
> > > > &pn_max);
> > > > +       if (ret != 1) {
> > > > +               DRM_DEBUG_KMS("%s: Failed to read pwmgen bit count cap
> > > > max:
> > > > %d\n", aux->name, ret);
> > > > +               return 0;
> > > > +       }
> > > > +       pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > > > +       pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > > > +
> > > > +       /* Ensure frequency is within 25% of desired value */
> > > > +       fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > > > +       fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > > > +       if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > > > +               DRM_DEBUG_KMS("%s: Driver defined backlight frequency
> > > > (%d)
> > > > out of range\n",
> > > > +                             aux->name, driver_pwm_freq_hz);
> > > > +               return 0;
> > > > +       }
> > > > +
> > > > +       for (pn = pn_max; pn >= pn_min; pn--) {
> > > > +               f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> > > > +               fxp_actual = f << pn;
> > > > +               if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > > > +                       break;
> > > > +       }
> > > > +
> > > > +       ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> > > > +       if (ret != 1) {
> > > > +               DRM_DEBUG_KMS("%s: Failed to write aux pwmgen bit count:
> > > > %d\n", aux->name, ret);
> > > > +               return 0;
> > > > +       }
> > > > +       bl->pwmgen_bit_count = pn;
> > > > +       bl->max = (1 << pn) - 1;
> > > > +
> > > > +       if (edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) {
> > > > +               bl->pwm_freq_pre_divider = f;
> > > > +               DRM_DEBUG_KMS("%s: Using backlight frequency from driver
> > > > (%dHz)\n",
> > > > +                             aux->name, driver_pwm_freq_hz);
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static inline int
> > > > +drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct
> > > > drm_edp_backlight_info *bl,
> > > > +                             u8 *current_mode)
> > > > +{
> > > > +       int ret;
> > > > +       u8 buf[2];
> > > > +       u8 mode_reg;
> > > > +
> > > > +       ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > &mode_reg);
> > > > +       if (ret != 1) {
> > > > +               DRM_DEBUG_KMS("%s: Failed to read backlight mode: %d\n",
> > > > aux->name, ret);
> > > > +               return ret < 0 ? ret : -EIO;
> > > > +       }
> > > > +
> > > > +       *current_mode = (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK);
> > > > +       if (*current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > > > +               int size = 1 + bl->lsb_reg_used;
> > > > +
> > > > +               ret = drm_dp_dpcd_read(aux,
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > buf, size);
> > > > +               if (ret != size) {
> > > > +                       DRM_DEBUG_KMS("%s: Failed to read backlight
> > > > level:
> > > > %d\n", aux->name, ret);
> > > > +                       return ret < 0 ? ret : -EIO;
> > > > +               }
> > > > +
> > > > +               if (bl->lsb_reg_used)
> > > > +                       return (buf[0] << 8) | buf[1];
> > > > +               else
> > > > +                       return buf[0];
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * If we're not in DPCD control mode yet, the programmed
> > > > brightness
> > > > value is meaningless and
> > > > +        * the driver should assume max brightness
> > > > +        */
> > > > +       return bl->max;
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_edp_backlight_init() - Probe a display panel's TCON using the
> > > > standard VESA eDP backlight
> > > > + * interface.
> > > > + * @aux: The DP aux device to use for probing
> > > > + * @bl: The &drm_edp_backlight_info struct to fill out with information
> > > > on
> > > > the backlight
> > > > + * @driver_pwm_freq_hz: Optional PWM frequency from the driver in hz
> > > > + * @edp_dpcd: A cached copy of the eDP DPCD
> > > > + * @current_level: Where to store the probed brightness level
> > > > + * @current_mode: Where to store the currently set backlight control
> > > > mode
> > > > + *
> > > > + * Initializes a &drm_edp_backlight_info struct by probing @aux for
> > > > it's
> > > > backlight capabilities,
> > > > + * along with also probing the current and maximum supported brightness
> > > > levels.
> > > > + *
> > > > + * If @driver_pwm_freq_hz is non-zero, this will be used as the
> > > > backlight
> > > > frequency. Otherwise, the
> > > > + * default frequency from the panel is used.
> > > > + *
> > > > + * Returns: %0 on success, negative error code on failure.
> > > > + */
> > > > +int
> > > > +drm_edp_backlight_init(struct drm_dp_aux *aux, struct
> > > > drm_edp_backlight_info *bl,
> > > > +                      u16 driver_pwm_freq_hz, const u8
> > > > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> > > > +                      u16 *current_level, u8 *current_mode)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       if (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> > > > +               bl->aux_enable = true;
> > > > +       if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> > > > +               bl->lsb_reg_used = true;
> > > > +
> > > > +       ret = drm_edp_backlight_probe_max(aux, bl, driver_pwm_freq_hz,
> > > > edp_dpcd);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       ret = drm_edp_backlight_probe_level(aux, bl, current_mode);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +       *current_level = ret;
> > > > +
> > > > +       DRM_DEBUG_KMS("%s: Found backlight level=%d/%d
> > > > pwm_freq_pre_divider=%d mode=%x\n",
> > > > +                     aux->name, *current_level, bl->max, bl-
> > > > > pwm_freq_pre_divider, *current_mode);
> > > > +       DRM_DEBUG_KMS("%s: Backlight caps: pwmgen_bit_count=%d
> > > > lsb_reg_used=%d aux_enable=%d\n",
> > > > +                     aux->name, bl->pwmgen_bit_count, bl->lsb_reg_used,
> > > > bl-
> > > > > aux_enable);
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_edp_backlight_init);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 16824eb3ef93..03051ab75d30 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -263,10 +263,7 @@ struct intel_panel {
> > > >                  /* DPCD backlight */
> > > >                  union {
> > > >                          struct {
> > > > -                               u8 pwmgen_bit_count;
> > > > -                               u8 pwm_freq_pre_divider;
> > > > -                               bool lsb_reg_used;
> > > > -                               bool aux_enable;
> > > > +                               struct drm_edp_backlight_info info;
> > > >                          } vesa;
> > > >                          struct {
> > > >                                  bool sdr_uses_aux;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > index 813f6c553156..286eb337448e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > @@ -270,114 +270,19 @@ intel_dp_aux_hdr_setup_backlight(struct
> > > > intel_connector *connector, enum pipe pi
> > > >    }
> > > >    
> > > >    /* VESA backlight callbacks */
> > > > -static bool intel_dp_aux_vesa_backlight_dpcd_mode(struct
> > > > intel_connector
> > > > *connector)
> > > > -{
> > > > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > -       int ret;
> > > > -       u8 mode_reg;
> > > > -
> > > > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &mode_reg);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read backlight mode:
> > > > %d\n", ret);
> > > > -               return false;
> > > > -       }
> > > > -
> > > > -       return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > -              DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > -}
> > > > -
> > > > -/*
> > > > - * Read the current backlight value from DPCD register(s) based
> > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > - */
> > > >    static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector
> > > > *connector, enum pipe unused)
> > > >    {
> > > > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > -       int ret;
> > > > -       u8 read_val[2] = { 0x0 };
> > > > -       u16 level = 0;
> > > > -
> > > > -       /*
> > > > -        * If we're not in DPCD control mode yet, the programmed
> > > > brightness
> > > > -        * value is meaningless and we should assume max brightness
> > > > -        */
> > > > -       if (!intel_dp_aux_vesa_backlight_dpcd_mode(connector))
> > > > -               return connector->panel.backlight.max;
> > > > -
> > > > -       ret = drm_dp_dpcd_read(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, &read_val,
> > > > -                              sizeof(read_val));
> > > > -       if (ret != sizeof(read_val)) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read brightness
> > > > level:
> > > > %d\n", ret);
> > > > -               return 0;
> > > > -       }
> > > > -
> > > > -       if (connector->panel.backlight.edp.vesa.lsb_reg_used)
> > > > -               level = (read_val[0] << 8 | read_val[1]);
> > > > -       else
> > > > -               level = read_val[0];
> > > > -
> > > > -       return level;
> > > > +       return connector->panel.backlight.level;
> > > >    }
> > > >    
> > > > -/*
> > > > - * Sends the current backlight level over the aux channel, checking if
> > > > its
> > > > using
> > > > - * 8-bit or 16 bit value (MSB and LSB)
> > > > - */
> > > >    static void
> > > > -intel_dp_aux_vesa_set_backlight(const struct drm_connector_state
> > > > *conn_state,
> > > > -                               u32 level)
> > > > +intel_dp_aux_vesa_set_backlight(const struct drm_connector_state
> > > > *conn_state, u32 level)
> > > >    {
> > > >          struct intel_connector *connector =
> > > > to_intel_connector(conn_state-
> > > > > connector);
> > > > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > -       int ret;
> > > > -       u8 vals[2] = { 0x0 };
> > > > -
> > > > -       /* Write the MSB and/or LSB */
> > > > -       if (connector->panel.backlight.edp.vesa.lsb_reg_used) {
> > > > -               vals[0] = (level & 0xFF00) >> 8;
> > > > -               vals[1] = (level & 0xFF);
> > > > -       } else {
> > > > -               vals[0] = level;
> > > > -       }
> > > > -
> > > > -       ret = drm_dp_dpcd_write(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, vals,
> > > > -                               sizeof(vals));
> > > > -       if (ret != sizeof(vals)) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to write aux backlight
> > > > level: %d\n", ret);
> > > > -               return;
> > > > -       }
> > > > -}
> > > > -
> > > > -static void set_vesa_backlight_enable(struct intel_connector
> > > > *connector,
> > > > bool enable)
> > > > -{
> > > > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > -       int ret;
> > > > -       u8 reg_val = 0;
> > > > -
> > > > -       /* Early return when display use other mechanism to enable
> > > > backlight. */
> > > > -       if (!connector->panel.backlight.edp.vesa.aux_enable)
> > > > -               return;
> > > > -
> > > > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_DISPLAY_CONTROL_REGISTER, &reg_val);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read eDP display
> > > > control
> > > > register: %d\n", ret);
> > > > -               return;
> > > > -       }
> > > > -
> > > > -       if (enable)
> > > > -               reg_val |= DP_EDP_BACKLIGHT_ENABLE;
> > > > -       else
> > > > -               reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> > > > +       struct intel_panel *panel = &connector->panel;
> > > > +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> > > >    
> > > > -       ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to %s aux backlight:
> > > > %d\n",
> > > > -                           enable ? "enable" : "disable", ret);
> > > > -       }
> > > > +       drm_edp_backlight_set_level(&intel_dp->aux, &panel-
> > > > > backlight.edp.vesa.info, level);
> > > >    }
> > > >    
> > > >    static void
> > > > @@ -385,170 +290,46 @@ intel_dp_aux_vesa_enable_backlight(const struct
> > > > intel_crtc_state *crtc_state,
> > > >                                     const struct drm_connector_state
> > > > *conn_state, u32 level)
> > > >    {
> > > >          struct intel_connector *connector =
> > > > to_intel_connector(conn_state-
> > > > > connector);
> > > > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > -       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > >          struct intel_panel *panel = &connector->panel;
> > > > -       int ret;
> > > > -       u8 dpcd_buf, new_dpcd_buf;
> > > > -       u8 pwmgen_bit_count = panel-
> > > > >backlight.edp.vesa.pwmgen_bit_count;
> > > > -
> > > > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read backlight mode:
> > > > %d\n", ret);
> > > > -               return;
> > > > -       }
> > > > -
> > > > -       new_dpcd_buf = dpcd_buf;
> > > > -
> > > > -       if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) {
> > > > -               new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > > > -               new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > -
> > > > -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_EDP_PWMGEN_BIT_COUNT, pwmgen_bit_count);
> > > > -               if (ret != 1)
> > > > -                       drm_dbg_kms(&i915->drm, "Failed to write aux
> > > > pwmgen
> > > > bit count: %d\n", ret);
> > > > -       }
> > > > -
> > > > -       if (panel->backlight.edp.vesa.pwm_freq_pre_divider) {
> > > > -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_FREQ_SET,
> > > > -                                        panel-
> > > > > backlight.edp.vesa.pwm_freq_pre_divider);
> > > > -               if (ret == 1)
> > > > -                       new_dpcd_buf |=
> > > > DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> > > > -               else
> > > > -                       drm_dbg_kms(&i915->drm, "Failed to write aux
> > > > backlight frequency: %d\n",
> > > > -                                   ret);
> > > > -       }
> > > > -
> > > > -       if (new_dpcd_buf != dpcd_buf) {
> > > > -               ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > -                                        new_dpcd_buf);
> > > > -               if (ret != 1)
> > > > -                       drm_dbg_kms(&i915->drm, "Failed to write aux
> > > > backlight mode: %d\n", ret);
> > > > -       }
> > > > +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> > > >    
> > > > -       intel_dp_aux_vesa_set_backlight(conn_state, level);
> > > > -       set_vesa_backlight_enable(connector, true);
> > > > +       drm_edp_backlight_enable(&intel_dp->aux, &panel-
> > > > > backlight.edp.vesa.info, level);
> > > >    }
> > > >    
> > > >    static void intel_dp_aux_vesa_disable_backlight(const struct
> > > > drm_connector_state *old_conn_state,
> > > >                                                  u32 level)
> > > >    {
> > > > -       set_vesa_backlight_enable(to_intel_connector(old_conn_state-
> > > > > connector), false);
> > > > -}
> > > > -
> > > > -/*
> > > > - * Compute PWM frequency divider value based off the frequency provided
> > > > to
> > > > us by the vbt.
> > > > - * The PWM Frequency is calculated as 27Mhz / (F x P).
> > > > - * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0
> > > > of
> > > > the
> > > > - *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > > > - * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of
> > > > the
> > > > - *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > > > - */
> > > > -static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector
> > > > *connector)
> > > > -{
> > > > -       struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > > > -       struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > +       struct intel_connector *connector =
> > > > to_intel_connector(old_conn_state->connector);
> > > >          struct intel_panel *panel = &connector->panel;
> > > > -       u32 max_backlight = 0;
> > > > -       int ret, freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> > > > -       u8 pn, pn_min, pn_max;
> > > > -
> > > > -       ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
> > > > &pn);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count
> > > > cap: %d\n", ret);
> > > > -               return 0;
> > > > -       }
> > > > -
> > > > -       pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > > > -       max_backlight = (1 << pn) - 1;
> > > > -
> > > > -       /* Find desired value of (F x P)
> > > > -        * Note that, if F x P is out of supported range, the maximum
> > > > value
> > > > or
> > > > -        * minimum value will applied automatically. So no need to check
> > > > that.
> > > > -        */
> > > > -       freq = i915->vbt.backlight.pwm_freq_hz;
> > > > -       drm_dbg_kms(&i915->drm, "VBT defined backlight frequency %u
> > > > Hz\n",
> > > > -                   freq);
> > > > -       if (!freq) {
> > > > -               drm_dbg_kms(&i915->drm,
> > > > -                           "Use panel default backlight frequency\n");
> > > > -               return max_backlight;
> > > > -       }
> > > > -
> > > > -       fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ),
> > > > freq);
> > > > -
> > > > -       /* Use highest possible value of Pn for more granularity of
> > > > brightness
> > > > -        * adjustment while satifying the conditions below.
> > > > -        * - Pn is in the range of Pn_min and Pn_max
> > > > -        * - F is in the range of 1 and 255
> > > > -        * - FxP is within 25% of desired value.
> > > > -        *   Note: 25% is arbitrary value and may need some tweak.
> > > > -        */
> > > > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count
> > > > cap
> > > > min: %d\n", ret);
> > > > -               return max_backlight;
> > > > -       }
> > > > -       ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to read pwmgen bit count
> > > > cap
> > > > max: %d\n", ret);
> > > > -               return max_backlight;
> > > > -       }
> > > > -       pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > > > -       pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > > > -
> > > > -       /* Ensure frequency is within 25% of desired value */
> > > > -       fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > > > -       fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > > > -
> > > > -       if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > > > -               drm_dbg_kms(&i915->drm,
> > > > -                           "VBT defined backlight frequency out of
> > > > range\n");
> > > > -               return max_backlight;
> > > > -       }
> > > > -
> > > > -       for (pn = pn_max; pn >= pn_min; pn--) {
> > > > -               f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> > > > -               fxp_actual = f << pn;
> > > > -               if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > > > -                       break;
> > > > -       }
> > > > -
> > > > -       drm_dbg_kms(&i915->drm, "Using eDP pwmgen bit count of %d\n",
> > > > pn);
> > > > -       ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_EDP_PWMGEN_BIT_COUNT,
> > > > pn);
> > > > -       if (ret != 1) {
> > > > -               drm_dbg_kms(&i915->drm, "Failed to write aux pwmgen bit
> > > > count: %d\n", ret);
> > > > -               return max_backlight;
> > > > -       }
> > > > -
> > > > -       panel->backlight.edp.vesa.pwmgen_bit_count = pn;
> > > > -       if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> > > > -               panel->backlight.edp.vesa.pwm_freq_pre_divider = f;
> > > > -
> > > > -       max_backlight = (1 << pn) - 1;
> > > > +       struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> > > >    
> > > > -       return max_backlight;
> > > > +       drm_edp_backlight_disable(&intel_dp->aux, &panel-
> > > > > backlight.edp.vesa.info);
> > > >    }
> > > >    
> > > > -static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
> > > > *connector,
> > > > -                                            enum pipe pipe)
> > > > +static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
> > > > *connector, enum pipe pipe)
> > > >    {
> > > >          struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > >          struct intel_panel *panel = &connector->panel;
> > > > +       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > +       u16 current_level;
> > > > +       u8 current_mode;
> > > > +       int ret;
> > > >    
> > > > -       if (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
> > > > -               panel->backlight.edp.vesa.aux_enable = true;
> > > > -       if (intel_dp->edp_dpcd[2] &
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> > > > -               panel->backlight.edp.vesa.lsb_reg_used = true;
> > > > -
> > > > -       panel->backlight.max =
> > > > intel_dp_aux_vesa_calc_max_backlight(connector);
> > > > -       if (!panel->backlight.max)
> > > > -               return -ENODEV;
> > > > +       ret = drm_edp_backlight_init(&intel_dp->aux, &panel-
> > > > > backlight.edp.vesa.info,
> > > > +                                    i915->vbt.backlight.pwm_freq_hz,
> > > > intel_dp->edp_dpcd,
> > > > +                                    &current_level, &current_mode);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > >    
> > > > +       panel->backlight.max = panel->backlight.edp.vesa.info.max;
> > > >          panel->backlight.min = 0;
> > > > -       panel->backlight.level =
> > > > intel_dp_aux_vesa_get_backlight(connector,
> > > > pipe);
> > > > -       panel->backlight.enabled =
> > > > intel_dp_aux_vesa_backlight_dpcd_mode(connector) &&
> > > > -                                  panel->backlight.level != 0;
> > > > +       if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > > > +               panel->backlight.level = current_level;
> > > > +               panel->backlight.enabled = panel->backlight.level != 0;
> > > > +       } else {
> > > > +               panel->backlight.level = panel->backlight.max;
> > > > +               panel->backlight.enabled = false;
> > > > +       }
> > > >    
> > > >          return 0;
> > > >    }
> > > > @@ -559,16 +340,12 @@ intel_dp_aux_supports_vesa_backlight(struct
> > > > intel_connector *connector)
> > > >          struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > >          struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > >    
> > > > -       /* Check the eDP Display control capabilities registers to
> > > > determine
> > > > if
> > > > -        * the panel can support backlight control over the aux channel.
> > > > -        *
> > > > -        * TODO: We currently only support AUX only backlight
> > > > configurations, not backlights which
> > > > +       /* TODO: We currently only support AUX only backlight
> > > > configurations, not backlights which
> > > >           * require a mix of PWM and AUX controls to work. In the mean
> > > > time,
> > > > these machines typically
> > > >           * work just fine using normal PWM controls anyway.
> > > >           */
> > > > -       if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> > > > &&
> > > > -           (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > > > -           (intel_dp->edp_dpcd[2] &
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
> > > > +       if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > > > +           drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
> > > >                  drm_dbg_kms(&i915->drm, "AUX Backlight Control
> > > > Supported!\n");
> > > >                  return true;
> > > >          }
> > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > > index edffd1dcca3e..1eca0b42fc45 100644
> > > > --- a/include/drm/drm_dp_helper.h
> > > > +++ b/include/drm/drm_dp_helper.h
> > > > @@ -1790,6 +1790,24 @@ drm_dp_sink_can_do_video_without_timing_msa(const
> > > > u8
> > > > dpcd[DP_RECEIVER_CAP_SIZE])
> > > >                  DP_MSA_TIMING_PAR_IGNORED;
> > > >    }
> > > >    
> > > > +/**
> > > > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight
> > > > support
> > > > + * @edp_dpcd: The DPCD to check
> > > > + *
> > > > + * Note that currently this function will return %false for panels
> > > > which
> > > > support various DPCD
> > > > + * backlight features but which require the brightness be set through
> > > > PWM,
> > > > and don't support setting
> > > > + * the brightness level via the DPCD. This is a TODO.
> > > > + *
> > > > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls
> > > > are
> > > > supported, %false
> > > > + * otherwise
> > > > + */
> > > > +static inline bool
> > > > +drm_edp_backlight_supported(const u8
> > > > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
> > > > +{
> > > > +       return (edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> > > > +               (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> > > > +}
> > > > +
> > > >    /*
> > > >     * DisplayPort AUX channel
> > > >     */
> > > > @@ -2089,6 +2107,36 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc,
> > > > enum
> > > > drm_dp_quirk quirk)
> > > >          return desc->quirks & BIT(quirk);
> > > >    }
> > > >    
> > > > +/**
> > > > + * struct drm_edp_backlight_info - Probed eDP backlight info struct
> > > > + * @pwmgen_bit_count: The pwmgen bit count
> > > > + * @pwm_freq_pre_divider: The PWM frequency pre-divider value being
> > > > used
> > > > for this backlight, if any
> > > > + * @max: The maximum backlight level that may be set
> > > > + * @lsb_reg_used: Do we also write values to the
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register?
> > > > + * @aux_enable: Does the panel support the AUX enable cap?
> > > > + *
> > > > + * This structure contains various data about an eDP backlight, which
> > > > can
> > > > be populated by using
> > > > + * drm_edp_backlight_init().
> > > > + */
> > > > +struct drm_edp_backlight_info {
> > > > +       u8 pwmgen_bit_count;
> > > > +       u8 pwm_freq_pre_divider;
> > > > +       u16 max;
> > > > +
> > > > +       bool lsb_reg_used : 1;
> > > > +       bool aux_enable : 1;
> > > > +};
> > > > +
> > > > +int
> > > > +drm_edp_backlight_init(struct drm_dp_aux *aux, struct
> > > > drm_edp_backlight_info *bl,
> > > > +                      u16 driver_pwm_freq_hz, const u8
> > > > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> > > > +                      u16 *current_level, u8 *current_mode);
> > > > +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
> > > > drm_edp_backlight_info *bl,
> > > > +                               u16 level);
> > > > +int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct
> > > > drm_edp_backlight_info *bl,
> > > > +                            u16 level);
> > > > +int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
> > > > drm_edp_backlight_info *bl);
> > > > +
> > > >    #ifdef CONFIG_DRM_DP_CEC
> > > >    void drm_dp_cec_irq(struct drm_dp_aux *aux);
> > > >    void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > > > 
> > > 
> > 
> 

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!