On Tue, Aug 04, 2020 at 11:33:51AM +0200, dan...@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 <realwa...@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 <dan...@ffwll.ch>
> > > Cc: Rodrigo Siqueira <rodrigosiqueiram...@gmail.com>
> > > Cc: Haneen Mohammed <hamohammed...@gmail.com>
> > >
> > > Signed-off-by: Sidong Yang <realwa...@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+unsubscr...@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
