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

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

[Sidong Yang]

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.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
 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

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@gmail.com> 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@ffwll.ch>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
>
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
>  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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20200729152231.13249-1-realwakka%40gmail.com.

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

[daniel]

On Sat, Aug 01, 2020 at 04:30:23PM -0300, Melissa Wen wrote:
> On Wed, Jul 29, 2020 at 12:22 PM Sidong Yang <realwakka@gmail.com> 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.

Yeah I think (but haven't tested, definitely need to confirm that) that
the vblank get/put fix from Melissa is the correct fix for all these
issues.

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

Yeah I think splitting out the warning fix is the right thing to do here.
-Daniel

> 
> 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@ffwll.ch>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> >
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >  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@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20200729152231.13249-1-realwakka%40gmail.com.

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

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

[Sidong Yang]

On Tue, Aug 04, 2020 at 11:33:51AM +0200, daniel@ffwll.ch wrote:
> On Sat, Aug 01, 2020 at 04:30:23PM -0300, Melissa Wen wrote:
> > On Wed, Jul 29, 2020 at 12:22 PM Sidong Yang <realwakka@gmail.com> 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.
> 
> Yeah I think (but haven't tested, definitely need to confirm that) that
> the vblank get/put fix from Melissa is the correct fix for all these
> issues.
> 
> > 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.
> 
> Yeah I think splitting out the warning fix is the right thing to do here.

Okay, I'll write another patch about the warning.
Thanks.

-Sidong

> -Daniel
> 
> > 
> > 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@ffwll.ch>
> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > >
> > > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > > ---
> > >  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@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20200729152231.13249-1-realwakka%40gmail.com.
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch