Re: [PATCH v6] drm/dp: clamp PWM bit count to advertised MIN and MAX capabilities

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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