On Tue, Jul 28, 2020 at 04:16:34PM +0000, Sidong Yang wrote:
On Sun, Jul 26, 2020 at 12:26:08PM +0200, Daniel Vetter wrote:
On Sat, Jul 25, 2020 at 9:29 PM Melissa Wen melissa.srw@gmail.com wrote:
On Sat, Jul 25, 2020 at 4:19 PM Melissa Wen melissa.srw@gmail.com wrote:
No, this very first warning continues (only once) :( From here (drm_crtc_vblank_on): if (atomic_read(&vblank->refcount) != 0 || drm_vblank_offdelay == 0) drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
Sorry, not sure when this warning is triggered.
Again, I just had to look at the trace: [ 52.299388] drm_get_last_vbltimestamp+0xaa/0xc0 [drm] [ 52.299389] drm_reset_vblank_timestamp+0x5b/0xd0 [drm] [ 52.299389] drm_crtc_vblank_on.cold+0x37/0x103 [drm] [ 52.299390] drm_atomic_helper_commit_modeset_enable
Yeah I think vkms can't generate a reasonable timestamp when the hrtimer is off. I thought the warning comes from a different callchain, but seems to be a general problem.
I guess in the vkms timestamp function we should check whether the timer is running, and if it's not running, then we just grab the current time and done.
I tried some test about this scenario that commit_tail calls in sequence disable
- enable - commit.
In a first test. there was a warning and found out that it raised from vkms_get_vblank_timestamp() the code checking vblank_hrtimer's expire time and vblank_time. In first run, vblank_time and hrtimer's expire time was both zero. because vblank wasn't happened yet. this warning wasn't happend since second run that vblank time was set from first run.
I don't know it's good way to solve the problem. Is there no problem in other drm modules?
Generally real hw drivers always have working clocks, not like the fake ones we have here :-) The idea behind the timestamp callback is that when vblank interrupts aren't enabled, the timestamp will help us keep track of how many vblanks have happened.
So I think (but might be wrong) correct fix for this issue would be to check whether vblanks are enabled, and if not, simply pass back the current system time. That's a lie, but much better than whatever value was set last time around the hrtimer fired- e.g. similar problem can happen later on when the vblank interrupt was off for a very long time. -Daniel
-Sidong
-Daniel
But I'm still wondering why after step 3 we don't get -EINVAL from vblank_get() - after vblank_off() vblank->enabled should be false again, getting us back to the same state as after 1. Is that not happening?
Yes (sorry if it got confused), we got -EINVAL after setp 3:
In step 3, at the end of the 2nd running, we have: atomic_disable --> vblank_off [!vblank->inmodeset + refcount going 0->1 + inmodeset=1] and then in next vblank_get: -EINVAL (!vblank->enabled + refcount ends 1) as in the first step.
Melissa
-Daniel
> > > > > > > Thanks > > > -Sidong > > > > > > > > > > > > > > > > crtc->state->event = NULL; > > > > > > > } > > > > > > > > > > > > > > -- > > > > > > > 2.27.0 > > > > > > > > > > > > > > > > > > > -- > > > > > > Daniel Vetter > > > > > > Software Engineer, Intel Corporation > > > > > > http://blog.ffwll.ch > > > > > > > > > > > > > > > > -- > > > > 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 > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch