Re: [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail

[Melissa Wen]

On Wed, Jul 29, 2020 at 12:22 PM Sidong Yang <realwakka@xxxxxxxxx> wrote:
>
> This patch modifies function call sequence in commit tail. This is for
> the problem that raised when kms_cursor_crc test is tested repeatedly.
> In second test, there is an bug that crtc commit doesn't start vblank events.
> Because there is some error about vblank's refcount. in commit_flush() that
> called from commit_plane, drm_vblank_get() is called and vblank is enabled
> in normal case. But in second test, vblank isn't enable for vblank->refcount
> is already increased in previous test. Increased refcount will be decreased
> in drm_atomic_helper_commit_modeset_enables() after commit_plane.
> Therefore, commit_plane should be called after commit_modeset_enable.
>
> In this situation, there is a warning raised in get_vblank_timestamp().
> hrtimer.node.expires and vblank->time are zero for no vblank events before.
> This patch returns current time when vblank is not enabled.
>
Hi Sidong,

I think this patch tries to solve two different issues.

I am not a maintainer, but I believe you can split it.

Everything indicates that changing the commit tail sequence does not
ideally solve the problem of subtests getting stuck (as we have dicussed);
however, for me, the treatment of the warning is valid and it is also related
to other IGT tests using VKMS.

One option is to send a patch that only treats the warning. I believe that
in the body of the commit message, it would be nice to have the warning
that this patch addresses, and when it appears by running an IGT test.
Also, say why it should be done this way in vkms.
This info could help future debugging.

Off-topic: I removed the group's mailing list of the University of SÃo
Paulo (kernel-usp) from the cc, since I believe you had no intention of
sending the patch to them.

Best regards,

Melissa

> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> Cc: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
>
> Signed-off-by: Sidong Yang <realwakka@xxxxxxxxx>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 5 +++++
>  drivers/gpu/drm/vkms/vkms_drv.c  | 4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index ac85e17428f8..09c012d54d58 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -86,6 +86,11 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
>         struct vkms_output *output = &vkmsdev->output;
>         struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> +       if (!READ_ONCE(vblank->enabled)) {
> +               *vblank_time = ktime_get();
> +               return true;
> +       }
> +
>         *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
>
>         if (WARN_ON(*vblank_time == vblank->time))
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..c2c83a01d4a7 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -76,10 +76,10 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>
>         drm_atomic_helper_commit_modeset_disables(dev, old_state);
>
> -       drm_atomic_helper_commit_planes(dev, old_state, 0);
> -
>         drm_atomic_helper_commit_modeset_enables(dev, old_state);
>
> +       drm_atomic_helper_commit_planes(dev, old_state, 0);
> +
>         drm_atomic_helper_fake_vblank(old_state);
>
>         drm_atomic_helper_commit_hw_done(old_state);
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@xxxxxxxxxxxxxxxxx
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20200729152231.13249-1-realwakka%40gmail.com.