Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Lockless state machine corruption on overlapping writeback jobs stalls fences and leaks vblank references. Pre-existing issues: - [High] Unconditional memory leak of dc_writeback_info structure in dm_set_writeback. -- --- Patch [1]: [PATCH] drm/amd/display: Fix writeback completion timing --- commit d7178e82119f1d066c5082ca676728119bef3a13 Author: Alex Hung <[email protected]> drm/amd/display: Fix writeback completion timing Hold a vblank reference while writeback is pending and signal the out fence on the second vblank instead of using mdelay(). Add amdgpu_dm_crtc_complete_writeback() to finish and clean up writeback from both the IRQ and teardown paths. > 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 b97ceabe61736..90ee250e6e360 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c [ ... ] > @@ -4988,6 +4989,7 @@ static void dm_set_writeback(struct > amdgpu_display_manager *dm, > * cannot run its matching vblank_put before this get. > */ > WARN_ON(drm_crtc_vblank_get(&acrtc->base)); > + acrtc->wb_frame_done = false; > acrtc->wb_pending = true; [Severity: High] Does this lockless state update cause issues if userspace submits another writeback job immediately? Since page flip events can signal on the first vblank, userspace could submit a new atomic commit before the second vblank occurs. If a new job is submitted in this window, dm_set_writeback() will overwrite acrtc->wb_frame_done = false and acrtc->wb_pending = true without holding a lock to protect the active job's state. [Severity: High] This is a pre-existing issue, but does this function leak the memory allocated for wb_info? Earlier in dm_set_writeback(), wb_info is allocated: wb_info = kzalloc(sizeof(*wb_info), GFP_KERNEL); Then it is passed to dc_stream_add_writeback(): dc_stream_add_writeback(dm->dc, crtc_state->stream, wb_info); Since dc_stream_add_writeback() copies the structure by value into the stream's array and does not take ownership of the pointer, it seems the originally allocated wb_info pointer is never freed before returning. Could this lead to continuous kernel memory leakage when userspace streams writeback? > } > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c > index c5467f34c51f1..4de7fb264cb26 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c > @@ -1974,23 +1974,26 @@ static void dm_crtc_high_irq(void *interrupt_params) > return; > > if (acrtc->wb_conn && acrtc->wb_pending) { > - struct dc_stream_state *stream = acrtc->dm_irq_params.stream; > - unsigned int v_total, refresh_hz; > - > - v_total = stream->adjust.v_total_max ? > - stream->adjust.v_total_max : stream->timing.v_total; > - refresh_hz = div_u64((uint64_t) stream->timing.pix_clk_100hz * > - 100LL, (v_total * stream->timing.h_total)); > - mdelay(1000 / refresh_hz); > - > - /* > - * Completion (signalling the out fence and releasing the vblank > - * reference taken in dm_set_writeback()) is handled by the > shared > - * helper, which is also used by the teardown path. > - */ > - if (amdgpu_dm_crtc_complete_writeback(acrtc)) > + if (acrtc->wb_frame_done) { [Severity: High] If the state was overwritten by a new commit as mentioned above, will this read wb_frame_done == false on the second vblank? If so, it seems it would take the else branch, set wb_frame_done = true again, and completely skip calling amdgpu_dm_crtc_complete_writeback() for the original job, potentially stalling the out-fence permanently and leaking a vblank reference. > + /* > + * Second vblank: the DMA for the captured frame has > + * had a full frame period to flush to memory. Signal > + * the out fence now. > + */ > + amdgpu_dm_crtc_complete_writeback(acrtc); > + } else { > + /* > + * First vblank after arming: the frame has been > + * scanned out and the DMA is finishing. Disable > + * writeback immediately to prevent the hardware from [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
