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.armstr...@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 <jo...@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 <jo...@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
