On 29/07/23 - 19:49, Maíra Canal 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> Hi Maìra, Thanks for pointing to this patch, it does not apply on drm-misc-next, but it was simple to manually rebase (see [0]). I think it should solve the issue, and the CI seems to agree. But it seems to be imperfect, as it introduce a warning on mutex unlock imbalance [1] (not always reproducable). It is better than a kernel crash already. Do you want/have time to continue this fix? [0]:https://gitlab.freedesktop.org/louischauvet/kernel/-/commit/017210f48c809730296d1f562e615b666fdbcfdc [1]:https://gitlab.freedesktop.org/louischauvet/kernel/-/jobs/66118565/viewer#L803 Thanks, Louis Chauvet > --- > 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); > 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 >
