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@xxxxxxxx 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@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.
> 
> 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@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@xxxxxxxxxxxxxxxx.
> > > 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