drivers/gpu/drm/display/drm_dp_helper.c | 42 +++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 15 deletions(-)
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 _CAP_MIN and _CAP_MAX.
As part of this change, the behavior is modified such that reading both
_CAP_MIN and _CAP_MAX registers is now required to succeed, otherwise
bl->max value could end up being not set although
drm_edp_backlight_probe_max() returned success.
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.silva@linaro.org>
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org>
---
Changes in v6:
- Update commit message around chaning reading PWMGEN_BIT_COUNT_CAP_MIN
and _CAP_MAX to be required.
- Link to v5: https://lore.kernel.org/r/20250330-wip-obbardc-qcom-t14s-oled-panel-brightness-v5-1-25083d9732fc@linaro.org
Changes in v5:
- Correctly check return value when reading PWMGEN_BIT_COUNT_CAP_MIN
and _CAP_MAX.
- Link to v4: https://lore.kernel.org/r/20250330-wip-obbardc-qcom-t14s-oled-panel-brightness-v4-1-85ef0991bdf8@linaro.org
Changes in v4:
- Remove unrelated whitespace changes.
- Remove unrelated commit change.
- Add note to commit message about changing read of PWMGEN_BIT_COUNT_CAP_MIN
and _CAP__MAX from optional to required.
- Link to v3: https://lore.kernel.org/r/20250330-wip-obbardc-qcom-t14s-oled-panel-brightness-v3-1-156801d97a8a@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-16dc3ee00276@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-e9bc7c9d30cc@linaro.org/
---
drivers/gpu/drm/display/drm_dp_helper.c | 42 +++++++++++++++++++++------------
1 file changed, 27 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index e2439c8a7fefe116b04aaa689b557e2387b05540..5550c40310c55ee275b3ebec08a7500cab38ae78 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>
@@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
}
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 < 0) {
+ drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
+ aux->name, ret);
+ return -ENODEV;
+ }
+ 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 < 0) {
+ 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;
@@ -4061,21 +4088,6 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
* - 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);
---
base-commit: 4c4d9b7b6c6e676eca22585139aba5f03de74b90
change-id: 20250327-wip-obbardc-qcom-t14s-oled-panel-brightness-4020865b6580
Best regards,
--
Christopher Obbard <christopher.obbard@linaro.org>On Sun, Mar 30, 2025 at 08:31:07PM +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 _CAP_MIN and _CAP_MAX.
>
> As part of this change, the behavior is modified such that reading both
> _CAP_MIN and _CAP_MAX registers is now required to succeed, otherwise
> bl->max value could end up being not set although
> drm_edp_backlight_probe_max() returned success.
>
> 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.silva@linaro.org>
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org>
> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
> }
>
> 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 < 0) {
> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
> + aux->name, ret);
> + return -ENODEV;
> + }
> + 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 < 0) {
> + 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);
You never make sure that pn_min <= pn_max so you could end up with
pn < pn_min on broken hardware here. Not sure if it's something you need
to worry about at this point.
> +
> bl->max = (1 << pn) - 1;
> if (!driver_pwm_freq_hz)
> return 0;
Otherwise this looks correct to me and does not break backlight control
on the X1E reference design:
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
Johan
Hi Johan,
On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
>
> On Sun, Mar 30, 2025 at 08:31:07PM +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 _CAP_MIN and _CAP_MAX.
> >
> > As part of this change, the behavior is modified such that reading both
> > _CAP_MIN and _CAP_MAX registers is now required to succeed, otherwise
> > bl->max value could end up being not set although
> > drm_edp_backlight_probe_max() returned success.
> >
> > 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.silva@linaro.org>
> > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> > Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org>
>
> > @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
> > }
> >
> > 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 < 0) {
> > + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
> > + aux->name, ret);
> > + return -ENODEV;
> > + }
> > + 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 < 0) {
> > + 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);
>
> You never make sure that pn_min <= pn_max so you could end up with
> pn < pn_min on broken hardware here. Not sure if it's something you need
> to worry about at this point.
I am honestly not sure. I would hope that devices follow the spec and
there is no need to be too paranoid, but then again we do live in the
real world where things are... not so simple ;-).
I will wait for further feedback from someone who has more experience
with eDP panels than I have.
> > +
> > bl->max = (1 << pn) - 1;
> > if (!driver_pwm_freq_hz)
> > return 0;
>
> Otherwise this looks correct to me and does not break backlight control
> on the X1E reference design:
>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
Thanks for testing!
Chris
On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
> > > @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
> > > }
> > >
> > > 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 < 0) {
> > > + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
> > > + aux->name, ret);
> > > + return -ENODEV;
> > > + }
> > > + 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 < 0) {
> > > + 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);
> >
> > You never make sure that pn_min <= pn_max so you could end up with
> > pn < pn_min on broken hardware here. Not sure if it's something you need
> > to worry about at this point.
>
> I am honestly not sure. I would hope that devices follow the spec and
> there is no need to be too paranoid, but then again we do live in the
> real world where things are... not so simple ;-).
> I will wait for further feedback from someone who has more experience
> with eDP panels than I have.
There's always going to be buggy devices and input should always be
sanitised so I suggest adding that check before calling clamp() (which
expects min <= max) so that the result here is well-defined.
Johan
Johan,
On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
> > On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
>
> > > > @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
> > > > }
> > > >
> > > > 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 < 0) {
> > > > + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
> > > > + aux->name, ret);
> > > > + return -ENODEV;
> > > > + }
> > > > + 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 < 0) {
> > > > + 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);
> > >
> > > You never make sure that pn_min <= pn_max so you could end up with
> > > pn < pn_min on broken hardware here. Not sure if it's something you need
> > > to worry about at this point.
> >
> > I am honestly not sure. I would hope that devices follow the spec and
> > there is no need to be too paranoid, but then again we do live in the
> > real world where things are... not so simple ;-).
> > I will wait for further feedback from someone who has more experience
> > with eDP panels than I have.
>
> There's always going to be buggy devices and input should always be
> sanitised so I suggest adding that check before calling clamp() (which
> expects min <= max) so that the result here is well-defined.
Makes sense, I will do so in the next revision.
Thanks.
Chris
Hi Chris,
On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
> > > On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
> > > > > @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
> > > > > }
> > > > >
> > > > > 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 < 0) {
> > > > > + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
> > > > > + aux->name, ret);
> > > > > + return -ENODEV;
> > > > > + }
> > > > > + 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 < 0) {
> > > > > + 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);
> > > >
> > > > You never make sure that pn_min <= pn_max so you could end up with
> > > > pn < pn_min on broken hardware here. Not sure if it's something you need
> > > > to worry about at this point.
> > >
> > > I am honestly not sure. I would hope that devices follow the spec and
> > > there is no need to be too paranoid, but then again we do live in the
> > > real world where things are... not so simple ;-).
> > > I will wait for further feedback from someone who has more experience
> > > with eDP panels than I have.
> >
> > There's always going to be buggy devices and input should always be
> > sanitised so I suggest adding that check before calling clamp() (which
> > expects min <= max) so that the result here is well-defined.
>
> Makes sense, I will do so in the next revision.
It seems you never got around to respinning this one so sending a
reminder.
Johan
On 20/05/2025 10:06, Johan Hovold wrote:
> Hi Chris,
>
> On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
>> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
>>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
>>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
>>>>>> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
>>>>>> }
>>>>>>
>>>>>> 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 < 0) {
>>>>>> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
>>>>>> + aux->name, ret);
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> + 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 < 0) {
>>>>>> + 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);
>>>>>
>>>>> You never make sure that pn_min <= pn_max so you could end up with
>>>>> pn < pn_min on broken hardware here. Not sure if it's something you need
>>>>> to worry about at this point.
I'm trying to figure out what would be the behavior in this case ?
- Warn ?
- pn_max = pn_min ?
- use BIT_COUNT as-is and ignore MIN/MAX ?
- pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ?
- reverse clamp? clamp(pn, pn_max, pn_min); ?
- generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ?
Or just bail out ?
Neil
>>>>
>>>> I am honestly not sure. I would hope that devices follow the spec and
>>>> there is no need to be too paranoid, but then again we do live in the
>>>> real world where things are... not so simple ;-).
>>>> I will wait for further feedback from someone who has more experience
>>>> with eDP panels than I have.
>>>
>>> There's always going to be buggy devices and input should always be
>>> sanitised so I suggest adding that check before calling clamp() (which
>>> expects min <= max) so that the result here is well-defined.
>>
>> Makes sense, I will do so in the next revision.
>
> It seems you never got around to respinning this one so sending a
> reminder.
>
> Johan
>
On Thu, 24 Jul 2025 at 12:08, <neil.armstrong@linaro.org> wrote:
>
> On 20/05/2025 10:06, Johan Hovold wrote:
> > Hi Chris,
> >
> > On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
> >> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
> >>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
> >>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
> >>>>>> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
> >>>>>> }
> >>>>>>
> >>>>>> 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 < 0) {
> >>>>>> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
> >>>>>> + aux->name, ret);
> >>>>>> + return -ENODEV;
> >>>>>> + }
> >>>>>> + 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 < 0) {
> >>>>>> + 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);
> >>>>>
> >>>>> You never make sure that pn_min <= pn_max so you could end up with
> >>>>> pn < pn_min on broken hardware here. Not sure if it's something you need
> >>>>> to worry about at this point.
>
> I'm trying to figure out what would be the behavior in this case ?
>
> - Warn ?
> - pn_max = pn_min ?
> - use BIT_COUNT as-is and ignore MIN/MAX ?
> - pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ?
> - reverse clamp? clamp(pn, pn_max, pn_min); ?
> - generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ?
Per the standard, the min >= 1 and max >= min. We don't need to bother
about anything here.
On the other hand, I think the patch needs to be updated a bit. If the
pn value changed after clamping, it makes sense to write it back to
the DP_EDP_PWMGEN_BIT_COUNT register by jumping to the tail of the
drm_edp_backlight_probe_max() function
>
> Or just bail out ?
>
> Neil
>
> >>>>
> >>>> I am honestly not sure. I would hope that devices follow the spec and
> >>>> there is no need to be too paranoid, but then again we do live in the
> >>>> real world where things are... not so simple ;-).
> >>>> I will wait for further feedback from someone who has more experience
> >>>> with eDP panels than I have.
> >>>
> >>> There's always going to be buggy devices and input should always be
> >>> sanitised so I suggest adding that check before calling clamp() (which
> >>> expects min <= max) so that the result here is well-defined.
> >>
> >> Makes sense, I will do so in the next revision.
> >
> > It seems you never got around to respinning this one so sending a
> > reminder.
> >
> > Johan
> >
>
--
With best wishes
Dmitry
On 24/07/2025 11:32, Dmitry Baryshkov wrote:
> On Thu, 24 Jul 2025 at 12:08, <neil.armstrong@linaro.org> wrote:
>>
>> On 20/05/2025 10:06, Johan Hovold wrote:
>>> Hi Chris,
>>>
>>> On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
>>>> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
>>>>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
>>>>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
>>>>>>>> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct drm_dp_aux *aux, struct drm_edp_backlight_inf
>>>>>>>> }
>>>>>>>>
>>>>>>>> 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 < 0) {
>>>>>>>> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read pwmgen bit count cap min: %d\n",
>>>>>>>> + aux->name, ret);
>>>>>>>> + return -ENODEV;
>>>>>>>> + }
>>>>>>>> + 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 < 0) {
>>>>>>>> + 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);
>>>>>>>
>>>>>>> You never make sure that pn_min <= pn_max so you could end up with
>>>>>>> pn < pn_min on broken hardware here. Not sure if it's something you need
>>>>>>> to worry about at this point.
>>
>> I'm trying to figure out what would be the behavior in this case ?
>>
>> - Warn ?
>> - pn_max = pn_min ?
>> - use BIT_COUNT as-is and ignore MIN/MAX ?
>> - pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ?
>> - reverse clamp? clamp(pn, pn_max, pn_min); ?
>> - generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ?
>
> Per the standard, the min >= 1 and max >= min. We don't need to bother
> about anything here.
Yeah, I agree. But I think a:
if (likely(pn_min <= pn_max))
is simple and doesn't cost much...
>
> On the other hand, I think the patch needs to be updated a bit. If the
> pn value changed after clamping, it makes sense to write it back to
> the DP_EDP_PWMGEN_BIT_COUNT register by jumping to the tail of the
> drm_edp_backlight_probe_max() function
You're right, we need to write it back, but we can't jump to
the tail of the function since it has all the pwmgen logic
in the middle.
Neil
>
>>
>> Or just bail out ?
>>
>> Neil
>>
>>>>>>
>>>>>> I am honestly not sure. I would hope that devices follow the spec and
>>>>>> there is no need to be too paranoid, but then again we do live in the
>>>>>> real world where things are... not so simple ;-).
>>>>>> I will wait for further feedback from someone who has more experience
>>>>>> with eDP panels than I have.
>>>>>
>>>>> There's always going to be buggy devices and input should always be
>>>>> sanitised so I suggest adding that check before calling clamp() (which
>>>>> expects min <= max) so that the result here is well-defined.
>>>>
>>>> Makes sense, I will do so in the next revision.
>>>
>>> It seems you never got around to respinning this one so sending a
>>> reminder.
>>>
>>> Johan
>>>
>>
>
>
On 24/07/2025 12:42, Neil Armstrong wrote:
> On 24/07/2025 11:32, Dmitry Baryshkov wrote:
>> On Thu, 24 Jul 2025 at 12:08, <neil.armstrong@linaro.org> wrote:
>>>
>>> On 20/05/2025 10:06, Johan Hovold wrote:
>>>> Hi Chris,
>>>>
>>>> On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
>>>>> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
>>>>>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
>>>>>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
>>>>>>>>> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct
>>>>>>>>> drm_dp_aux *aux, struct drm_edp_backlight_inf
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> 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 < 0) {
>>>>>>>>> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read
>>>>>>>>> pwmgen bit count cap min: %d\n",
>>>>>>>>> + aux->name, ret);
>>>>>>>>> + return -ENODEV;
>>>>>>>>> + }
>>>>>>>>> + 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 < 0) {
>>>>>>>>> + 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);
>>>>>>>>
>>>>>>>> You never make sure that pn_min <= pn_max so you could end up with
>>>>>>>> pn < pn_min on broken hardware here. Not sure if it's something
>>>>>>>> you need
>>>>>>>> to worry about at this point.
>>>
>>> I'm trying to figure out what would be the behavior in this case ?
>>>
>>> - Warn ?
>>> - pn_max = pn_min ?
>>> - use BIT_COUNT as-is and ignore MIN/MAX ?
>>> - pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ?
>>> - reverse clamp? clamp(pn, pn_max, pn_min); ?
>>> - generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ?
>>
>> Per the standard, the min >= 1 and max >= min. We don't need to bother
>> about anything here.
>
> Yeah, I agree. But I think a:
> if (likely(pn_min <= pn_max))
> is simple and doesn't cost much..
Really, no need to.
>
>>
>> On the other hand, I think the patch needs to be updated a bit. If the
>> pn value changed after clamping, it makes sense to write it back to
>> the DP_EDP_PWMGEN_BIT_COUNT register by jumping to the tail of the
>> drm_edp_backlight_probe_max() function
>
> You're right, we need to write it back, but we can't jump to
> the tail of the function since it has all the pwmgen logic
> in the middle.
If you add 'driver_pwm_freq_hz && 'to the
DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP condition at the end of the function,
then we can jump to the tail.
>
> Neil
>
>>
>>>
>>> Or just bail out ?
>>>
>>> Neil
>>>
>>>>>>>
>>>>>>> I am honestly not sure. I would hope that devices follow the spec
>>>>>>> and
>>>>>>> there is no need to be too paranoid, but then again we do live in
>>>>>>> the
>>>>>>> real world where things are... not so simple ;-).
>>>>>>> I will wait for further feedback from someone who has more
>>>>>>> experience
>>>>>>> with eDP panels than I have.
>>>>>>
>>>>>> There's always going to be buggy devices and input should always be
>>>>>> sanitised so I suggest adding that check before calling clamp()
>>>>>> (which
>>>>>> expects min <= max) so that the result here is well-defined.
>>>>>
>>>>> Makes sense, I will do so in the next revision.
>>>>
>>>> It seems you never got around to respinning this one so sending a
>>>> reminder.
>>>>
>>>> Johan
>>>>
>>>
>>
>>
>
--
With best wishes
Dmitry
On Thu, Jul 24, 2025 at 02:09:10PM +0300, Dmitry Baryshkov wrote:
> On 24/07/2025 12:42, Neil Armstrong wrote:
> > On 24/07/2025 11:32, Dmitry Baryshkov wrote:
> >> On Thu, 24 Jul 2025 at 12:08, <neil.armstrong@linaro.org> wrote:
> >>> On 20/05/2025 10:06, Johan Hovold wrote:
> >>>> On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
> >>>>> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
> >>>>>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
> >>>>>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
> >>>>>>>>> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct
> >>>>>>>>> drm_dp_aux *aux, struct drm_edp_backlight_inf
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> 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 < 0) {
> >>>>>>>>> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read
> >>>>>>>>> pwmgen bit count cap min: %d\n",
> >>>>>>>>> + aux->name, ret);
> >>>>>>>>> + return -ENODEV;
> >>>>>>>>> + }
> >>>>>>>>> + 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 < 0) {
> >>>>>>>>> + 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);
> >>>>>>>>
> >>>>>>>> You never make sure that pn_min <= pn_max so you could end up with
> >>>>>>>> pn < pn_min on broken hardware here. Not sure if it's something
> >>>>>>>> you need
> >>>>>>>> to worry about at this point.
> >>>
> >>> I'm trying to figure out what would be the behavior in this case ?
> >>>
> >>> - Warn ?
> >>> - pn_max = pn_min ?
> >>> - use BIT_COUNT as-is and ignore MIN/MAX ?
> >>> - pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ?
> >>> - reverse clamp? clamp(pn, pn_max, pn_min); ?
> >>> - generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ?
> >>
> >> Per the standard, the min >= 1 and max >= min. We don't need to bother
> >> about anything here.
> >
> > Yeah, I agree. But I think a:
> > if (likely(pn_min <= pn_max))
> > is simple and doesn't cost much..
>
> Really, no need to.
It doesn't matter what the spec says, what matters is what may happen if
a device violates the spec (e.g. if a driver triggers a division by
zero).
Always sanitise your input.
(But there is no need for likely() as this is not a hot path.)
Johan
On Mon, Aug 04, 2025 at 03:22:32PM +0200, Johan Hovold wrote:
> On Thu, Jul 24, 2025 at 02:09:10PM +0300, Dmitry Baryshkov wrote:
> > On 24/07/2025 12:42, Neil Armstrong wrote:
> > > On 24/07/2025 11:32, Dmitry Baryshkov wrote:
> > >> On Thu, 24 Jul 2025 at 12:08, <neil.armstrong@linaro.org> wrote:
> > >>> On 20/05/2025 10:06, Johan Hovold wrote:
> > >>>> On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote:
> > >>>>> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote:
> > >>>>>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote:
> > >>>>>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote:
> > >>>>>>>>> @@ -4035,6 +4036,32 @@ drm_edp_backlight_probe_max(struct
>
> > >>>>>>>>> drm_dp_aux *aux, struct drm_edp_backlight_inf
> > >>>>>>>>> }
> > >>>>>>>>>
> > >>>>>>>>> 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 < 0) {
> > >>>>>>>>> + drm_dbg_kms(aux->drm_dev, "%s: Failed to read
> > >>>>>>>>> pwmgen bit count cap min: %d\n",
> > >>>>>>>>> + aux->name, ret);
> > >>>>>>>>> + return -ENODEV;
> > >>>>>>>>> + }
> > >>>>>>>>> + 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 < 0) {
> > >>>>>>>>> + 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);
> > >>>>>>>>
> > >>>>>>>> You never make sure that pn_min <= pn_max so you could end up with
> > >>>>>>>> pn < pn_min on broken hardware here. Not sure if it's something
> > >>>>>>>> you need
> > >>>>>>>> to worry about at this point.
> > >>>
> > >>> I'm trying to figure out what would be the behavior in this case ?
> > >>>
> > >>> - Warn ?
> > >>> - pn_max = pn_min ?
> > >>> - use BIT_COUNT as-is and ignore MIN/MAX ?
> > >>> - pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ?
> > >>> - reverse clamp? clamp(pn, pn_max, pn_min); ?
> > >>> - generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ?
> > >>
> > >> Per the standard, the min >= 1 and max >= min. We don't need to bother
> > >> about anything here.
> > >
> > > Yeah, I agree. But I think a:
> > > if (likely(pn_min <= pn_max))
> > > is simple and doesn't cost much..
> >
> > Really, no need to.
>
> It doesn't matter what the spec says, what matters is what may happen if
> a device violates the spec (e.g. if a driver triggers a division by
> zero).
>
> Always sanitise your input.
Agreed. I hope Chris will now post v7...
>
> (But there is no need for likely() as this is not a hot path.)
>
> Johan
--
With best wishes
Dmitry
On Thu, Jul 24, 2025 at 11:42:38AM +0200, Neil Armstrong wrote: > On 24/07/2025 11:32, Dmitry Baryshkov wrote: > > On Thu, 24 Jul 2025 at 12:08, <neil.armstrong@linaro.org> wrote: > >> On 20/05/2025 10:06, Johan Hovold wrote: > >>> On Fri, Apr 04, 2025 at 02:24:32PM +0100, Christopher Obbard wrote: > >>>> On Fri, 4 Apr 2025 at 09:54, Johan Hovold <johan@kernel.org> wrote: > >>>>> On Fri, Apr 04, 2025 at 08:54:29AM +0100, Christopher Obbard wrote: > >>>>>> On Mon, 31 Mar 2025 at 09:33, Johan Hovold <johan@kernel.org> wrote: > >>>>>>>> + /* > >>>>>>>> + * 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); > >>>>>>> > >>>>>>> You never make sure that pn_min <= pn_max so you could end up with > >>>>>>> pn < pn_min on broken hardware here. Not sure if it's something you need > >>>>>>> to worry about at this point. > >> > >> I'm trying to figure out what would be the behavior in this case ? > >> > >> - Warn ? > >> - pn_max = pn_min ? > >> - use BIT_COUNT as-is and ignore MIN/MAX ? > >> - pm_max = max(pn_min, pn_max); pm_min = min(pn_min, pn_max); ? > >> - reverse clamp? clamp(pn, pn_max, pn_min); ? > >> - generic clamp? clamp(pn, min(pn_min, pn_max), max(pn_min, pn_max)); ? > > > > Per the standard, the min >= 1 and max >= min. We don't need to bother > > about anything here. > > Yeah, I agree. But I think a: > if (likely(pn_min <= pn_max)) > is simple and doesn't cost much... > >> Or just bail out ? Yeah, just bail out. If we ever run into broken hardware like this, we can determine some workaround then. Johan
On Sun, Mar 30, 2025 at 08:31:07PM +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 _CAP_MIN and _CAP_MAX. > > As part of this change, the behavior is modified such that reading both > _CAP_MIN and _CAP_MAX registers is now required to succeed, otherwise > bl->max value could end up being not set although > drm_edp_backlight_probe_max() returned success. > > 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.silva@linaro.org> > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> > Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> -- With best wishes Dmitry
© 2016 - 2026 Red Hat, Inc.