Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Jul 2020 14:05:33 -0300
From:   Melissa Wen <melissa.srw@gmail.com>
To:     Daniel Vetter <daniel@ffwll.ch>
Cc:     Sidong Yang <realwakka@gmail.com>,
        Haneen Mohammed <hamohammed.sa@gmail.com>,
        Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Emil Velikov <emil.l.velikov@gmail.com>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail
Message-ID: <20200710170533.xn2ar2cpekom6i2n@smtp.gmail.com>
References: <20200701153134.4187-1-realwakka@gmail.com>
 <20200702131355.GX3278063@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200702131355.GX3278063@phenom.ffwll.local>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 07/02, Daniel Vetter wrote:
> On Wed, Jul 01, 2020 at 03:31:34PM +0000, Sidong Yang wrote:
> > there is an error when igt test is run continuously. vkms_atomic_commit_tail()
> > need to call drm_atomic_helper_wait_for_vblanks() for give up ownership of
> > vblank events. without this code, next atomic commit will not enable vblank
> > and raise timeout error.
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 1e8b2169d834..10b9be67a068 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -93,6 +93,8 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  		flush_work(&vkms_state->composer_work);
> >  	}
> >  
> > +	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> 
> Uh, we have a wait_for_flip_done right above, which should be doing
> exactly the same, but more precisely: Instead of just waiting for any
> vblank to happen, we wait for exactly the vblank corresponding to this
> atomic commit. So no races possible. If this is papering over some issue,
> then I think more debugging is needed.
> 
> What exactly is going wrong here for you?

Hi Daniel and Sidong,

I noticed a similar issue when running the IGT test kms_cursor_crc. For
example, a subtest that passes on the first run (alpha-opaque) fails on
the second due to a kind of busy waiting in subtest preparation (the
subtest fails before actually running).

In addition, in the same test, the dpms subtest started to fail since
the commit that change from wait_for_vblanks to wait_for_flip_done. By
reverting this commit, the dpms subtest passes again and the sequential
subtests return to normal.

I am trying to figure out what's missing from using flip_done op on
vkms, since I am also interested in solving this problem and I
understand that the change for flip_done has been discussed in the past.

Do you have any idea?

Melissa

> -Daniel
> 
> > +
> >  	drm_atomic_helper_cleanup_planes(dev, old_state);
> >  }
> >  
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel