linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail
@ 2020-07-29 15:22 Sidong Yang
  2020-08-01 19:30 ` Melissa Wen
  0 siblings, 1 reply; 4+ messages in thread
From: Sidong Yang @ 2020-07-29 15:22 UTC (permalink / raw)
  To: Rodrigo Siqueira, Haneen Mohammed, Daniel Vetter, David Airlie
  Cc: Sidong Yang, linux-kernel, dri-devel, kernel-usp

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail
  2020-07-29 15:22 [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail Sidong Yang
@ 2020-08-01 19:30 ` Melissa Wen
  2020-08-04  9:33   ` daniel
  0 siblings, 1 reply; 4+ messages in thread
From: Melissa Wen @ 2020-08-01 19:30 UTC (permalink / raw)
  To: Sidong Yang
  Cc: Rodrigo Siqueira, Haneen Mohammed, Daniel Vetter, David Airlie,
	LKML, DRI Development

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail
  2020-08-01 19:30 ` Melissa Wen
@ 2020-08-04  9:33   ` daniel
  2020-08-07 21:17     ` Sidong Yang
  0 siblings, 1 reply; 4+ messages in thread
From: daniel @ 2020-08-04  9:33 UTC (permalink / raw)
  Cc: Sidong Yang, Rodrigo Siqueira, Haneen Mohammed, Daniel Vetter,
	David Airlie, LKML, DRI Development

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail
  2020-08-04  9:33   ` daniel
@ 2020-08-07 21:17     ` Sidong Yang
  0 siblings, 0 replies; 4+ messages in thread
From: Sidong Yang @ 2020-08-07 21:17 UTC (permalink / raw)
  To: Melissa Wen, Rodrigo Siqueira, Haneen Mohammed, David Airlie,
	LKML, DRI Development
  Cc: Rodrigo Siqueira, Haneen Mohammed, Daniel Vetter, David Airlie,
	LKML, DRI Development

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-08-07 21:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 15:22 [PATCH] drm/vkms: modify sequence disable/plane/enable in commit_tail Sidong Yang
2020-08-01 19:30 ` Melissa Wen
2020-08-04  9:33   ` daniel
2020-08-07 21:17     ` Sidong Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).