Re: [PATCH v5 03/10] drm/msm: release scanout framebuffers only after a vblank

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: unpin stall in msm_framebuffer_cleanup() on writeback CRTC planes
- [High] drm/msm: premature fb unpin in msm_atomic_commit_tail() async path
--

commit ee002ddb1edfe27610ec310afec717b4795eee7e
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: release scanout framebuffers only after a vblank

This patch defers unpinning retired scanout framebuffers to a
drm_vblank_work scheduled on the CRTC, preventing the display controller
from fetching from an unmapped IOMMU VMA before the next vblank latches the
new configuration. Pending unpin works are tracked per CRTC and released
directly in msm_crtc_vblank_off() when a CRTC is disabled.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 7b92082d35a64..0e986b533bf07 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -684,7 +684,8 @@ static void dpu_plane_cleanup_fb(struct drm_plane *plane,
>  
>  	DPU_DEBUG_PLANE(pdpu, "FB[%u]\n", old_state->fb->base.id);
>  
> -	msm_framebuffer_cleanup(old_state->fb, old_pstate->needs_dirtyfb);
> +	msm_framebuffer_cleanup(old_state->fb, old_state->crtc,
> +				old_pstate->needs_dirtyfb);
>  }

[Severity: High]
When a CRTC is driven by a standalone writeback encoder, can passing
old_state->crtc here leave the retired source plane framebuffer pinned on
kms->fb_unpin[idx].fbs indefinitely after the writeback completes?

In dpu_crtc_enable(), msm_crtc_vblank_on(crtc) is called for all CRTCs,
which clears vblank->inmodeset and sets kms->fb_unpin[idx].vblank_off =
false.

During a standalone writeback commit in msm_atomic_commit_tail(),
kms->funcs->wait_flush() blocks in dpu_encoder_phys_wb_wait_for_commit_done()
until dpu_encoder_phys_wb_done_irq() runs:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c:dpu_encoder_phys_wb_done_irq()
    dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, event);
    dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
    ...
    wake_up_all(&phys_enc->pending_kickoff_wq);

At that point, the hardware has finished reading the input planes and the
only vblank for the one-shot writeback transfer has already been signaled via
dpu_encoder_vblank_callback() -> dpu_crtc_vblank_callback() ->
drm_crtc_handle_vblank().

Only after wait_flush() returns does msm_atomic_commit_tail() run
drm_atomic_helper_cleanup_planes() -> dpu_plane_cleanup_fb() ->
msm_framebuffer_cleanup(), which schedules msm_fb_unpin_work for
drm_crtc_vblank_count(crtc) + 1.

Because the writeback encoder is one-shot and has no free-running timing
engine (and subsequent commits without a new writeback job skip kickoff in
dpu_crtc_commit_kickoff() when !dpu_encoder_is_valid_for_commit(encoder)),
no further vblank arrives to run the work, leaving the drm_framebuffer
reference, prepare_count pin, kms->vm VMA, and vblank reference held until
another writeback job is submitted or the CRTC is disabled.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index 9a207c1e85837..d836d6dd7f3ec 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c

[ ... ]

> +/*
> + * Publish and schedule the work in one go under the lock, as
> + * msm_crtc_vblank_off() frees whatever it finds on the list.
> + */
> +static bool msm_kms_fb_unpin_schedule(struct msm_kms_fb_unpin *pending,
> +				      struct msm_fb_unpin_work *unpin,
> +				      struct drm_crtc *crtc)
> +{
> +	guard(spinlock)(&pending->lock);
> +
> +	if (pending->vblank_off)
> +		return false;
> +
> +	list_add_tail(&unpin->node, &pending->fbs);
> +
> +	if (drm_vblank_work_schedule(&unpin->base,
> +				     drm_crtc_vblank_count(crtc) + 1, true) == 1)
> +		return true;

[Severity: High]
Could scheduling the unpin for drm_crtc_vblank_count(crtc) + 1 unpin and
unmap the old cursor framebuffer one frame too early on the timer-deferred
async path in msm_atomic_commit_tail()?

When a legacy cursor update changes or disables the cursor framebuffer
(state->legacy_cursor_update = true), DPU1 and MDP4 do not implement
atomic_async_check, and mdp5_plane_atomic_async_check() rejects framebuffer
changes (plane->state->fb != new_plane_state->fb). As a result, the commit
goes through msm_atomic_commit_tail() with async = can_do_async(state,
&async_crtc) == true:

drivers/gpu/drm/msm/msm_atomic.c:msm_atomic_commit_tail()
    if (async) {
        if (!(kms->pending_crtc_mask & crtc_mask)) {
            ...
            if (drm_crtc_next_vblank_start(async_crtc, &vsync_time))
                goto fallback;

            wakeup_time = ktime_sub(vsync_time, ms_to_ktime(1));

            msm_hrtimer_queue_work(&timer->work, wakeup_time,
                    HRTIMER_MODE_ABS);
        }

        kms->funcs->disable_commit(kms);
        unlock_crtcs(kms, crtc_mask);
        drm_atomic_helper_commit_hw_done(state);
        drm_atomic_helper_cleanup_planes(dev, state);
        ...
        return;
    }

In this branch, kms->funcs->flush_commit() has not been called yet; writing
CTL_FLUSH is deferred to timer->work (msm_atomic_pending_work ->
msm_atomic_async_commit), while drm_atomic_helper_cleanup_planes() runs
immediately and schedules msm_kms_fb_unpin_work for
drm_crtc_vblank_count(crtc) + 1.

If msm_atomic_commit_tail() runs during the Vertical Front Porch (line >
vactive_end), dpu_crtc_get_scanout_position() returns a negative vpos:

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_get_scanout_position()
    if (line < vactive_start)
        line -= vactive_start;
    else if (line > vactive_end)
        line = line - vfp_end - vactive_start;
    else
        line -= vactive_start;

This causes drm_crtc_next_vblank_start() to return vsync_time at the end of
frame N + 1 (before vblank N + 2), while drm_crtc_vblank_count(crtc) is
still N until the VSYNC interrupt fires at line = 1. Similarly, if the
cursor update arrives within 1ms of vsync_time or timer->work runs after
programmable fetch start / vblank N + 1, flush_commit() will not latch
until vblank N + 2, but msm_kms_fb_unpin_work will run at vblank N + 1 and
detach the VMA while the hardware is still scanning out the old framebuffer
during frame N + 1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-fd-kms-fix-smmu-v5-0-a5b0b284aead@oss.qualcomm.com?part=3