Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/amd: incomplete fix, plane programming still executes after commit failure - [Medium] drm/amd/display: hdmi_frl_status_polling_work leaks on commit failure --
commit 5df85d959754dc46c8ae54787ba1fc6639162940 Author: Wang Jiang <[email protected]> drm/amd/display: Don't ignore dc_commit_streams() failures This patch modifies the AMD DRM driver to stop ignoring failures from dc_commit_streams(). It logs the failure and adds an early return in amdgpu_dm_commit_streams() to avoid programming plane state against a half-committed stream list. It also logs failures in the reset recovery path in dm_resume(). > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c [ ... ] > @@ -4735,7 +4740,18 @@ static void amdgpu_dm_commit_streams(struct > drm_atomic_commit *state, > dm_enable_per_frame_crtc_master_sync(dc_state); > mutex_lock(&dm->dc_lock); > dc_exit_ips_for_hw_access(dm->dc); > - WARN_ON(!dc_commit_streams(dm->dc, ¶ms)); > + if (dc_commit_streams(dm->dc, ¶ms) != DC_OK) { > + mutex_unlock(&dm->dc_lock); > + /* > + * DC refused the stream list, so the hardware and the DC state > + * no longer agree. Bail out instead of programming more > + * stream/plane state for a half committed stream list (this > + * used to be a silently ignored WARN_ON()). > + */ > + drm_err(dev, "DC failed to commit %u stream(s)\n", > + (unsigned int)params.stream_count); > + return; [Severity: High] Will this early return successfully stop the plane programming? Since amdgpu_dm_commit_streams() returns void, it cannot propagate this failure state back to its caller. Looking at the caller amdgpu_dm_atomic_commit_tail(): amdgpu_dm_atomic_commit_tail() { ... if (dm_state && dm_state->context) { dc_state = dm_state->context; amdgpu_dm_commit_streams(state, dc_state); } ... if (dm_new_crtc_state->stream) amdgpu_dm_commit_planes(state, dev, dm, crtc, wait_for_vblank); } Because the tail continues unconditionally, won't it still program the planes against the rejected stream list, resulting in the exact crash this patch intended to prevent? [Severity: Medium] Does this early return cause the HDMI FRL status polling work to leak? By returning early, we bypass the subsequent block that manages the FRL polling work: if (frl_stream_found) { queue_delayed_work(...) } else { if (cancel_delayed_work_sync(&dm->hdmi_frl_status_polling_work)) ... } Looking at hdmi_frl_status_polling_work(), the handler unconditionally re-arms itself by calling queue_delayed_work() at the end of its execution. If we fail the stream commit and return early, doesn't it leave the delayed work to run and wake up the CPU indefinitely on a recoverable failure path? > + } > > bool frl_stream_found = false; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
