Re: [PATCH 1/2] drm/amd/display: Skip connector updates without a stream

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The function `amdgpu_dm_atomic_commit_tail` uses `kzalloc` with `GFP_KERNEL` and acquires a `mutex`, both of which can sleep, despite being an atomic commit path.
- [High] AMDGPU never updates the privacy screen hardware state because privacy screen updates force a modeset, but the commit tail skips privacy screen updates during modesets.
--

--- Patch [1]: [PATCH 1/2] drm/amd/display: Skip connector updates without a stream ---
commit 3a3a91e5bab6bdb82ebcebf89c73cd8b740ca904
Author: David Weber <weber.aulendorf@gmail.com>

drm/amd/display: Skip connector updates without a stream
    
Atomic DPMS can leave a connector assigned to an enabled but inactive
CRTC after the driver has removed its DC stream. A later atomic commit
can then change HDR_OUTPUT_METADATA or Broadcast RGB while leaving the
CRTC mode, routing and active state unchanged.

This patch skips stream updates until a stream exists, preventing null
pointer dereferences when the connector stream-update path is reached
with a NULL stream.

> 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 567bf04979ee5..712e8d3e0a01d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5050,6 +5050,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
>  
>  		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>  		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);

[Severity: High]
This isn't a bug introduced by this patch, but does this skip logic prevent
privacy screen updates?

In amdgpu_dm_connector_atomic_check(), any change to the privacy screen
software state forces a modeset:

    new_crtc_state->mode_changed = true;

However, earlier in this loop in amdgpu_dm_atomic_commit_tail(), connectors
undergoing a modeset are skipped:

    /* Skip any modesets/resets */
    if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
        continue;

Since the only call to update the privacy screen hardware is located at the
very end of this loop:

    drm_connector_update_privacy_screen(new_con_state);

Will this cause privacy screen updates to be permanently skipped because they
force a modeset that bypasses the update code?

> +		/* DPMS-off leaves the connector routed to a streamless CRTC. */
> +		if (!dm_new_crtc_state->stream)
> +			continue;
>  
>  		scaling_changed = is_scaling_state_different(dm_new_con_state,
>  							     dm_old_con_state);

[Severity: High]
This is a pre-existing issue, but are there sleeping functions called from
this atomic context?

Later in amdgpu_dm_atomic_commit_tail(), kzalloc is called with GFP_KERNEL:

    dummy_updates = kzalloc(sizeof(struct dc_surface_update) * MAX_SURFACES, GFP_KERNEL);

And then a mutex is acquired:

    mutex_lock(&dm->dc_lock);

Since drm_atomic_helper_commit_tail() and its variants execute in an atomic
context where sleeping is forbidden, could this kzalloc(..., GFP_KERNEL) and
mutex_lock() cause kernel warnings, system instability, or potential
deadlocks?

Also, if the memory allocation fails, the subsequent hardware programming in
dc_update_planes_and_stream() is silently skipped. Can this lead to an
unrecoverable state mismatch since commit_tail cannot return error codes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813112634.11455-1-weber.aulendorf@gmail.com?part=1