[Nouveau] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control

[Nouveau] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control

[Lyude Paul]

Now that we've added support to i915 for controlling panel backlights that
need PWM to be enabled/disabled, let's finalize this and add support for
controlling brightness levels via PWM as well. This should hopefully put us
towards the path of supporting _ALL_ backlights via VESA's DPCD interface
which would allow us to finally start trusting the DPCD again.

Note that for the DRM helpers for this, we change some behavior by starting
to hide all backlights that require PWM controls by default, and require
that the driver pass our new DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM flag to
drm_edp_backlight_supported(). The primary reason for doing this is that
panels requiring PWM in addition to DPCD controls will require additional
implementation on the driver's side, as there's no way for us to handle
PWM controls from the helpers in a driver-independent way.

Note however that we still don't enable using this by default when it's not
needed, primarily because I haven't yet had a chance to confirm if it's
safe to do this on the one machine in Intel's CI that had an issue with
this: samus-fi-bdw. I have done basic testing of this on other machines
though, by manually patching i915 to force it into PWM-only mode on some of
my laptops.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Rajeev Nandan <rajeevny@codeaurora.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Satadru Pramanik <satadru@gmail.com>
---
 drivers/gpu/drm/drm_dp_helper.c               | 102 ++++++++++++++----
 .../drm/i915/display/intel_dp_aux_backlight.c |  51 ++++++---
 drivers/gpu/drm/nouveau/nouveau_backlight.c   |   2 +-
 include/drm/drm_dp_helper.h                   |  46 ++++----
 4 files changed, 146 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4d0d1e8e51fa..a1cf849fc6ed 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3163,7 +3163,10 @@ EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
  * @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().
+ * already have been enabled by the driver by calling drm_edp_backlight_enable(). Note that if the
+ * panel in question requires the PWM pin be used to control brightness levels (e.g.
+ * &drm_edp_backlight_info.aux_set is %false), then this function becomes a no-op and it is up to
+ * the driver to handle adjusting the brightness levels.
  *
  * Returns: %0 on success, negative error code on failure
  */
@@ -3173,6 +3176,10 @@ int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct drm_edp_bac
 	int ret;
 	u8 buf[2] = { 0 };
 
+	/* The panel uses PWM for controlling brightness levels */
+	if (!bl->aux_set)
+		return 0;
+
 	if (bl->lsb_reg_used) {
 		buf[0] = (level & 0xff00) >> 8;
 		buf[1] = (level & 0x00ff);
@@ -3234,11 +3241,8 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct drm_edp_backli
  * 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.
+ * Drivers supporting %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM are expected to enable the panel backlight
+ * and/or program the panel's brightness level after calling this function.
  *
  * Returns: %0 on success, negative error code on failure.
  */
@@ -3246,7 +3250,7 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backli
 			     const u16 level)
 {
 	int ret;
-	u8 dpcd_buf, new_dpcd_buf;
+	u8 dpcd_buf, new_dpcd_buf, new_mode;
 
 	ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf);
 	if (ret != 1) {
@@ -3256,10 +3260,14 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backli
 	}
 
 	new_dpcd_buf = dpcd_buf;
+	if (bl->aux_set)
+		new_mode = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+	else
+		new_mode = DP_EDP_BACKLIGHT_CONTROL_MODE_PWM;
 
-	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
+	if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != new_mode) {
 		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
-		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+		new_dpcd_buf |= new_mode;
 
 		if (bl->pwmgen_bit_count) {
 			ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, bl->pwmgen_bit_count);
@@ -3305,11 +3313,10 @@ EXPORT_SYMBOL(drm_edp_backlight_enable);
  * @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.
+ * panels have backlights that are enabled/disabled via PWM. 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 handle disabling the
+ * backlight via PWM.
  *
  * Returns: %0 on success or no-op, negative error code on failure.
  */
@@ -3333,6 +3340,9 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
 	int ret;
 	u8 pn, pn_min, pn_max;
 
+	if (!bl->aux_set)
+		return 0;
+
 	ret = drm_dp_dpcd_readb(aux, DP_EDP_PWMGEN_BIT_COUNT, &pn);
 	if (ret != 1) {
 		drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap: %d\n",
@@ -3418,7 +3428,7 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
 }
 
 static inline int
-drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
+drm_edp_backlight_probe_state(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl,
 			      u8 *current_mode)
 {
 	int ret;
@@ -3433,6 +3443,9 @@ drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct drm_edp_backlight_i
 	}
 
 	*current_mode = (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK);
+	if (!bl->aux_set)
+		return 0;
+
 	if (*current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
 		int size = 1 + bl->lsb_reg_used;
 
@@ -3463,7 +3476,7 @@ drm_edp_backlight_probe_level(struct drm_dp_aux *aux, struct drm_edp_backlight_i
  * @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_level: Where to store the probed brightness level, if any
  * @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,
@@ -3483,28 +3496,71 @@ drm_edp_backlight_init(struct drm_dp_aux *aux, struct drm_edp_backlight_info *bl
 
 	if (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)
 		bl->aux_enable = true;
+	if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)
+		bl->aux_set = true;
 	if (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
 		bl->lsb_reg_used = true;
 
+	/* Sanity check caps */
+	if (!bl->aux_set && !(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
+		drm_dbg_kms(aux->drm_dev,
+			    "%s: Panel supports neither AUX or PWM brightness control? Aborting\n",
+			    aux->name);
+		return -EINVAL;
+	}
+
 	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);
+	ret = drm_edp_backlight_probe_state(aux, bl, current_mode);
 	if (ret < 0)
 		return ret;
 	*current_level = ret;
 
 	drm_dbg_kms(aux->drm_dev,
-		    "%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_dbg_kms(aux->drm_dev,
-		    "%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);
+		    "%s: Found backlight: aux_set=%d aux_enable=%d mode=%d\n",
+		    aux->name, bl->aux_set, bl->aux_enable, *current_mode);
+	if (bl->aux_set) {
+		drm_dbg_kms(aux->drm_dev,
+			    "%s: Backlight caps: level=%d/%d pwm_freq_pre_divider=%d lsb_reg_used=%d\n",
+			    aux->name, *current_level, bl->max, bl->pwm_freq_pre_divider,
+			    bl->lsb_reg_used);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_edp_backlight_init);
 
+/**
+ * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support
+ * @aux: The AUX channel, only used for debug logging
+ * @edp_dpcd: The DPCD to check
+ * @caps: The backlight capabilities this driver supports
+ *
+ * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false
+ * otherwise
+ */
+bool drm_edp_backlight_supported(struct drm_dp_aux *aux,
+				 const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
+				 enum drm_edp_backlight_driver_caps caps)
+{
+	if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP))
+		return false;
+
+	if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) &&
+	    (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) ||
+	     !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) {
+		drm_dbg_kms(aux->drm_dev,
+			    "%s: eDP backlight needs PWM support, but driver doesn't have it\n",
+			    aux->name);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_edp_backlight_supported);
+
 #if IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
 	(IS_MODULE(CONFIG_DRM_KMS_HELPER) && IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE))
 
@@ -3576,7 +3632,7 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
 	if (ret < 0)
 		return ret;
 
-	if (!drm_edp_backlight_supported(edp_dpcd)) {
+	if (!drm_edp_backlight_supported(aux, edp_dpcd, 0)) {
 		DRM_DEV_INFO(panel->dev, "DP AUX backlight is not supported\n");
 		return 0;
 	}
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 594fdc7453ca..9b1ac02b0263 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -282,6 +282,12 @@ intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state, u3
 	struct intel_panel *panel = &connector->panel;
 	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
+	if (!panel->backlight.edp.vesa.info.aux_set) {
+		const u32 pwm_level = intel_backlight_level_to_pwm(connector, level);
+
+		intel_backlight_set_pwm_level(conn_state, pwm_level);
+	}
+
 	drm_edp_backlight_set_level(&intel_dp->aux, &panel->backlight.edp.vesa.info, level);
 }
 
@@ -293,9 +299,16 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
 	struct intel_panel *panel = &connector->panel;
 	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
-	if (!panel->backlight.edp.vesa.info.aux_enable)
-		panel->backlight.pwm_funcs->enable(crtc_state, conn_state,
-						   panel->backlight.pwm_level_max);
+	if (!panel->backlight.edp.vesa.info.aux_enable) {
+		u32 pwm_level;
+
+		if (!panel->backlight.edp.vesa.info.aux_set)
+			pwm_level = intel_backlight_level_to_pwm(connector, level);
+		else
+			pwm_level = panel->backlight.pwm_level_max;
+
+		panel->backlight.pwm_funcs->enable(crtc_state, conn_state, pwm_level);
+	}
 
 	drm_edp_backlight_enable(&intel_dp->aux, &panel->backlight.edp.vesa.info, level);
 }
@@ -329,7 +342,7 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
 	if (ret < 0)
 		return ret;
 
-	if (!panel->backlight.edp.vesa.info.aux_enable) {
+	if (!panel->backlight.edp.vesa.info.aux_set || !panel->backlight.edp.vesa.info.aux_enable) {
 		ret = panel->backlight.pwm_funcs->setup(connector, pipe);
 		if (ret < 0) {
 			drm_err(&i915->drm,
@@ -338,14 +351,27 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
 			return ret;
 		}
 	}
-	panel->backlight.max = panel->backlight.edp.vesa.info.max;
-	panel->backlight.min = 0;
-	if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
-		panel->backlight.level = current_level;
-		panel->backlight.enabled = panel->backlight.level != 0;
+
+	if (panel->backlight.edp.vesa.info.aux_set) {
+		panel->backlight.max = panel->backlight.edp.vesa.info.max;
+		panel->backlight.min = 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;
+		}
 	} else {
-		panel->backlight.level = panel->backlight.max;
-		panel->backlight.enabled = false;
+		panel->backlight.max = panel->backlight.pwm_level_max;
+		panel->backlight.min = panel->backlight.pwm_level_min;
+		if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_PWM) {
+			panel->backlight.level = panel->backlight.pwm_funcs->get(connector, pipe);
+			panel->backlight.enabled = panel->backlight.pwm_enabled;
+		} else {
+			panel->backlight.level = panel->backlight.max;
+			panel->backlight.enabled = false;
+		}
 	}
 
 	return 0;
@@ -357,7 +383,8 @@ 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);
 
-	if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
+	if (drm_edp_backlight_supported(&intel_dp->aux, intel_dp->edp_dpcd,
+					DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM)) {
 		drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n");
 		return true;
 	}
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 1cbd71abc80a..c54642c038c4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -308,7 +308,7 @@ nv50_backlight_init(struct nouveau_backlight *bl,
 		if (ret < 0)
 			return ret;
 
-		if (drm_edp_backlight_supported(edp_dpcd)) {
+		if (drm_edp_backlight_supported(&nv_conn->aux, edp_dpcd, 0)) {
 			NV_DEBUG(drm, "DPCD backlight controls supported on %s\n",
 				 nv_conn->base.name);
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 3ee0b3ffb8a5..74dce86946e9 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1825,24 +1825,6 @@ 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
  */
@@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
  * @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?
+ * @aux_enable: Does the panel support the AUX enable cap? Always %false when the driver doesn't
+ * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM
+ * @aux_set: Does the panel support setting the brightness through AUX? Always %true when the driver
+ * doesn't support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM
+ * @pwm_set: Does the panel support setting the brightness through a PWM pin?
  *
  * This structure contains various data about an eDP backlight, which can be populated by using
  * drm_edp_backlight_init().
@@ -2212,8 +2198,30 @@ struct drm_edp_backlight_info {
 
 	bool lsb_reg_used : 1;
 	bool aux_enable : 1;
+	bool aux_set : 1;
+};
+
+/**
+ * enum drm_edp_backlight_driver_caps - Flags for drivers to indicate support of various eDP
+ * backlight functionality
+ *
+ * Used with DRM's eDP backlight helpers in order to indicate to the DRM core which eDP backlight
+ * capabilities the driver is capable of supporting.
+ */
+enum drm_edp_backlight_driver_caps {
+	/**
+	 * @DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM: Some eDP panels require that either panel enablement
+	 * and/or brightness level controls are controlled via PWM. Drivers which want to support
+	 * such panels should enable this cap. If this flag is omitted,
+	 * drm_edp_backlight_supported() will return %false upon encountering such panels and will
+	 * additionally print a debugging message to the kernel log.
+	 */
+	DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM = BIT(0),
 };
 
+bool drm_edp_backlight_supported(struct drm_dp_aux *aux,
+				 const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
+				 enum drm_edp_backlight_driver_caps caps);
 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],
-- 
2.31.1

Re: [Nouveau] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control

[Doug Anderson]

Hi,

On Mon, Sep 27, 2021 at 1:12 PM Lyude Paul <lyude@redhat.com> wrote:
>
> @@ -3305,11 +3313,10 @@ EXPORT_SYMBOL(drm_edp_backlight_enable);
>   * @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.
> + * panels have backlights that are enabled/disabled via PWM. 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 handle disabling the
> + * backlight via PWM.

I'm not sure I understand the comment above. You say "enabled/disabled
via PWM" and that doesn't make sense w/ my mental model. Normally I
think of a PWM allowing you to adjust the brightness and there being a
separate GPIO that's in charge of enable/disable. To some extent you
could think of a PWM as being "disabled" when its duty cycle is 0%,
but usually there's separate "enable" logic that really has nothing to
do with the PWM itself.

In general, it seems like the options are:

1. DPCD controls PWM and the "enable" logic.

2. DPCD controls PWM but requires an external "enable" GPIO.

3. We require an external PWM but DPCD controls the "enable" logic.

Maybe you need a second "capability" to describe whether the client of
your code knows how to control an enable GPIO? ...or perhaps better
you don't need a capability and you can just assume that if the client
needs to set an "enable" GPIO that it will do so. That would match how
things work today. AKA:

a) Client calls the AUX backlight code to "enable"

b) AUX backlight code will set the "enable" bit if supported.

c) Client will set the "enable" GPIO if it knows about one.

Presumably only one of b) or c) will actually do something. If neither
does something then this panel simply isn't compatible with this
board.


> +/**
> + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support
> + * @aux: The AUX channel, only used for debug logging
> + * @edp_dpcd: The DPCD to check
> + * @caps: The backlight capabilities this driver supports
> + *
> + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false
> + * otherwise
> + */
> +bool drm_edp_backlight_supported(struct drm_dp_aux *aux,
> +                                const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> +                                enum drm_edp_backlight_driver_caps caps)
> +{
> +       if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP))
> +               return false;
> +
> +       if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) &&
> +           (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) ||
> +            !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) {

Elsewhere you match DP_EDP_BACKLIGHT_AUX_ENABLE_CAP against
edp_dpcd[1]. Here you match against [2]. Are you sure that's correct?


>  /*
>   * DisplayPort AUX channel
>   */
> @@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
>   * @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?
> + * @aux_enable: Does the panel support the AUX enable cap? Always %false when the driver doesn't
> + * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM

Why is aux_enable always false if it doesn't support
DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM? It doesn't seem like the code
enforces this and I'm not sure why it would. Am I confused?

Re: [Nouveau] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control

[Lyude Paul]

On Tue, 2021-09-28 at 13:00 -0700, Doug Anderson wrote:
> Hi,
> 
> 
> I'm not sure I understand the comment above. You say "enabled/disabled
> via PWM" and that doesn't make sense w/ my mental model. Normally I
> think of a PWM allowing you to adjust the brightness and there being a
> separate GPIO that's in charge of enable/disable. To some extent you

Oops - you're completely right, it is a GPIO pin and I got myself
confused since in i915 I'm the chipset-specific callbacks for turning
the panel on are intertwined with the PWM callbacks.

> could think of a PWM as being "disabled" when its duty cycle is 0%,
> but usually there's separate "enable" logic that really has nothing to
> do with the PWM itself.
> 
> In general, it seems like the options are:
> 
> 1. DPCD controls PWM and the "enable" logic.
> 
> 2. DPCD controls PWM but requires an external "enable" GPIO.
> 
> 3. We require an external PWM but DPCD controls the "enable" logic.
> 
> Maybe you need a second "capability" to describe whether the client of
> your code knows how to control an enable GPIO? ...or perhaps better
> you don't need a capability and you can just assume that if the client
> needs to set an "enable" GPIO that it will do so. That would match how
> things work today. AKA:
> 
> a) Client calls the AUX backlight code to "enable"
> 
> b) AUX backlight code will set the "enable" bit if supported.
> 
> c) Client will set the "enable" GPIO if it knows about one.
> 
> Presumably only one of b) or c) will actually do something. If neither
> does something then this panel simply isn't compatible with this
> board.

I will definitely take note from this explanation and rewrite some of
the helper docs I'm updating to reflect this, thank you!

Being that I think panel drivers are basically the only other user of
this driver, if you think this is the way to go I'm OK with this. My
original reasoning for having a cap for this was worrying about the ARM
drivers handling this, along with potentially changing backlight
behavior in nouveau. I'm realizing now though that those are probably
problems for nouveau and not the helper, and I could probably avoid
hitting any issues by just adding some additional DPCD checks for GPIO
enabling/PWM passthrough in nouveau itself.

So I'll drop the cap in the next respin of this
> 
> 
> > +/**
> > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight
> > support
> > + * @aux: The AUX channel, only used for debug logging
> > + * @edp_dpcd: The DPCD to check
> > + * @caps: The backlight capabilities this driver supports
> > + *
> > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are
> > supported, %false
> > + * otherwise
> > + */
> > +bool drm_edp_backlight_supported(struct drm_dp_aux *aux,
> > +                                const u8
> > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> > +                                enum drm_edp_backlight_driver_caps caps)
> > +{
> > +       if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP))
> > +               return false;
> > +
> > +       if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) &&
> > +           (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) ||
> > +            !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) {
> 
> Elsewhere you match DP_EDP_BACKLIGHT_AUX_ENABLE_CAP against
> edp_dpcd[1]. Here you match against [2]. Are you sure that's correct?

absolutely not! thank you for catching this

> 
> 
> >  /*
> >   * DisplayPort AUX channel
> >   */
> > @@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc,
> > enum drm_dp_quirk quirk)
> >   * @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?
> > + * @aux_enable: Does the panel support the AUX enable cap? Always %false
> > when the driver doesn't
> > + * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM
> 
> Why is aux_enable always false if it doesn't support
> DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM? It doesn't seem like the code
> enforces this and I'm not sure why it would. Am I confused?

This was mainly just to keep the behavior identical for drivers that
didn't support controlling backlights like this, but re: the response I
wrote up above I think we can just totally drop the caps.

> 

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