If interrupts are disabled (e.g. via vblank_disable_fn) and we come to
disable vblank, update the vblank count to best guess as to what it
would be had the interrupts remained enabled, and update the timesamp to
now.
This avoids a stale vblank event being sent while disabling crtcs during
atomic modeset.
Fixes: 68036b08b91bc ("drm/vblank: Do not update vblank count if interrupts
are already disabled.")
Signed-off-by: Robert Beckett <[email protected]>
---
drivers/gpu/drm/drm_vblank.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 7dabb2bdb733..db68b8cbf797 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -375,9 +375,23 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
* interrupts were enabled. This avoids calling the ->disable_vblank()
* operation in atomic context with the hardware potentially runtime
* suspended.
+ * If interrupts are disabled (e.g. via blank_disable_fn) then make
+ * best guess as to what it would be now and make sure we have an up
+ * to date timestamp.
*/
- if (!vblank->enabled)
+ if (!vblank->enabled) {
+ ktime_t now = ktime_get();
+ u32 diff = 0;
+ if (vblank->framedur_ns) {
+ u64 diff_ns = ktime_to_ns(ktime_sub(now, vblank->time));
+ diff = DIV_ROUND_CLOSEST_ULL(diff_ns,
+ vblank->framedur_ns);
+ }
+
+ store_vblank(dev, pipe, diff, now, vblank->count);
+
goto out;
+ }
/*
* Update the count and timestamp to maintain the
--
2.18.0
On Tue, Jun 25, 2019 at 06:59:14PM +0100, Robert Beckett wrote:
> If interrupts are disabled (e.g. via vblank_disable_fn) and we come to
> disable vblank, update the vblank count to best guess as to what it
> would be had the interrupts remained enabled, and update the timesamp to
> now.
>
> This avoids a stale vblank event being sent while disabling crtcs during
> atomic modeset.
>
> Fixes: 68036b08b91bc ("drm/vblank: Do not update vblank count if interrupts
> are already disabled.")
>
> Signed-off-by: Robert Beckett <[email protected]>
> ---
> drivers/gpu/drm/drm_vblank.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 7dabb2bdb733..db68b8cbf797 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -375,9 +375,23 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
> * interrupts were enabled. This avoids calling the ->disable_vblank()
> * operation in atomic context with the hardware potentially runtime
> * suspended.
> + * If interrupts are disabled (e.g. via blank_disable_fn) then make
> + * best guess as to what it would be now and make sure we have an up
> + * to date timestamp.
> */
> - if (!vblank->enabled)
> + if (!vblank->enabled) {
> + ktime_t now = ktime_get();
Would be nice to fake this a bit more accurately and round the timestamp
here to a multiple of the frame duration, i.e. ...
> + u32 diff = 0;
> + if (vblank->framedur_ns) {
> + u64 diff_ns = ktime_to_ns(ktime_sub(now, vblank->time));
> + diff = DIV_ROUND_CLOSEST_ULL(diff_ns,
> + vblank->framedur_ns);
> + }
now = vblank->time + diff * vblank_framedur_ns;
Picking the right macro for doing 64bit multiplies left to you :-)
> +
> + store_vblank(dev, pipe, diff, now, vblank->count);
> +
> goto out;
> + }
>
> /*
> * Update the count and timestamp to maintain the
Somewhat unhappy that we're duplicating this logic with
drm_update_vblank_count, but it's just 2 lines of math.
-Daniel
> --
> 2.18.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, Jun 25, 2019 at 06:59:14PM +0100, Robert Beckett wrote:
> If interrupts are disabled (e.g. via vblank_disable_fn) and we come to
> disable vblank, update the vblank count to best guess as to what it
> would be had the interrupts remained enabled, and update the timesamp to
> now.
>
> This avoids a stale vblank event being sent while disabling crtcs during
> atomic modeset.
>
> Fixes: 68036b08b91bc ("drm/vblank: Do not update vblank count if interrupts
> are already disabled.")
I don't understand that commit. drm_vblank_off() should be called
when the power is still present, so it looks to me like that
commit is actually wrong. So I think we may want to just revert
it and figure out what the actual bug was.
>
> Signed-off-by: Robert Beckett <[email protected]>
> ---
> drivers/gpu/drm/drm_vblank.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 7dabb2bdb733..db68b8cbf797 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -375,9 +375,23 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
> * interrupts were enabled. This avoids calling the ->disable_vblank()
> * operation in atomic context with the hardware potentially runtime
> * suspended.
> + * If interrupts are disabled (e.g. via blank_disable_fn) then make
> + * best guess as to what it would be now and make sure we have an up
> + * to date timestamp.
> */
> - if (!vblank->enabled)
> + if (!vblank->enabled) {
> + ktime_t now = ktime_get();
> + u32 diff = 0;
> + if (vblank->framedur_ns) {
> + u64 diff_ns = ktime_to_ns(ktime_sub(now, vblank->time));
> + diff = DIV_ROUND_CLOSEST_ULL(diff_ns,
> + vblank->framedur_ns);
> + }
> +
> + store_vblank(dev, pipe, diff, now, vblank->count);
> +
> goto out;
> + }
>
> /*
> * Update the count and timestamp to maintain the
> --
> 2.18.0
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
On Wed, Jun 26, 2019 at 04:27:32PM +0300, Ville Syrj?l? wrote:
> On Tue, Jun 25, 2019 at 06:59:14PM +0100, Robert Beckett wrote:
> > If interrupts are disabled (e.g. via vblank_disable_fn) and we come to
> > disable vblank, update the vblank count to best guess as to what it
> > would be had the interrupts remained enabled, and update the timesamp to
> > now.
> >
> > This avoids a stale vblank event being sent while disabling crtcs during
> > atomic modeset.
> >
> > Fixes: 68036b08b91bc ("drm/vblank: Do not update vblank count if interrupts
> > are already disabled.")
>
> I don't understand that commit. drm_vblank_off() should be called
> when the power is still present, so it looks to me like that
> commit is actually wrong. So I think we may want to just revert
> it and figure out what the actual bug was.
Hm yeah we might need a power domain get/put around our
drm_crtc_vblank_off() call to make sure it dtrt. Revert sounds like a good
idea instead of adding more kludges ... a-b: me on the revert, even though
I did ack the original patch too.
-Daniel
>
> >
> > Signed-off-by: Robert Beckett <[email protected]>
> > ---
> > drivers/gpu/drm/drm_vblank.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 7dabb2bdb733..db68b8cbf797 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -375,9 +375,23 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
> > * interrupts were enabled. This avoids calling the ->disable_vblank()
> > * operation in atomic context with the hardware potentially runtime
> > * suspended.
> > + * If interrupts are disabled (e.g. via blank_disable_fn) then make
> > + * best guess as to what it would be now and make sure we have an up
> > + * to date timestamp.
> > */
> > - if (!vblank->enabled)
> > + if (!vblank->enabled) {
> > + ktime_t now = ktime_get();
> > + u32 diff = 0;
> > + if (vblank->framedur_ns) {
> > + u64 diff_ns = ktime_to_ns(ktime_sub(now, vblank->time));
> > + diff = DIV_ROUND_CLOSEST_ULL(diff_ns,
> > + vblank->framedur_ns);
> > + }
> > +
> > + store_vblank(dev, pipe, diff, now, vblank->count);
> > +
> > goto out;
> > + }
> >
> > /*
> > * Update the count and timestamp to maintain the
> > --
> > 2.18.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrj?l?
> Intel
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch