Re: [PATCH v8 03/14] drm/property: add a per-connector luminance flag

[Leo Li <sunpeng.li@amd.com>]

On 2026-09-22 04:32, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.09.26 um 06:40 schrieb Mario Limonciello:
>> The upcoming per-connector LUMINANCE range property needs to accept the
>> value 0 (to turn the display off on DPMS-off) even when its advertised
>> minimum is 1. The existing check special-cased a single device-wide
>> property object, which does not work once every connector owns its own
>> LUMINANCE property.
>>
>> Add a kernel-internal is_luminance flag on struct drm_property and key
>> the value-0 exception off it instead of a pointer comparison. The flag
>> is not exposed to userspace.
> 
> I don't think this is a good idea. Let userspace control display status and luminance independently.  To my understanding, both are independent devices. Just because the backlight is off doesn't mean that the display is off as well, (right ?)
> 
> Best regards
> Thomas

Yeah, backlight off and DPMS off are quite different. The CRTC is still active if backlight off, but that's not the case for DPMS.

I agree that we should detangle LUMINANCE from DPMS completely: Leave LUMINANCE alone when DPMS_OFF. If LUMINANCE is changed during DPMS_OFF, save the state in sw. Then on DPMS_ON, restore it to panel. Sysfs seems to have the same behavior today (I just tried it on my fw13 laptop).

It would be good to remove references to DPMS on the LUMINANCE property in patch 04/14 as well.

Thanks,
Leo

> 
>>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>>   drivers/gpu/drm/drm_property.c |  6 ++++++
>>   include/drm/drm_property.h     | 10 ++++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
>> index f38f2c5437e68..adccee10cfde4 100644
>> --- a/drivers/gpu/drm/drm_property.c
>> +++ b/drivers/gpu/drm/drm_property.c
>> @@ -952,6 +952,12 @@ bool drm_property_change_valid_get(struct drm_property *property,
>>       *ref = NULL;
>>         if (drm_property_type_is(property, DRM_MODE_PROP_RANGE)) {
>> +        /*
>> +         * Special case for the luminance property: allow 0 to turn the
>> +         * display off even when the normal range starts at 1.
>> +         */
>> +        if (property->is_luminance && value == 0 && property->values[1] > 0)
>> +            return true;
>>           if (value < property->values[0] || value > property->values[1])
>>               return false;
>>           return true;
>> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
>> index aa49b5a42bb56..8bb568a4cd485 100644
>> --- a/include/drm/drm_property.h
>> +++ b/include/drm/drm_property.h
>> @@ -188,6 +188,16 @@ struct drm_property {
>>        */
>>       struct drm_device *dev;
>>   +    /**
>> +     * @is_luminance:
>> +     *
>> +     * True for the per-connector LUMINANCE range property. Such a property
>> +     * additionally accepts the value 0 (to turn the display off) even when
>> +     * its minimum is 1. This is a kernel-internal flag and is not exposed
>> +     * to userspace.
>> +     */
>> +    bool is_luminance;
>> +
>>       /**
>>        * @enum_list:
>>        *
>