From: daniel@ffwll.ch To: unlisted-recipients:; (no To-header on input) Cc: Daniel Vetter <daniel@ffwll.ch>, Melissa Wen <melissa.srw@gmail.com>, Haneen Mohammed <hamohammed.sa@gmail.com>, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>, David Airlie <airlied@linux.ie>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, kernel-usp@googlegroups.com Subject: Re: [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush Date: Tue, 28 Jul 2020 23:55:15 +0200 [thread overview] Message-ID: <20200728215515.GG6419@phenom.ffwll.local> (raw) In-Reply-To: <20200728161634.GA6555@realwakka> 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: daniel@ffwll.ch Cc: Haneen Mohammed <hamohammed.sa@gmail.com>, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>, David Airlie <airlied@linux.ie>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, kernel-usp@googlegroups.com, Melissa Wen <melissa.srw@gmail.com> Subject: Re: [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush Date: Tue, 28 Jul 2020 23:55:15 +0200 [thread overview] Message-ID: <20200728215515.GG6419@phenom.ffwll.local> (raw) In-Reply-To: <20200728161634.GA6555@realwakka> 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 -- 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
next prev parent reply other threads:[~2020-07-28 21:55 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-22 11:04 [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush Melissa Wen 2020-07-22 11:04 ` Melissa Wen 2020-07-22 12:05 ` daniel 2020-07-22 12:05 ` daniel 2020-07-22 14:06 ` Melissa Wen 2020-07-22 14:06 ` Melissa Wen 2020-07-22 15:17 ` Daniel Vetter 2020-07-22 15:17 ` Daniel Vetter 2020-07-25 1:17 ` Sidong Yang 2020-07-25 1:17 ` Sidong Yang 2020-07-25 15:57 ` Daniel Vetter 2020-07-25 15:57 ` Daniel Vetter 2020-07-25 17:45 ` Melissa Wen 2020-07-25 17:45 ` Melissa Wen 2020-07-25 18:12 ` Daniel Vetter 2020-07-25 18:12 ` Daniel Vetter 2020-07-25 18:49 ` Melissa Wen 2020-07-25 18:49 ` Melissa Wen 2020-07-25 19:19 ` Melissa Wen 2020-07-25 19:19 ` Melissa Wen 2020-07-25 19:29 ` Melissa Wen 2020-07-25 19:29 ` Melissa Wen 2020-07-26 10:26 ` Daniel Vetter 2020-07-26 10:26 ` Daniel Vetter 2020-07-28 16:16 ` Sidong Yang 2020-07-28 16:16 ` Sidong Yang 2020-07-28 21:55 ` daniel [this message] 2020-07-28 21:55 ` daniel 2020-07-29 19:09 ` Melissa Wen 2020-07-29 19:09 ` Melissa Wen 2020-07-29 21:48 ` Daniel Vetter 2020-07-29 21:48 ` Daniel Vetter 2020-07-30 10:09 ` Melissa Wen 2020-07-30 10:09 ` Melissa Wen 2020-07-31 9:08 ` daniel 2020-07-31 9:08 ` daniel 2020-07-31 16:13 ` Sidong Yang 2020-07-31 16:13 ` Sidong Yang 2020-07-31 16:47 ` Melissa Wen 2020-07-31 16:47 ` Melissa Wen 2020-07-31 17:29 ` Leandro Ribeiro 2020-07-31 17:36 ` Leandro Ribeiro 2020-07-31 17:36 ` Leandro Ribeiro 2020-07-31 18:10 ` Leandro Ribeiro 2020-07-31 18:10 ` Leandro Ribeiro 2020-07-31 18:33 ` Daniel Vetter 2020-07-31 18:33 ` Daniel Vetter 2020-07-31 18:39 ` Leandro Ribeiro 2020-07-31 18:39 ` Leandro Ribeiro 2020-08-01 16:06 ` Sidong Yang 2020-08-01 16:06 ` Sidong Yang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200728215515.GG6419@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=Rodrigo.Siqueira@amd.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=hamohammed.sa@gmail.com \ --cc=kernel-usp@googlegroups.com \ --cc=linux-kernel@vger.kernel.org \ --cc=melissa.srw@gmail.com \ --cc=rodrigosiqueiramelo@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.