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...
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
