Re: [PATCH v11 04/12] drm/amd: add luminance property support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/amd/display: stale brightness restored in amdgpu_dm_commit_streams()
- [Medium] drm/amd/display: race and PSR exit in amdgpu_dm_backlight_restore()
- [Medium] drm/amd/display: zero actual_brightness collision in amdgpu_dm_set_luminance()
--

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

drm/amd: add luminance property support

This commit exposes the DRM LUMINANCE connector property for internal panels
when CONFIG_DRM_LUMINANCE is enabled and applies connector luminance updates
through Display Core backlight helpers and atomic commit callbacks.

> 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 56fca4e22df8d..2a9ddd1dc6f70 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

[ ... ]

> @@ -4802,12 +4799,8 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
>  	 * level. (the dm->brightness is the correct panel level)
>  	 * So we set the backlight level with dm->brightness value after set mode
>  	 */
> -	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 (set_backlight_level)
> +		amdgpu_dm_backlight_restore(dm);
>  }
>  
>  STATIC_IFN_KUNIT void
> @@ -5413,6 +5406,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
>  	/* Update audio instances for each connector. */
>  	amdgpu_dm_commit_audio(dev, state);
>  
> +	/* Apply connector luminance after the display pipeline is enabled. */
> +	drm_atomic_helper_commit_luminance_enables(state);
> +
>  	/* restore the backlight level */
>  	for (i = 0; i < dm->num_of_edps; i++) {
>  		if (dm->backlight_dev[i] &&

[Severity: Medium]
When a connector is controlled via the LUMINANCE property, dm->brightness[] is
only updated when drm_atomic_helper_commit_luminance_enables() calls
amdgpu_dm_set_luminance() near the end of amdgpu_dm_atomic_commit_tail().

If userspace updates LUMINANCE while the connector is inactive
(!new_conn_state->crtc or !new_crtc_state->active), or updates LUMINANCE in
the same atomic commit as a modeset, dm->brightness[bl_idx] still holds the
previous level when amdgpu_dm_commit_streams() runs earlier in
amdgpu_dm_atomic_commit_tail():

amdgpu_dm_atomic_commit_tail()
  amdgpu_dm_commit_streams()
    amdgpu_dm_backlight_restore()
      amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i])
  amdgpu_dm_mod_power_setup_streams()
  amdgpu_dm_commit_planes()
  ...
  drm_atomic_helper_commit_luminance_enables()
    amdgpu_dm_set_luminance()
      amdgpu_dm_backlight_set_level(dm, bl_idx, state->luminance)

Because drm_atomic_helper_update_legacy_modeset_state() has already set
aconnector->base.encoder and dc_commit_streams() has committed the stream,
amdgpu_dm_backlight_restore() programs the panel hardware,
dm->power_module->bl_state, and the ATOMBIOS scratch register with the stale
dm->brightness[i] value before planes are committed, and then
drm_atomic_helper_commit_luminance_enables() reprograms the hardware a second
time with state->luminance.

Furthermore, if a LUMINANCE change was committed while the connector was
inactive and a later modeset exits self-refresh
(old_crtc_state->self_refresh_active is true while old_conn_state->luminance ==
new_conn_state->luminance), drm_atomic_helper_commit_luminance_enables() skips
calling set_luminance, leaving the panel permanently at the stale
dm->brightness[i] level applied by amdgpu_dm_backlight_restore().

Should amdgpu_dm_commit_streams() avoid restoring stale dm->brightness[] for
luminance-controlled connectors, or should dm->brightness[] be synced from the
connector state before amdgpu_dm_backlight_restore() runs?

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

[ ... ]

> @@ -675,6 +684,30 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
>  
>  	amdgpu_dm_update_connector_ext_caps(aconnector);
>  
> +	/*
> +	 * The property must exist before the device is registered. If ACPI
> +	 * video should handle the backlight, leave it to
> +	 * amdgpu_dm_register_backlight_device() to register the fallback.
> +	 */
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE) && acpi_video_backlight_use_native()) {
> +		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);
> +		/* Start from the level the hardware is actually at */
> +		ret = amdgpu_dm_backlight_get_level(dm, bl_idx);
> +		if (ret >= 0 && ret <= props.max_brightness)
> +			props.brightness = ret;
> +		ret = drm_connector_attach_luminance_property(connector, 0,
> +							      props.max_brightness,
> +							      props.brightness);
> +		if (!drm_WARN_ON(adev_to_drm(dm->adev), ret)) {
> +			dm->brightness[bl_idx] = props.brightness;
> +			dm->luminance[bl_idx] = true;
> +		}
> +	}
> +
>  	/* 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.
> @@ -686,6 +719,45 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
>  }
>  EXPORT_IF_KUNIT(amdgpu_dm_setup_backlight_device);
>  
> +void amdgpu_dm_set_luminance(const struct drm_connector_state *state)
> +{
> +	struct amdgpu_dm_connector *aconnector =
> +		to_amdgpu_dm_connector(state->connector);
> +	struct amdgpu_display_manager *dm =
> +		&drm_to_adev(state->connector->dev)->dm;
> +
> +	int bl_idx = aconnector->bl_idx;
> +
> +	if (bl_idx < 0)
> +		return;
> +
> +	/* Skip if already applied, e.g. by amdgpu_dm_backlight_restore(). */
> +	if (dm->brightness[bl_idx] == state->luminance &&
> +	    dm->actual_brightness[bl_idx] == state->luminance)
> +		return;
> +
> +	amdgpu_dm_backlight_set_level(dm, bl_idx, state->luminance);
> +}

[Severity: Medium]
Can amdgpu_dm_set_luminance() silently skip programming the hardware when
state->luminance is 0?

In amdgpu_dm_setup_backlight_device(), drm_connector_attach_luminance_property()
registers 0 as the minimum valid luminance level, and dm->actual_brightness[]
starts at 0 (unlike amdgpu_dm_register_backlight_device(), which seeds
dm->actual_brightness[bl_idx] from the hardware level). In addition,
amdgpu_dm_backlight_set_level() uses dm->actual_brightness[bl_idx] = 0 as the
sentinel indicating that brightness was not applied while the connector had no
encoder:

		/* if connector is off, save the brightness for next time it's on */
		if (!aconnector->base.encoder) {
			dm->brightness[bl_idx] = user_brightness;
			dm->actual_brightness[bl_idx] = 0;
			return;
		}

If dm->brightness[bl_idx] is 0 and dm->actual_brightness[bl_idx] is 0 (for
instance, after saving brightness 0 while the connector was disabled, or if a
prior amdgpu_dm_backlight_set_level(dm, bl_idx, 0) returned early or failed
with rc == false), then calling amdgpu_dm_set_luminance() with
state->luminance == 0 sees:

	if (dm->brightness[bl_idx] == state->luminance &&
	    dm->actual_brightness[bl_idx] == state->luminance)
		return;

and returns early without applying minimum brightness to the panel.

Also, in amdgpu_dm_atomic_commit_tail(), the fallback loop that retries
unapplied brightness levels only checks dm->backlight_dev[i]:

	/* 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]);
	}

Should dm->actual_brightness[] track whether a level has been applied without
colliding with valid luminance 0, and should the retry loop in
amdgpu_dm_atomic_commit_tail() also cover dm->luminance[i]?

> +
> +/**
> + * amdgpu_dm_backlight_restore - reprogram the cached backlight levels
> + * @dm: display manager
> + *
> + * Display Core may reset the panel brightness when committing streams, so
> + * reapply the cached level of every panel whose backlight is controlled by
> + * the driver, either through a backlight class device or through the
> + * connector LUMINANCE property.
> + */
> +void amdgpu_dm_backlight_restore(struct amdgpu_display_manager *dm)
> +{
> +	int i;
> +
> +	for (i = 0; i < dm->num_of_edps; i++) {
> +		if (dm->backlight_dev[i] || dm->luminance[i])
> +			amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> +	}
> +}

[Severity: Medium]
Could calling amdgpu_dm_backlight_restore() from amdgpu_dm_commit_streams() on
every stream commit cause a race with concurrent luminance updates and trigger
unnecessary PSR/Replay/IPS exits on unaffected eDP panels?

First, when a non-blocking modeset commit runs on an external display CRTC,
do_aquire_global_lock() locks all modeset locks during atomic_check but does
not add unmodified CRTCs (such as an active eDP CRTC) to the atomic state.
Once drm_atomic_helper_swap_state() finishes and locks are dropped, a subsequent
fast atomic commit updating LUMINANCE on the eDP connector drops
dm->atomic_obj in amdgpu_dm_atomic_check() and runs amdgpu_dm_atomic_commit_tail()
concurrently:

CPU 0 (external CRTC modeset)           CPU 1 (eDP fast luminance commit)
amdgpu_dm_commit_streams()              drm_atomic_helper_commit_luminance_enables()
  amdgpu_dm_backlight_restore()           amdgpu_dm_set_luminance()
    reads old dm->brightness[0]             amdgpu_dm_backlight_set_level(new)
    amdgpu_dm_backlight_set_level(old)        dm->brightness[0] = new
      dm->brightness[0] = old                 dm->actual_brightness[0] = new
      overwrites hw & actual_brightness

Because nothing serializes dm->brightness[] and dm->actual_brightness[] across
these two commits, CPU 0 can overwrite the panel brightness back to the old
level after CPU 1 has programmed the new luminance, leaving hardware brightness
out of sync with connector->state->luminance.

Second, whenever an external monitor undergoes a modeset while the eDP panel is
idle in PSR, Replay, or IPS with unchanged luminance,
amdgpu_dm_commit_streams() sets set_backlight_level = true and calls
amdgpu_dm_backlight_restore(), which unconditionally calls
amdgpu_dm_backlight_set_level() on all eDP panels. That exits IPS and latches
psr_event_hw_programming and replay_event_hw_programming on the eDP stream,
which is not cleared because the eDP CRTC has no plane commit in the state to
run amdgpu_dm_enable_self_refresh().

Could amdgpu_dm_commit_streams() only restore backlight for eDP streams that
actually underwent a modeset in that commit, or let
drm_atomic_helper_commit_luminance_enables() handle luminance-controlled
connectors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-superm1-backlight-property-v11-0-00cce7b2ba22@kernel.org?part=4