On Sun, Mar 30, 2025 at 05:31:20PM +0100, Christopher Obbard wrote:
> According to the eDP specification (VESA Embedded DisplayPort Standard
> v1.4b, Section 3.3.10.2), if the value of DP_EDP_PWMGEN_BIT_COUNT is
> less than DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, the sink is required to use
> the MIN value as the effective PWM bit count.
>
> This commit updates the logic to clamp the reported
> DP_EDP_PWMGEN_BIT_COUNT to the range defined by
> DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN and _CAP_MAX. This ensures correct
> handling of eDP panels that report a zero PWM bit count but still
> provide valid non-zero MIN and MAX capability values. Without this
> clamping, brightness values may be interpreted incorrectly, leading
> to a dim or non-functional backlight.
>
> For example, the Samsung ATNA40YK20 OLED panel used in the Lenovo
> ThinkPad T14s Gen6 (Snapdragon) reports a PWM bit count of 0, but
> supports AUX backlight control and declares a valid 11-bit range.
> Clamping ensures brightness scaling works as intended on such panels.
>
> Co-developed-by: Rui Miguel Silva <rui.si...@linaro.org>
> Signed-off-by: Rui Miguel Silva <rui.si...@linaro.org>
> Signed-off-by: Christopher Obbard <christopher.obb...@linaro.org>
> ---
> Changes in v3:
> - Properly rebase patch on top of latest version of drm-misc-next.
> - Make patch more generic by clamping PWM bit count to advertised MIN
> and MAX capabilities (suggested by Dmitry).
> - Link to v2:
> https://lore.kernel.org/r/20250327-wip-obbardc-qcom-t14s-oled-panel-brightness-v2-1-16dc3ee00...@linaro.org
>
> Changes in v2:
> - Split backlight brightness patch from T14s OLED enablement series.
> - Use PWMGEN_CAP_MIN rather than MAX (Dmitry).
> - Rework commit message to reference eDP spec.
> - Rebase on drm-misc-next.
> - Link to v1:
> https://lore.kernel.org/all/20250325-wip-obbardc-qcom-t14s-oled-panel-v2-4-e9bc7c9d3...@linaro.org/
> ---
> drivers/gpu/drm/display/drm_dp_helper.c | 48
> ++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index
> e2439c8a7fefe116b04aaa689b557e2387b05540..fcc26cb96a51066a503433b2dc660126155d179c
> 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -28,6 +28,7 @@
> #include <linux/init.h>
> #include <linux/iopoll.h>
> #include <linux/kernel.h>
> +#include <linux/minmax.h>
> #include <linux/module.h>
> #include <linux/sched.h>
> #include <linux/seq_file.h>
> @@ -4033,8 +4034,33 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux,
> struct drm_edp_backlight_inf
> aux->name, ret);
> return -ENODEV;
> }
> -
Nitpick: please keep the empty line.
> pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> + ret = drm_dp_dpcd_read_byte(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> &pn_min);
> + if (ret != 1) {
No. Please take a look a few lines below, where you are removing
corresponding lines.
> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count
> cap min: %d\n",
> + aux->name, ret);
> + return -ENODEV;
Hmm. Why? It was 'return 0' before and your commit message contains no
explanation.
> + }
> + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> + ret = drm_dp_dpcd_read_byte(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
> &pn_max);
> + if (ret != 1) {
> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count
> cap max: %d\n",
> + aux->name, ret);
> + return -ENODEV;
> + }
> + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> + /*
> + * Per VESA eDP Spec v1.4b, section 3.3.10.2:
> + * If DP_EDP_PWMGEN_BIT_COUNT is less than
> DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> + * the sink must use the MIN value as the effective PWM bit count.
> + * Clamp the reported value to the [MIN, MAX] capability range to ensure
> + * correct brightness scaling on compliant eDP panels.
> + */
> + pn = clamp(pn, pn_min, pn_max);
> +
> bl->max = (1 << pn) - 1;
> if (!driver_pwm_freq_hz)
> return 0;
> @@ -4054,29 +4080,15 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux,
> struct drm_edp_backlight_inf
> */
> 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
> + /*
> + * Ensure frequency is within 25% of desired value.
> + * Use highest possible value of Pn for more granularity of brightness
> adjustment while
Huh? I don't see a corresponding code change. If you are fixing the
comment, it should come as a separate commit.
> * satisfying 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_read_byte(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> &pn_min);
> - if (ret < 0) {
> - drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count
> cap min: %d\n",
> - aux->name, ret);
> - return 0;
> - }
> - ret = drm_dp_dpcd_read_byte(aux, DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
> &pn_max);
> - if (ret < 0) {
> - drm_dbg_kms(aux->drm_dev, "%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) {
--
With best wishes
Dmitry
