| Message ID | 20200806111705.xebopzucxr3367z4@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/vkms: guarantee vblank when capturing crc | expand |
On Thu, Aug 06, 2020 at 08:17:05AM -0300, Melissa Wen wrote: > VKMS needs vblank interrupts enabled to capture CRC. When vblank is > disabled, tests like kms_cursor_crc and kms_pipe_crc_basic getting stuck > waiting for a capture that will not occur until vkms wakes up. This patch > adds a helper to set composer and ensure that vblank remains enabled as > long as the CRC capture is needed. > > It clears the execution of the following kms_cursor_crc subtests: > 1. pipe-A-cursor-[size,alpha-opaque, NxN-(on-screen, off-screen, sliding, > random, fast-moving])] - successful when running individually. > 2. pipe-A-cursor-dpms passes again > 3. pipe-A-cursor-suspend also passes > > The issue was initially tracked in the sequential execution of IGT > kms_cursor_crc subtest: when running the test sequence or one of its > subtests twice, the odd execs complete and the pairs get stuck in an > endless wait. In the IGT code, calling a wait_for_vblank on preparing for > CRC capture prevented the busy-wait. But the problem persisted in the > pipe-A-cursor-dpms and -suspend subtests. > > Checking the history, the pipe-A-cursor-dpms subtest was successful when, > in vkms_atomic_commit_tail, instead of using the flip_done op, it used > wait_for_vblanks. Another way to prevent blocking was wait_one_vblank when > enabling crtc. However, in both cases, pipe-A-cursor-suspend persisted > blocking in the 2nd start of CRC capture, which may indicate that > something got stuck in the step of CRC setup. Indeed, wait_one_vblank in > the crc setup was able to sync things and free all kms_cursor_crc > subtests. Besides, other alternatives to force enabling vblanks or prevent > disabling them such as calling drm_crtc_put_vblank or modeset_enables > before commit_planes + offdelay = 0, also unlock all subtests executions. > > Finally, due to vkms's dependence on vblank interruptions to perform > tasks, this patch uses refcount to ensure that vblanks happen when > enabling composer and while crc capture is needed. For next time around, please include a patch changelog here so people know what was changed, and why. E.g. v2: - extract a vkms_set_composer helper - fix vblank refcounting for the disabling case > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > Co-developed-by: Sidong Yang <realwakka@gmail.com> > Signed-off-by: Sidong Yang <realwakka@gmail.com> > Co-developed-by: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Daniel Vetter <daniel@ffwll.ch> Nah that's a bit too much credit for me, I'll get the r-b tag. You & Sidong figured this story out, so I'd use Co-debugged-by: Sidong here for credits. > Signed-off-by: Melissa Wen <melissa.srw@gmail.com> > --- > drivers/gpu/drm/vkms/vkms_composer.c | 20 +++++++++++++++++--- > drivers/gpu/drm/vkms/vkms_drv.h | 1 + > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index b8b060354667..e2ac2b9759bf 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -233,6 +233,22 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name, > return 0; > } > > +void vkms_set_composer(struct vkms_output *out, bool enabled) Since this isn't yet used outside of this file I'd just make this static. Writeback connector support can then add the prototype in the header. With that Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Please include that tag for revision 3. Cheers, Daniel > +{ > + bool old_enabled; > + > + if (enabled) > + drm_crtc_vblank_get(&out->crtc); > + > + spin_lock_irq(&out->lock); > + old_enabled = out->composer_enabled; > + out->composer_enabled = enabled; > + spin_unlock_irq(&out->lock); > + > + if (old_enabled) > + drm_crtc_vblank_put(&out->crtc); > +} > + > int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) > { > struct vkms_output *out = drm_crtc_to_vkms_output(crtc); > @@ -241,9 +257,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) > > ret = vkms_crc_parse_source(src_name, &enabled); > > - spin_lock_irq(&out->lock); > - out->composer_enabled = enabled; > - spin_unlock_irq(&out->lock); > + vkms_set_composer(out, enabled); > > return ret; > } > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index f4036bb0b9a8..2cc86d08bd4e 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -142,6 +142,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name, > size_t *values_cnt); > > /* Composer Support */ > +void vkms_set_composer(struct vkms_output *output, bool enabled); > void vkms_composer_worker(struct work_struct *work); > > #endif /* _VKMS_DRV_H_ */ > -- > 2.27.0 >
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index b8b060354667..e2ac2b9759bf 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -233,6 +233,22 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name, return 0; } +void vkms_set_composer(struct vkms_output *out, bool enabled) +{ + bool old_enabled; + + if (enabled) + drm_crtc_vblank_get(&out->crtc); + + spin_lock_irq(&out->lock); + old_enabled = out->composer_enabled; + out->composer_enabled = enabled; + spin_unlock_irq(&out->lock); + + if (old_enabled) + drm_crtc_vblank_put(&out->crtc); +} + int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) { struct vkms_output *out = drm_crtc_to_vkms_output(crtc); @@ -241,9 +257,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) ret = vkms_crc_parse_source(src_name, &enabled); - spin_lock_irq(&out->lock); - out->composer_enabled = enabled; - spin_unlock_irq(&out->lock); + vkms_set_composer(out, enabled); return ret; } diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index f4036bb0b9a8..2cc86d08bd4e 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -142,6 +142,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name, size_t *values_cnt); /* Composer Support */ +void vkms_set_composer(struct vkms_output *output, bool enabled); void vkms_composer_worker(struct work_struct *work); #endif /* _VKMS_DRV_H_ */