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

[Maxime Ripard]

On Mon, Nov 17, 2025 at 11:05:10AM +0200, Jani Nikula wrote:
> On Fri, 14 Nov 2025, Mario Limonciello <mario.limonciello@xxxxxxx> 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@xxxxxxx/
> >
> > 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@xxxxxxxxxx/
> >
> > Xaver had some change of heart and wanted to talk about it at the next 
> > hackfest.
> >
> > https://lore.kernel.org/amd-gfx/CAFZQkGxUwodf5bW0qQkXoPoz0CFFA1asJfUxFftMGgs5-VK2Hw@xxxxxxxxxxxxxx/
> >
> > 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@xxxxxxxxxx/
> >
> > Userspace implementation was done and so we merged this for 6.19.
> >
> > https://lore.kernel.org/amd-gfx/CAFZQkGwLWcyS0SqCHoiGsJd5J_u4aBJ0HMV5Bx3NknLdLkr8Uw@xxxxxxxxxxxxxx/
> >
> > 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

Attachment: signature.asc
Description: PGP signature