Hi Louis,
On 05/11/24 10:59, Louis Chauvet wrote:
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.
Yeah, I think it needs improvement indeed. Usually, unbalanced mutexes
aren't a good idea.
Do you want/have time to continue this fix?
I don't plan to keep working in the patch. Feel free to pick the idea
and implement a proper fix.
Best Regards,
- Maíra
[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
