[PATCH] drm/vkms: guarantee vblank when capturing crc

[Melissa Wen]

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 <rodrigosi...@gmail.com>
Cc: Haneen Mohammed <hamoha...@gmail.com>
Co-developed-by: Sidong Yang <real...@gmail.com>
Signed-off-by: Sidong Yang <real...@gmail.com>
Co-developed-by: Daniel Vetter <dan...@ffwll.ch>
Signed-off-by: Daniel Vetter <dan...@ffwll.ch>
Signed-off-by: Melissa Wen <melis...@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);
--
2.27.0

[dan...@ffwll.ch]

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
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Melissa Wen]

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.

Cc: Rodrigo Siqueira <rodrigosi...@gmail.com>
Cc: Haneen Mohammed <hamoha...@gmail.com>
Co-developed-by: Sidong Yang <real...@gmail.com>
Signed-off-by: Sidong Yang <real...@gmail.com>
Co-developed-by: Daniel Vetter <dan...@ffwll.ch>
Signed-off-by: Daniel Vetter <dan...@ffwll.ch>
Signed-off-by: Melissa Wen <melis...@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)
+{
+ 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

[Melissa Wen]

Hi Daniel,

Thanks for the feedback and advice.
I applied the suggestion and just sent a v2.

Best regards,

Melissa

[Daniel Vetter]

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 <rodrigosi...@gmail.com>
> Cc: Haneen Mohammed <hamoha...@gmail.com>
> Co-developed-by: Sidong Yang <real...@gmail.com>
> Signed-off-by: Sidong Yang <real...@gmail.com>
> Co-developed-by: Daniel Vetter <dan...@ffwll.ch>
> Signed-off-by: Daniel Vetter <dan...@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 <melis...@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...@ffwll.ch>

Please include that tag for revision 3.

Cheers, Daniel

[Melissa Wen]

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 <rodrigosi...@gmail.com>
Cc: Haneen Mohammed <hamoha...@gmail.com>

Co-debugged-by: Sidong Yang <real...@gmail.com>
Signed-off-by: Sidong Yang <real...@gmail.com>
Signed-off-by: Melissa Wen <melis...@gmail.com>
Reviewed-by: Daniel Vetter <daniel...@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

---
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

[Daniel Vetter]

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