Re: [PATCH v10 03/15] drm/amd: add luminance property support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/amdgpu: broken atomic LUMINANCE support on non-atomic legacy display
- [High] drm/amd/display: skipped backlight restore and setup when DRM_LUMINANCE=y
- [Medium] drm/amd/display: unconditional backlight programming blocks PSR/Replay
- [Low] drm/amd: Kconfig and link breakage when DRM_LUMINANCE=y without backlight
--

commit de26f6665aed29ccc412fe4be3f0737b06b98759
Author: Mario Limonciello <mario.limonciello@amd.com>

drm/amd: add luminance property support

This commit adds support for the DRM LUMINANCE connector property on internal
panels when CONFIG_DRM_LUMINANCE is enabled, bypassing sysfs backlight device
registration and wiring connector luminance callbacks for Display Core and
legacy display paths.

> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> index 50305630f5335..7a6a049aa6e90 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> @@ -20,7 +20,7 @@ config DRM_AMDGPU
>  	select I2C
>  	select I2C_ALGOBIT
>  	select CRC16
> -	select BACKLIGHT_CLASS_DEVICE
> +	select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
>  	select INTERVAL_TREE
>  	select DRM_BUDDY
>  	select DRM_SUBALLOC_HELPER

[Severity: Low]
Does dropping select BACKLIGHT_CLASS_DEVICE when CONFIG_DRM_LUMINANCE is
enabled cause Kconfig dependency warnings and link errors when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or built as a module with
CONFIG_DRM_AMDGPU=y)?

In drivers/gpu/drm/amd/amdgpu/Kconfig, config DRM_AMDGPU still unconditionally
selects ACPI_VIDEO when ACPI is enabled:

	# decidable, for the moment, to just select all of ACPI_VIDEO's
	# dependencies, which is fewer than you might think.
	select ACPI_VIDEO if ACPI

Because config ACPI_VIDEO in drivers/acpi/Kconfig depends on
BACKLIGHT_CLASS_DEVICE, enabling CONFIG_DRM_LUMINANCE=y with CONFIG_ACPI=y and
CONFIG_BACKLIGHT_CLASS_DEVICE=n triggers an unmet direct dependency warning
for ACPI_VIDEO.

In addition, include/linux/backlight.h declares backlight_device_register(),
backlight_device_unregister(), and backlight_device_set_brightness() without
inline stubs when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, while amdgpu
still compiles direct references to those symbols:

- amdgpu_dm_register_backlight_device() in
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c is a non-static
  function compiled unconditionally that calls backlight_device_register().
- amdgpu_dm_connector_destroy() in
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c and
  amdgpu_atombios_encoder_fini_backlight() in
  drivers/gpu/drm/amd/amdgpu/atombios_encoders.c call
  backlight_device_unregister() behind runtime pointer checks.
- amdgpu_atif_handler() in drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c calls
  backlight_device_set_brightness(atif->bd, req.backlight_level) behind a
  runtime check on atif->bd.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> index 92c98e999efe9..209a7a6c246db 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> @@ -798,11 +798,31 @@ static int amdgpu_connector_set_lcd_property(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +static void amdgpu_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct drm_encoder *encoder =
> +		amdgpu_connector_best_single_encoder(conn_state->connector);
> +
> +	if (encoder)
> +		amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder),
> +							    conn_state->luminance);
> +}
> +
> +static void amdgpu_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct drm_encoder *encoder =
> +		amdgpu_connector_best_single_encoder(conn_state->connector);
> +
> +	if (encoder)
> +		amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0);
> +}
>  
>  static const struct drm_connector_helper_funcs amdgpu_connector_lvds_helper_funcs = {
>  	.get_modes = amdgpu_connector_lvds_get_modes,
>  	.mode_valid = amdgpu_connector_lvds_mode_valid,
>  	.best_encoder = amdgpu_connector_best_single_encoder,
> +	.set_luminance = amdgpu_connector_set_luminance,
> +	.disable_luminance = amdgpu_connector_disable_luminance,
>  };
>  
>  static const struct drm_connector_funcs amdgpu_connector_lvds_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> index a1d05df0ac99b..dbff532ac1bcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> @@ -191,6 +191,21 @@ void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder *amdgpu_encode
>  		goto register_acpi_backlight;
>  	}
>  
> +	dig = amdgpu_encoder->enc_priv;
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		unsigned int brightness;
> +
> +		brightness = amdgpu_atombios_encoder_get_backlight_level(amdgpu_encoder);
> +		if (drm_connector_attach_luminance_property(drm_connector, 0,
> +							    AMDGPU_MAX_BL_LEVEL,
> +							     brightness))
> +			return;
> +
> +		dig->backlight_level = brightness;
> +		dig->bl_dev = NULL;
> +		return;
> +	}

[Severity: High]
Can atomic LUMINANCE work on the legacy non-DC display path in
drivers/gpu/drm/amd/amdgpu/atombios_encoders.c and amdgpu_connectors.c?

Those files are used only when Display Core is not active, where
amdgpu_pci_probe() in drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c clears
DRIVER_ATOMIC:

	if (!supports_atomic)
		ddev->driver_features &= ~DRIVER_ATOMIC;

On this legacy path, connectors do not implement atomic state callbacks
(.reset, .atomic_duplicate_state, .atomic_destroy_state), so
drm_connector->state is NULL, and modesets use legacy CRTC helpers
(drm_crtc_helper_set_config() and drm_helper_connector_dpms()).

Because drm_connector_attach_luminance_property() creates LUMINANCE with
DRM_MODE_PROP_ATOMIC, non-atomic drivers hide LUMINANCE from userspace, legacy
property updates go through amdgpu_connector_set_lcd_property() (which ignores
everything except scaling_mode_property), and the legacy helpers never invoke
drm_atomic_helper_commit_luminance_enables() or
drm_atomic_helper_commit_luminance_disables(). Skipping backlight_device_register()
in amdgpu_atombios_encoder_init_backlight() therefore leaves legacy panels
with no working brightness control when CONFIG_DRM_LUMINANCE=y.

Even if atomic callbacks were reachable here, there are two additional issues
in drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:

1. .set_luminance and .disable_luminance are only added to
   amdgpu_connector_lvds_helper_funcs and are missing from
   amdgpu_connector_dp_helper_funcs, which is used by DRM_MODE_CONNECTOR_eDP
   and DP-bridge LVDS connectors that also call
   amdgpu_atombios_encoder_init_backlight().
2. amdgpu_connector_disable_luminance() calls
   amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0),
   which overwrites dig->backlight_level = 0 and mmBIOS_SCRATCH_2 = 0,
   clobbering the saved brightness level used by
   amdgpu_atombios_encoder_setup_dig() on enable and by dce_v*_suspend().

[ ... ]

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 0ea45ab12119c..53ff9a55c3a8e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

[ ... ]

> @@ -5397,11 +5398,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
>  	/* Update audio instances for each connector. */
>  	amdgpu_dm_commit_audio(dev, state);
>  
> -	/* restore the backlight level */
> -	for (i = 0; i < dm->num_of_edps; i++) {
> -		if (dm->backlight_dev[i] &&
> -		    (dm->actual_brightness[i] != dm->brightness[i]))
> -			amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		/* Apply brightness after the display pipeline is enabled. */
> +		drm_atomic_helper_commit_luminance_enables(state);
> +	} else {
> +		/* Restore the legacy backlight level. */
> +		for (i = 0; i < dm->num_of_edps; i++) {
> +			if (dm->backlight_dev[i] &&
> +			    dm->actual_brightness[i] != dm->brightness[i])
> +				amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> +		}
>  	}

[Severity: Medium]
Unlike the !CONFIG_DRM_LUMINANCE branch, which checks
dm->actual_brightness[i] != dm->brightness[i], doesn't calling
drm_atomic_helper_commit_luminance_enables(state) -> amdgpu_dm_set_luminance() ->
amdgpu_dm_backlight_set_level() here reprogram the backlight on every atomic
commit that includes an active eDP connector in state even when
state->luminance has not changed?

In amdgpu_dm_atomic_commit_tail(), drm_atomic_helper_commit_luminance_enables()
runs after amdgpu_dm_commit_planes(). Inside amdgpu_dm_backlight_set_level()
in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c, the driver
exits IPS, reprograms the backlight via mod_power_set_backlight_nits() or
mod_power_set_backlight_percent(), and disables PSR and Panel Replay until the
next plane flip:

	amdgpu_dm_psr_set_event(dm, stream, true,
		psr_event_hw_programming, true);
	amdgpu_dm_replay_set_event(dm, stream, true,
		replay_event_hw_programming, true);

Because amdgpu_dm_commit_planes() -> amdgpu_dm_enable_self_refresh() has
already run earlier in amdgpu_dm_atomic_commit_tail(), any commit that pulls
the active eDP connector into state without changing state->luminance (such as
CRTC color management or VRR updates via drm_atomic_add_affected_connectors(),
ABM/HDR/scaling connector property updates, or compositor commits that include
connector properties) will immediately re-assert psr_event_hw_programming and
replay_event_hw_programming and block PSR/Replay entry.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index 0f664c658a127..0857354e7327e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c

[ ... ]

> @@ -657,6 +662,10 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
>  {
>  	struct dc_link *link = aconnector->dc_link;
>  	int bl_idx = dm->num_of_edps;
> +	struct backlight_properties props = { 0 };
> +	struct amdgpu_dm_backlight_caps *caps;
> +	struct drm_connector *connector = &aconnector->base;
> +	int ret;
>  
>  	if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
>  	    link->type == dc_connection_none)
> @@ -675,6 +684,19 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
>  
>  	amdgpu_dm_update_connector_ext_caps(aconnector);
>  
> +	caps = &dm->backlight_caps[bl_idx];
> +	amdgpu_dm_backlight_fill_props(caps, power_supply_is_system_supplied() > 0,
> +				       !(amdgpu_dc_debug_mask &
> +					 DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE),
> +				       &props);
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		unsigned int max = props.max_brightness;
> +		unsigned int value = props.brightness;
> +
> +		ret = drm_connector_attach_luminance_property(connector, 0, max, value);
> +		drm_WARN_ON(adev_to_drm(dm->adev), ret);
> +	}
> +
>  	/* Offer ABM property when user didn't turn off by module parameter.
>  	 * OLED panels are included to support CACP (Content Adaptive
>  	 * Contrast and Power) feature via set_abm_level.

[ ... ]

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index b498fc5fbb140..f2c2a2e730d3e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c

[ ... ]

> @@ -2038,7 +2039,8 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
>  			return r;
>  	}
>  
> -	amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
> +	if (!IS_ENABLED(CONFIG_DRM_LUMINANCE))
> +		amdgpu_dm_register_backlight_device(amdgpu_dm_connector);

[Severity: High]
When CONFIG_DRM_LUMINANCE is enabled, skipping
amdgpu_dm_register_backlight_device() leaves dm->backlight_dev[i] as NULL and
leaves dm->brightness[bl_idx] uninitialized (0) in
amdgpu_dm_setup_backlight_device(). Doesn't this break several backlight
restoration and initialization paths in Display Core?

Specifically:

1. In dm_resume() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, the
   GPU reset recovery branch (if (amdgpu_in_reset(adev))) does not go through
   drm_atomic_helper_resume() or amdgpu_dm_atomic_commit_tail(), and instead
   restores panel brightness via:

		/* set the backlight after a reset */
		for (i = 0; i < dm->num_of_edps; i++) {
			if (dm->backlight_dev[i])
				amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
		}

   Because dm->backlight_dev[i] is NULL (and dm->brightness[i] is 0 until a
   luminance commit occurs), panel brightness will not be restored after a GPU
   reset.

2. In amdgpu_dm_commit_streams() in
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, after dc_commit_streams()
   is called, backlight restoration is likewise gated on dm->backlight_dev[i]:

	if (set_backlight_level) {
		for (i = 0; i < dm->num_of_edps; i++) {
			if (dm->backlight_dev[i])
				amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
		}
	}

   If a modeset happens on an external display on another CRTC while the eDP
   connector is not part of the atomic commit state, neither
   amdgpu_dm_commit_streams() nor drm_atomic_helper_commit_luminance_enables(state)
   will restore the eDP brightness.

3. Unlike amdgpu_dm_register_backlight_device(), amdgpu_dm_setup_backlight_device()
   does not check acpi_video_backlight_use_native() before attaching the
   LUMINANCE property, and skipping amdgpu_dm_register_backlight_device() also
   skips the acpi_video_register_backlight() fallback when native backlight
   control should not be used.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=3