Messages in this thread | | | Date | Fri, 7 Aug 2020 11:10:44 +0200 | From | Daniel Vetter <> | Subject | Re: [PATCH v2] drm/vkms: guarantee vblank when capturing crc |
| |
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 >
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
| |