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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.


BR,
Jani.


-- 
Jani Nikula, Intel