| Message ID | 20200808120900.pudwwrfz44g3rqx7@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/vkms: guarantee vblank when capturing crc | expand |
On Sat, Aug 08, 2020 at 09:09:00AM -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 subtests: 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. > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > > Co-debugged-by: Sidong Yang <realwakka@gmail.com> > Signed-off-by: Sidong Yang <realwakka@gmail.com> > Signed-off-by: Melissa Wen <melissa.srw@gmail.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > v2: > - extract a vkms_set_composer helper > - fix vblank refcounting for the disabling case > > v3: > - make the vkms_set_composer helper static > - review the credit tags Thanks a lot for your work here, patch merged to drm-misc-next. I've kept the changelog in the commit message, I often find that quite useful. But that's a bit a drm peculiarity, most other subsystems prefer it like you've done it. Cheers, Daniel > > --- > drivers/gpu/drm/vkms/vkms_composer.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index b8b060354667..4f3b07a32b60 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; > } > > +static 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; > } > -- > 2.27.0 >
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index b8b060354667..4f3b07a32b60 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; } +static 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; }