Re: [PATCH] drm/amd: Move adaptive backlight modulation property to drm core

[Mario Limonciello <mario.limonciello@amd.com>]

On 11/18/2025 2:47 AM, Maxime Ripard wrote:
> On Mon, Nov 17, 2025 at 11:05:10AM +0200, Jani Nikula wrote:
>> On Fri, 14 Nov 2025, Mario Limonciello <mario.limonciello@amd.com> wrote:
>>> +Xaver
>>>
>>> On 11/14/2025 2:39 AM, Jani Nikula wrote:
>>> <snip>
>>>
>>>>>
>>>>> So this is basically Content Adaptive Brightness Control, but with the
>>>>> technology ("backlight" and "modulation") unnecessarily encoded in the
>>>>> ABI.
>>>>>
>>>>> You could have the same knob for adjusting CABC implemented in an OLED
>>>>> panel, controlled via DPCD.
>>>>>
>>>>>> + *
>>>>>> + *	sysfs
>>>>>> + *		The ABM property is exposed to userspace via sysfs interface
>>>>>> + *		located at 'amdgpu/panel_power_savings' under the DRM device.
>>>>>
>>>>> Err what? Seriously suggesting that to the common ABI? We shouldn't have
>>>>> sysfs involved at all, let alone vendor specific sysfs.
>>>>>
>>>>>> + *	off
>>>>>> + *		Adaptive backlight modulation is disabled.
>>>>>> + *	min
>>>>>> + *		Adaptive backlight modulation is enabled at minimum intensity.
>>>>>> + *	bias min
>>>>>> + *		Adaptive backlight modulation is enabled at a more intense
>>>>>> + *		level than 'min'.
>>>>>> + *	bias max
>>>>>> + *		Adaptive backlight modulation is enabled at a more intense
>>>>>> + *		level than 'bias min'.
>>>>>> + *	max
>>>>>> + *		Adaptive backlight modulation is enabled at maximum intensity.
>>>>>
>>>>> So values 0-4 but with names. I don't know what "bias" means here, and I
>>>>> probably shouldn't even have to know. It's an implementation detail
>>>>> leaking to the ABI.
>>>>>
>>>>> In the past I've encountered CABC with different modes based on the use
>>>>> case, e.g. "video" or "game", but I don't know how those would map to
>>>>> the intensities.
>>>>>
>>>>> I'm concerned the ABI serves AMD hardware, no one else, and everyone
>>>>> else coming after this is forced to shoehorn their implementation into
>>>>> this.
>>>>
>>>> Apparently Harry had the same concerns [1].
>>>>
>>> So let me explain how we got here.  At the display next hackfest last
>>> year (2024) we talked about how to get compositors to indicate they want
>>> technologies like this to get out the way.  A patch series was made that
>>> would allow compositor to say "Require color accuracy" or "Require low
>>> latency" are required.
>>>
>>> https://lore.kernel.org/amd-gfx/20240703051722.328-1-mario.limonciello@amd.com/
>>>
>>> This got reverted because userspace didn't have an implementation ready
>>> to go at the time.  One was created and so I rebased/resent the series
>>> earlier this year.
>>>
>>> https://lore.kernel.org/amd-gfx/20250621152657.1048807-1-superm1@kernel.org/
>>>
>>> Xaver had some change of heart and wanted to talk about it at the next
>>> hackfest.
>>>
>>> https://lore.kernel.org/amd-gfx/CAFZQkGxUwodf5bW0qQkXoPoz0CFFA1asJfUxFftMGgs5-VK2Hw@mail.gmail.com/
>>>
>>> So we talked about it again at the hackfest this year and the decision
>>> was not everyone can an octagon into a peg hole, so we're better off
>>> re-introducing vendor properties for this.  So series was respun per
>>> that discussion.
>>>
>>> https://lore.kernel.org/amd-gfx/20250718192045.2091650-1-superm1@kernel.org/
>>>
>>> Userspace implementation was done and so we merged this for 6.19.
>>>
>>> https://lore.kernel.org/amd-gfx/CAFZQkGwLWcyS0SqCHoiGsJd5J_u4aBJ0HMV5Bx3NknLdLkr8Uw@mail.gmail.com/
>>>
>>> Then Simona suggested we need to make some changes where the propertye
>>> should be in generic documentation etc:
>>>
>>> https://lore.kernel.org/amd-gfx/aQUz-mbM_WlXn_uZ@phenom.ffwll.local/
>>>
>>> So that's where we are now with this patch.  I can clean it up per the
>>> feedback so far - but I think we need to be in agreement that this
>>> property is actually the way forward or we should revert the property in
>>> amdgpu instead of this moving approach and keep discussing.
>>
>> IMO we should either
>>
>> - admit we can't do a generic property for this *and* keep the vendor
>>    specific property details hidden in drivers, or
>>
>> - figure out a generic property and add that in drm core
>>
>> But I'm pretty much against adding an AMD vendor specific property in
>> drm core.
> 
> Agreed
> 
> Maxime

OK - I will discard this patch.

Simona, please any comments if you still want to see anything change 
from the vendor property.