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

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

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