| Message ID | 20230523123207.173976-1-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/vkms: Fix race-condition between the hrtimer and the atomic commit | expand |
On 23/05/23 09:32, Maíra Canal wrote: > Currently, it is possible for the composer to be set as enabled and then > as disabled without a proper call for the vkms_vblank_simulate(). This > is problematic, because the driver would skip one CRC output, causing CRC > tests to fail. Therefore, we need to make sure that, for each time the > composer is set as enabled, a composer job is added to the queue. > > In order to provide this guarantee, add a mutex that will lock before > the composer is set as enabled and will unlock only after the composer > job is added to the queue. This way, we can have a guarantee that the > driver won't skip a CRC entry. > > This race-condition is affecting the IGT test "writeback-check-output", > making the test fail and also, leaking writeback framebuffers, as the > writeback job is queued, but it is not signaled. This patch avoids both > problems. > > [v2]: > * Create a new mutex and keep the spinlock across the atomic commit in > order to avoid interrupts that could result in deadlocks. > > Signed-off-by: Maíra Canal <mcanal@igalia.com> Great catch! Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net> > --- > drivers/gpu/drm/vkms/vkms_composer.c | 9 +++++++-- > drivers/gpu/drm/vkms/vkms_crtc.c | 9 +++++---- > drivers/gpu/drm/vkms/vkms_drv.h | 4 +++- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index 906d3df40cdb..b12188fd6b38 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -320,10 +320,15 @@ void vkms_set_composer(struct vkms_output *out, bool enabled) > if (enabled) > drm_crtc_vblank_get(&out->crtc); > > - spin_lock_irq(&out->lock); > + mutex_lock(&out->enabled_lock); > old_enabled = out->composer_enabled; > out->composer_enabled = enabled; > - spin_unlock_irq(&out->lock); > + > + /* the composition wasn't enabled, so unlock the lock to make sure the lock > + * will be balanced even if we have a failed commit > + */ > + if (!out->composer_enabled) > + mutex_unlock(&out->enabled_lock); > > if (old_enabled) > drm_crtc_vblank_put(&out->crtc); > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > index 515f6772b866..3079013c8b32 100644 > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > @@ -16,7 +16,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > struct drm_crtc *crtc = &output->crtc; > struct vkms_crtc_state *state; > u64 ret_overrun; > - bool ret, fence_cookie; > + bool ret, fence_cookie, composer_enabled; > > fence_cookie = dma_fence_begin_signalling(); > > @@ -25,15 +25,15 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > if (ret_overrun != 1) > pr_warn("%s: vblank timer overrun\n", __func__); > > - spin_lock(&output->lock); > ret = drm_crtc_handle_vblank(crtc); > if (!ret) > DRM_ERROR("vkms failure on handling vblank"); > > state = output->composer_state; > - spin_unlock(&output->lock); > + composer_enabled = output->composer_enabled; > + mutex_unlock(&output->enabled_lock); > > - if (state && output->composer_enabled) { > + if (state && composer_enabled) { > u64 frame = drm_crtc_accurate_vblank_count(crtc); > > /* update frame_start only if a queued vkms_composer_worker() > @@ -292,6 +292,7 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > > spin_lock_init(&vkms_out->lock); > spin_lock_init(&vkms_out->composer_lock); > + mutex_init(&vkms_out->enabled_lock); > > vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0); > if (!vkms_out->composer_workq) > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 5f1a0a44a78c..dcf4e302fb4d 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -100,8 +100,10 @@ struct vkms_output { > struct workqueue_struct *composer_workq; > /* protects concurrent access to composer */ > spinlock_t lock; > + /* guarantees that if the composer is enabled, a job will be queued */ > + struct mutex enabled_lock; > > - /* protected by @lock */ > + /* protected by @enabled_lock */ > bool composer_enabled; > struct vkms_crtc_state *composer_state; >
On 5/23/23 09:32, Maíra Canal wrote: > Currently, it is possible for the composer to be set as enabled and then > as disabled without a proper call for the vkms_vblank_simulate(). This > is problematic, because the driver would skip one CRC output, causing CRC > tests to fail. Therefore, we need to make sure that, for each time the > composer is set as enabled, a composer job is added to the queue. > > In order to provide this guarantee, add a mutex that will lock before > the composer is set as enabled and will unlock only after the composer > job is added to the queue. This way, we can have a guarantee that the > driver won't skip a CRC entry. > > This race-condition is affecting the IGT test "writeback-check-output", > making the test fail and also, leaking writeback framebuffers, as the > writeback job is queued, but it is not signaled. This patch avoids both > problems. > > [v2]: > * Create a new mutex and keep the spinlock across the atomic commit in > order to avoid interrupts that could result in deadlocks. > > Signed-off-by: Maíra Canal <mcanal@igalia.com> Applied to drm-misc/drm-misc-next. Best Regards, - Maíra > --- > drivers/gpu/drm/vkms/vkms_composer.c | 9 +++++++-- > drivers/gpu/drm/vkms/vkms_crtc.c | 9 +++++---- > drivers/gpu/drm/vkms/vkms_drv.h | 4 +++- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index 906d3df40cdb..b12188fd6b38 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -320,10 +320,15 @@ void vkms_set_composer(struct vkms_output *out, bool enabled) > if (enabled) > drm_crtc_vblank_get(&out->crtc); > > - spin_lock_irq(&out->lock); > + mutex_lock(&out->enabled_lock); > old_enabled = out->composer_enabled; > out->composer_enabled = enabled; > - spin_unlock_irq(&out->lock); > + > + /* the composition wasn't enabled, so unlock the lock to make sure the lock > + * will be balanced even if we have a failed commit > + */ > + if (!out->composer_enabled) > + mutex_unlock(&out->enabled_lock); > > if (old_enabled) > drm_crtc_vblank_put(&out->crtc); > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > index 515f6772b866..3079013c8b32 100644 > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > @@ -16,7 +16,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > struct drm_crtc *crtc = &output->crtc; > struct vkms_crtc_state *state; > u64 ret_overrun; > - bool ret, fence_cookie; > + bool ret, fence_cookie, composer_enabled; > > fence_cookie = dma_fence_begin_signalling(); > > @@ -25,15 +25,15 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > if (ret_overrun != 1) > pr_warn("%s: vblank timer overrun\n", __func__); > > - spin_lock(&output->lock); > ret = drm_crtc_handle_vblank(crtc); > if (!ret) > DRM_ERROR("vkms failure on handling vblank"); > > state = output->composer_state; > - spin_unlock(&output->lock); > + composer_enabled = output->composer_enabled; > + mutex_unlock(&output->enabled_lock); > > - if (state && output->composer_enabled) { > + if (state && composer_enabled) { > u64 frame = drm_crtc_accurate_vblank_count(crtc); > > /* update frame_start only if a queued vkms_composer_worker() > @@ -292,6 +292,7 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > > spin_lock_init(&vkms_out->lock); > spin_lock_init(&vkms_out->composer_lock); > + mutex_init(&vkms_out->enabled_lock); > > vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0); > if (!vkms_out->composer_workq) > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 5f1a0a44a78c..dcf4e302fb4d 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -100,8 +100,10 @@ struct vkms_output { > struct workqueue_struct *composer_workq; > /* protects concurrent access to composer */ > spinlock_t lock; > + /* guarantees that if the composer is enabled, a job will be queued */ > + struct mutex enabled_lock; > > - /* protected by @lock */ > + /* protected by @enabled_lock */ > bool composer_enabled; > struct vkms_crtc_state *composer_state; >
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 906d3df40cdb..b12188fd6b38 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -320,10 +320,15 @@ void vkms_set_composer(struct vkms_output *out, bool enabled) if (enabled) drm_crtc_vblank_get(&out->crtc); - spin_lock_irq(&out->lock); + mutex_lock(&out->enabled_lock); old_enabled = out->composer_enabled; out->composer_enabled = enabled; - spin_unlock_irq(&out->lock); + + /* the composition wasn't enabled, so unlock the lock to make sure the lock + * will be balanced even if we have a failed commit + */ + if (!out->composer_enabled) + mutex_unlock(&out->enabled_lock); if (old_enabled) drm_crtc_vblank_put(&out->crtc); diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 515f6772b866..3079013c8b32 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -16,7 +16,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) struct drm_crtc *crtc = &output->crtc; struct vkms_crtc_state *state; u64 ret_overrun; - bool ret, fence_cookie; + bool ret, fence_cookie, composer_enabled; fence_cookie = dma_fence_begin_signalling(); @@ -25,15 +25,15 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) if (ret_overrun != 1) pr_warn("%s: vblank timer overrun\n", __func__); - spin_lock(&output->lock); ret = drm_crtc_handle_vblank(crtc); if (!ret) DRM_ERROR("vkms failure on handling vblank"); state = output->composer_state; - spin_unlock(&output->lock); + composer_enabled = output->composer_enabled; + mutex_unlock(&output->enabled_lock); - if (state && output->composer_enabled) { + if (state && composer_enabled) { u64 frame = drm_crtc_accurate_vblank_count(crtc); /* update frame_start only if a queued vkms_composer_worker() @@ -292,6 +292,7 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, spin_lock_init(&vkms_out->lock); spin_lock_init(&vkms_out->composer_lock); + mutex_init(&vkms_out->enabled_lock); vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0); if (!vkms_out->composer_workq) diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 5f1a0a44a78c..dcf4e302fb4d 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -100,8 +100,10 @@ struct vkms_output { struct workqueue_struct *composer_workq; /* protects concurrent access to composer */ spinlock_t lock; + /* guarantees that if the composer is enabled, a job will be queued */ + struct mutex enabled_lock; - /* protected by @lock */ + /* protected by @enabled_lock */ bool composer_enabled; struct vkms_crtc_state *composer_state;
Currently, it is possible for the composer to be set as enabled and then as disabled without a proper call for the vkms_vblank_simulate(). This is problematic, because the driver would skip one CRC output, causing CRC tests to fail. Therefore, we need to make sure that, for each time the composer is set as enabled, a composer job is added to the queue. In order to provide this guarantee, add a mutex that will lock before the composer is set as enabled and will unlock only after the composer job is added to the queue. This way, we can have a guarantee that the driver won't skip a CRC entry. This race-condition is affecting the IGT test "writeback-check-output", making the test fail and also, leaking writeback framebuffers, as the writeback job is queued, but it is not signaled. This patch avoids both problems. [v2]: * Create a new mutex and keep the spinlock across the atomic commit in order to avoid interrupts that could result in deadlocks. Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/vkms/vkms_composer.c | 9 +++++++-- drivers/gpu/drm/vkms/vkms_crtc.c | 9 +++++---- drivers/gpu/drm/vkms/vkms_drv.h | 4 +++- 3 files changed, 15 insertions(+), 7 deletions(-)