Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The patch introduces a critical indexing mismatch by using a pipe index `j` derived from the newly evaluated `context` to fetch the corresponding pipe from the old `current_state`. Because pipe topology can change dynamically between states, this leads to NULL pointer dereferences or incorrectly powering down the wrong display's hardware pipeline. --
commit 4b109eb12ffb5f2fcc8ece3764e3caf46a2d8331 Author: Dominik Kaszewski <[email protected]> drm/amd/display: Fix DPMS using partially updated pipe context DPMS functions are modified to use the pipe context from current_state (the most recently programmed HW config) rather than the partially updated context. > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c > b/drivers/gpu/drm/amd/display/dc/core/dc.c > index 5f2d5638c8191..8793383e39410 100644 > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c > @@ -3690,27 +3690,34 @@ static void commit_planes_do_stream_update(struct dc > *dc, [ ... ] > + // DPMS should not use partially updated pipe context > + struct pipe_ctx *dpms_pipe_ctx = > &dc->current_state->res_ctx.pipe_ctx[j]; > + [Severity: High] Does this introduce an indexing mismatch between the old and new contexts? The commit_planes_do_stream_update() loop iterates over the new context using index j to find pipes for the stream being updated. However, because pipe topology can change dynamically between states during stream updates, index j in current_state might not map to the same stream. > if (stream_update->dpms_off) { > if (*stream_update->dpms_off) { > - dc->link_srv->set_dpms_off(pipe_ctx); > + > dc->link_srv->set_dpms_off(dpms_pipe_ctx); [Severity: High] If pipe j belonged to a different stream in current_state due to a dynamic reassignment, could this pass the wrong context and power down the incorrect physical display hardware? > /* for dpms, keep acquired resources*/ > - if (pipe_ctx->stream_res.audio && > !dc->debug.az_endpoint_mute_only) > - > pipe_ctx->stream_res.audio->funcs->az_disable(pipe_ctx->stream_res.audio); > + if (dpms_pipe_ctx->stream_res.audio && > !dc->debug.az_endpoint_mute_only) { > + struct audio *audio = > dpms_pipe_ctx->stream_res.audio; > + > + audio->funcs->az_disable(audio); > + } > > dc->optimized_required = true; > > } else { > if > (get_seamless_boot_stream_count(context) == 0) > dc->hwss.prepare_bandwidth(dc, > dc->current_state); > - > dc->link_srv->set_dpms_on(dc->current_state, pipe_ctx); > + > dc->link_srv->set_dpms_on(dc->current_state, dpms_pipe_ctx); > } > - } else if > (pipe_ctx->stream->link->wa_flags.blank_stream_on_ocs_change && > stream_update->output_color_space > - && !stream->dpms_off && > dc_is_dp_signal(pipe_ctx->stream->signal)) { > + } else if > (dpms_pipe_ctx->stream->link->wa_flags.blank_stream_on_ocs_change && > + stream_update->output_color_space && > + !stream->dpms_off && > dc_is_dp_signal(dpms_pipe_ctx->stream->signal)) { [Severity: High] Could this result in a NULL pointer dereference? If pipe j was completely unused in current_state prior to this commit, dpms_pipe_ctx->stream will be NULL, which would lead to a kernel panic when unconditionally dereferencing dpms_pipe_ctx->stream->link here. > /* > * Workaround for firmware issue in some > receivers where they don't pick up > * correct output color space unless DP link is > disabled/re-enabled > */ > - dc->link_srv->set_dpms_on(dc->current_state, > pipe_ctx); > + dc->link_srv->set_dpms_on(dc->current_state, > dpms_pipe_ctx); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
