On Sun, Jul 30, 2023 at 12:51 AM Maíra Canal <mairacanal@xxxxxxxxxx> wrote:
>
> After we flush the workqueue at the commit tale, we need to make sure
> that no work is queued until we destroy the state. Currently, new work
> can be queued in the workqueue, even after the commit tale, as the
> vblank thread is still running.
>
> Therefore, to avoid a race-condition that will lead to the trigger of a
> WARN_ON() at the function vkms_atomic_crtc_destroy_state(), add a mutex
> to protect the sections where the queue is manipulated.
>
> This way we can make sure that no work will be added to the workqueue
> between flushing the queue (at the commit tail) and destroying the
> state.
>
> Signed-off-by: Maíra Canal <mairacanal@xxxxxxxxxx>
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 10 +++++++++-
> drivers/gpu/drm/vkms/vkms_drv.c | 1 +
> drivers/gpu/drm/vkms/vkms_drv.h | 8 ++++++++
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 3c5ebf106b66..e5ec21a0da05 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -49,7 +49,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> state->crc_pending = true;
> spin_unlock(&output->composer_lock);
>
> + mutex_lock(&state->queue_lock);
> ret = queue_work(output->composer_workq, &state->composer_work);
> + mutex_unlock(&state->queue_lock);
> if (!ret)
> DRM_DEBUG_DRIVER("Composer worker already queued\n");
> }
> @@ -129,6 +131,7 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
>
> __drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
>
> + mutex_init(&vkms_state->queue_lock);
> INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
>
> return &vkms_state->base;
> @@ -142,6 +145,9 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
> __drm_atomic_helper_crtc_destroy_state(state);
>
> WARN_ON(work_pending(&vkms_state->composer_work));
> + mutex_unlock(&vkms_state->queue_lock);
> +
> + mutex_destroy(&vkms_state->queue_lock);
> kfree(vkms_state->active_planes);
> kfree(vkms_state);
> }
> @@ -155,8 +161,10 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
> vkms_atomic_crtc_destroy_state(crtc, crtc->state);
>
> __drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
> - if (vkms_state)
> + if (vkms_state) {
> + mutex_init(&vkms_state->queue_lock);
> INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
> + }
> }
>
> static const struct drm_crtc_funcs vkms_crtc_funcs = {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index dd0af086e7fa..9212686ca88a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -84,6 +84,7 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> struct vkms_crtc_state *vkms_state =
> to_vkms_crtc_state(old_crtc_state);
>
> + mutex_lock(&vkms_state->queue_lock);
I haven't looked at the code in detail but doesn't this need to be
unlocked after flush_work again?
> flush_work(&vkms_state->composer_work);
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c7ae6c2ba1df..83767692469a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -89,6 +89,14 @@ struct vkms_crtc_state {
> struct vkms_writeback_job *active_writeback;
> struct vkms_color_lut gamma_lut;
>
> + /* protects the access to the workqueue
> + *
> + * we need to hold this lock between flushing the workqueue and
> + * destroying the state to avoid work to be queued by the worker
> + * thread
> + */
> + struct mutex queue_lock;
> +
> /* below four are protected by vkms_output.composer_lock */
> bool crc_pending;
> bool wb_pending;
> --
> 2.41.0
>
