Re: [PATCH v2 4/5] drm/amd/display: Commit panel_power_savings writes from the kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amd/display: overlay plane loss in
  panel_power_savings_apply() with VRR
--

commit f07b9f0f5284e550d50d58da3cfcd7dcb7eae3b4
Author: Hugo Sales <hugo@hsal.es>

drm/amd/display: Commit panel_power_savings writes from the kernel

This commit updates panel_power_savings_store() to apply ABM level changes
through a non-modeset atomic commit instead of sending a hotplug uevent.

> 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 27546febf90d3..8487ac0661396 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

[ ... ]

> @@ -712,13 +727,100 @@ ssize_t panel_power_savings_show(struct device *device,
>  }
>  EXPORT_IF_KUNIT(panel_power_savings_show);
>  
> +static int panel_power_savings_apply(struct drm_connector *connector,
> +				     unsigned int level)
> +{
> +	struct drm_connector_state *conn_state;
> +	struct dm_connector_state *dm_state;
> +	struct drm_device *dev = connector->dev;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_commit *state;
> +	bool forbidden, unchanged;
> +	int ret, idx;

[ ... ]

> +retry:
> +	conn_state = drm_atomic_get_connector_state(state, connector);
> +	if (IS_ERR(conn_state)) {
> +		ret = PTR_ERR(conn_state);
> +		goto out;
> +	}
> +
> +	dm_state = to_dm_connector_state(conn_state);
> +	if (dm_state->abm_sysfs_forbidden) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	if (dm_state->abm_level == level) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	dm_state->abm_level = level;
> +
> +	ret = drm_atomic_commit(state);

[Severity: High]
Can this commit drop active overlay planes from DC when VRR is enabled on
the CRTC?

panel_power_savings_apply() builds a non-modeset commit containing only
the connector and its CRTC, with no planes.  In amdgpu_dm_atomic_check(),
the loop that pulls in all primary and overlay planes on a CRTC runs
before drm_atomic_helper_check_planes(), while state still has no planes,
so it skips the CRTC.  Later, drm_atomic_helper_check_planes() calls
amdgpu_dm_crtc_helper_atomic_check():

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c:amdgpu_dm_crtc_helper_atomic_check() {
	...
	if (!state->legacy_cursor_update && amdgpu_dm_crtc_vrr_active(dm_crtc_state)) {
		struct drm_plane_state *primary_state;

		/* Pull in primary plane for correct VRR handling */
		primary_state = drm_atomic_get_plane_state(state, crtc->primary);
		if (IS_ERR(primary_state))
			return PTR_ERR(primary_state);
	}
	...
}

That adds only crtc->primary to state after the plane-gathering loop has
already run.  In amdgpu_dm_commit_planes(), planes_count is then 1, and
update_planes_and_stream_state() sees stream_status->plane_count !=
surface_count, elevates the update to UPDATE_TYPE_FULL, and calls
dc_state_rem_all_planes_for_stream() followed by
dc_state_add_all_planes_for_stream() with only the primary plane,
detaching any enabled overlay plane in DC while DRM state still tracks it
as active.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-abm-sysfs-commit-v2-0-5808d17a7e89@hsal.es?part=4