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

[daniel@ffwll.ch]

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