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 ensures 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. These facts together converge on the lack of vblank to unblock the crc capture.
Finally, considering the vkms's dependence on vblank interruptions to perform tasks, this patch acquires a vblank ref when setup CRC source and releases ref when disabling crc capture, ensuring vblanks happen to compute CRC.
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 Signed-off-by: Melissa Wen melissa.srw@gmail.com --- drivers/gpu/drm/vkms/vkms_composer.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 4af2f19480f4..1161eaa383f1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -241,6 +241,14 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
ret = vkms_crc_parse_source(src_name, &enabled);
+ /* Ensure that vblank interruptions are enabled for crc capture */ + /* Enabling CRC: acquire vblank ref */ + if (enabled) + drm_crtc_vblank_get(crtc); + /* Disabling CRC: release vblank ref */ + if (!src_name) + drm_crtc_vblank_put(crtc); + spin_lock_irq(&out->lock); out->composer_enabled = enabled; spin_unlock_irq(&out->lock);
On Sat, Aug 01, 2020 at 03:49:29PM -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 ensures that vblank remains enabled as long as the CRC capture is needed.
It clears the execution of the following kms_cursor_crc subtests:
- 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. These facts together converge on the lack of vblank to unblock the crc capture.
Finally, considering the vkms's dependence on vblank interruptions to perform tasks, this patch acquires a vblank ref when setup CRC source and releases ref when disabling crc capture, ensuring vblanks happen to compute CRC.
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 Signed-off-by: Melissa Wen melissa.srw@gmail.com
Excellent summary of the debug story.
drivers/gpu/drm/vkms/vkms_composer.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 4af2f19480f4..1161eaa383f1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -241,6 +241,14 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
ret = vkms_crc_parse_source(src_name, &enabled);
- /* Ensure that vblank interruptions are enabled for crc capture */
- /* Enabling CRC: acquire vblank ref */
This comment just explains what the code does, that's not needed. The first comment I think can be replaced if we create a little helper function like
void vkms_set_composer(struct vkms_output, bool enable) { bool old_state;
if (enabled) drm_crtc_vblank_get(crtc);
spin_lock_irq(&out->lock); old_enable = out->composer_enabled; out->composer_enabled = enabled; spin_unlock_irq(&out->lock);
if (old_enabled) drm_crtc_vblank_put(crtc);
}
This should also help as prep for the writeback work, where we have a 2nd user that might need to enable the composer (maybe even need to switch to refcounting the composer state then).
- if (enabled)
drm_crtc_vblank_get(crtc);- /* Disabling CRC: release vblank ref */
- if (!src_name)
drm_crtc_vblank_put(crtc);
I'm not sure this correctly releases the vblank reference in all cases, I think the suggestion in the helper function pseudo code should work better. It does mean we temporarily elevate the vblank refcount if we go enabled -> enabled, but that's not a problem since it's refcounted.
Cheers, Daniel
- spin_lock_irq(&out->lock); out->composer_enabled = enabled; spin_unlock_irq(&out->lock);
-- 2.27.0
On 08/04, daniel@ffwll.ch wrote:
On Sat, Aug 01, 2020 at 03:49:29PM -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 ensures that vblank remains enabled as long as the CRC capture is needed.
It clears the execution of the following kms_cursor_crc subtests:
- 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. These facts together converge on the lack of vblank to unblock the crc capture.
Finally, considering the vkms's dependence on vblank interruptions to perform tasks, this patch acquires a vblank ref when setup CRC source and releases ref when disabling crc capture, ensuring vblanks happen to compute CRC.
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 Signed-off-by: Melissa Wen melissa.srw@gmail.com
Excellent summary of the debug story.
drivers/gpu/drm/vkms/vkms_composer.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 4af2f19480f4..1161eaa383f1 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -241,6 +241,14 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
ret = vkms_crc_parse_source(src_name, &enabled);
- /* Ensure that vblank interruptions are enabled for crc capture */
- /* Enabling CRC: acquire vblank ref */
This comment just explains what the code does, that's not needed. The first comment I think can be replaced if we create a little helper function like
void vkms_set_composer(struct vkms_output, bool enable) { bool old_state;
if (enabled) drm_crtc_vblank_get(crtc);
spin_lock_irq(&out->lock); old_enable = out->composer_enabled; out->composer_enabled = enabled; spin_unlock_irq(&out->lock);
if (old_enabled) drm_crtc_vblank_put(crtc);
}
This should also help as prep for the writeback work, where we have a 2nd user that might need to enable the composer (maybe even need to switch to refcounting the composer state then).
Hi Daniel,
Thanks for the feedback and advice. I applied the suggestion and just sent a v2.
Best regards,
Melissa
- if (enabled)
drm_crtc_vblank_get(crtc);- /* Disabling CRC: release vblank ref */
- if (!src_name)
drm_crtc_vblank_put(crtc);I'm not sure this correctly releases the vblank reference in all cases, I think the suggestion in the helper function pseudo code should work better. It does mean we temporarily elevate the vblank refcount if we go enabled -> enabled, but that's not a problem since it's refcounted.
Cheers, Daniel
- spin_lock_irq(&out->lock); out->composer_enabled = enabled; spin_unlock_irq(&out->lock);
-- 2.27.0
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
daniel＠ffwll.ch -
Melissa Wen