[PATCH v3 0/4] handle vblank when disabling ctc with interrupt disabled (was [PATCH v2] drm/imx: correct order of crtc disable)

[PATCH v3 0/4] handle vblank when disabling ctc with interrupt disabled (was [PATCH v2] drm/imx: correct order of crtc disable)

[Robert Beckett]

Handle vblank event sent to signal crtc disable while the backend vblank
interrupt has already been disabled by vblank_disable_fn.

Fixes: a474478642d5 ("drm/imx: fix crtc vblank state regression")
Fixes: 68036b08b91bc ("drm/vblank: Do not update vblank count if interrupts are already disabled.")
Fixes: 5f2f911578fb ("drm/imx: atomic phase 3 step 1: Use atomic configuration")


Changes since v2:
Split up the patch in to smaller pieces.
Add warning when about to send bogus vblank event.
Update vblank to best guess info during drm_vblank_disable_and_save.

Robert Beckett (4):
  drm/vblank: warn on sending stale event
  drm/imx: notify drm core before sending event during crtc disable
  drm/vblank: estimate vblank while disabling vblank if interrupt
    disabled
  drm/imx: only send event on crtc disable if kept disabled

 drivers/gpu/drm/drm_vblank.c     | 33 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/imx/ipuv3-crtc.c |  6 +++---
 2 files changed, 35 insertions(+), 4 deletions(-)

-- 
2.18.0

[PATCH v3 1/4] drm/vblank: warn on sending stale event

[Robert Beckett]

Warn when about to send stale vblank info and add advice to
documentation on how to avoid.

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 603ab105125d..7dabb2bdb733 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -918,6 +918,19 @@ EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
  *
  * See drm_crtc_arm_vblank_event() for a helper which can be used in certain
  * situation, especially to send out events for atomic commit operations.
+ *
+ * Care should be taken to avoid stale timestamps. If:
+ *   - your driver has vblank support (i.e. dev->num_crtcs > 0)
+ *   - the vblank irq is off (i.e. no one called drm_crtc_vblank_get)
+ *   - from the vblank code's pov the pipe is still running (i.e. not
+ *     in-between a drm_crtc_vblank_off()/on() pair)
+ * If all of these conditions hold then drm_crtc_send_vblank_event is
+ * going to give you a garbage timestamp and and sequence number (the last
+ * recorded before the irq was disabled). If you call drm_crtc_vblank_get/put
+ * around it, or after vblank_off, then either of those will have rolled things
+ * forward for you.
+ * So, drivers should call drm_crtc_vblank_off() before this function in their
+ * crtc atomic_disable handlers.
  */
 void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 				struct drm_pending_vblank_event *e)
@@ -925,8 +938,12 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	u64 seq;
 	unsigned int pipe = drm_crtc_index(crtc);
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	ktime_t now;
 
+	WARN_ONCE(dev->num_crtcs > 0 && !vblank->enabled && !vblank->inmodeset,
+		  "sending stale vblank info\n");
+
 	if (dev->num_crtcs > 0) {
 		seq = drm_vblank_count_and_time(dev, pipe, &now);
 	} else {
-- 
2.18.0

Re: [PATCH v3 1/4] drm/vblank: warn on sending stale event

[Daniel Vetter]

On Tue, Jun 25, 2019 at 06:59:12PM +0100, Robert Beckett wrote:
> Warn when about to send stale vblank info and add advice to
> documentation on how to avoid.
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 603ab105125d..7dabb2bdb733 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -918,6 +918,19 @@ EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
>   *
>   * See drm_crtc_arm_vblank_event() for a helper which can be used in certain
>   * situation, especially to send out events for atomic commit operations.
> + *
> + * Care should be taken to avoid stale timestamps. If:
> + *   - your driver has vblank support (i.e. dev->num_crtcs > 0)
> + *   - the vblank irq is off (i.e. no one called drm_crtc_vblank_get)

drm_crtc_vblank_get() so it becomes a neat hyperlink.

> + *   - from the vblank code's pov the pipe is still running (i.e. not
> + *     in-between a drm_crtc_vblank_off()/on() pair)

Not sure the above will lead to great markup, maybe spell out the
drm_crtc_vblank_on() and maybe make it a bit clearer like "i.e. not after
the call to drm_crtc_vblank_off() but before the next call to
drm_crtc_vblank_on()" so it's clear which said of this pair we're talking
about.

> + * If all of these conditions hold then drm_crtc_send_vblank_event is

style nit: the enumeration is one sentence (and and oxford comman missing
but whatever), but you don't continue it here. Also, does the enumeration
look pretty in the htmldocs output?

> + * going to give you a garbage timestamp and and sequence number (the last
> + * recorded before the irq was disabled). If you call drm_crtc_vblank_get/put
> + * around it, or after vblank_off, then either of those will have rolled things
> + * forward for you.

Again pls fix the markup so all the function reference stick out and work.

> + * So, drivers should call drm_crtc_vblank_off() before this function in their
> + * crtc atomic_disable handlers.

Imo this sentence here is needed but a bit confusing, and we have it
documented already in the atomic_disable hook.

Other option would be to list all the places where a driver might want to
call this and how they could get it wrong, which imo doesn't make sense.

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>   */
>  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  				struct drm_pending_vblank_event *e)
> @@ -925,8 +938,12 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	u64 seq;
>  	unsigned int pipe = drm_crtc_index(crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	ktime_t now;
>  
> +	WARN_ONCE(dev->num_crtcs > 0 && !vblank->enabled && !vblank->inmodeset,
> +		  "sending stale vblank info\n");
> +
>  	if (dev->num_crtcs > 0) {
>  		seq = drm_vblank_count_and_time(dev, pipe, &now);
>  	} else {
> -- 
> 2.18.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 1/4] drm/vblank: warn on sending stale event

[Daniel Vetter]

On Tue, Jun 25, 2019 at 10:00:42PM +0200, Daniel Vetter wrote:
> On Tue, Jun 25, 2019 at 06:59:12PM +0100, Robert Beckett wrote:
> > Warn when about to send stale vblank info and add advice to
> > documentation on how to avoid.
> > 
> > Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 603ab105125d..7dabb2bdb733 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -918,6 +918,19 @@ EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
> >   *
> >   * See drm_crtc_arm_vblank_event() for a helper which can be used in certain
> >   * situation, especially to send out events for atomic commit operations.
> > + *
> > + * Care should be taken to avoid stale timestamps. If:
> > + *   - your driver has vblank support (i.e. dev->num_crtcs > 0)
> > + *   - the vblank irq is off (i.e. no one called drm_crtc_vblank_get)
> 
> drm_crtc_vblank_get() so it becomes a neat hyperlink.
> 
> > + *   - from the vblank code's pov the pipe is still running (i.e. not
> > + *     in-between a drm_crtc_vblank_off()/on() pair)
> 
> Not sure the above will lead to great markup, maybe spell out the
> drm_crtc_vblank_on() and maybe make it a bit clearer like "i.e. not after
> the call to drm_crtc_vblank_off() but before the next call to
> drm_crtc_vblank_on()" so it's clear which said of this pair we're talking
> about.
> 
> > + * If all of these conditions hold then drm_crtc_send_vblank_event is
> 
> style nit: the enumeration is one sentence (and and oxford comman missing
> but whatever), but you don't continue it here. Also, does the enumeration
> look pretty in the htmldocs output?
> 
> > + * going to give you a garbage timestamp and and sequence number (the last
> > + * recorded before the irq was disabled). If you call drm_crtc_vblank_get/put
> > + * around it, or after vblank_off, then either of those will have rolled things
> > + * forward for you.
> 
> Again pls fix the markup so all the function reference stick out and work.

One more style nit: s/you/drivers/, so maybe:

"Drivers must either hold a vblank reference acquired through
drm_crtc_vblank_get() or the vblank must have been shut off by calling
drm_crtc_vblank_off()." Those functions then have in turn links and hints
what you also need to do, like not forget to call drm_crtc_vblank_put().
> 
> > + * So, drivers should call drm_crtc_vblank_off() before this function in their
> > + * crtc atomic_disable handlers.
> 
> Imo this sentence here is needed but a bit confusing, and we have it
> documented already in the atomic_disable hook.
> 
> Other option would be to list all the places where a driver might want to
> call this and how they could get it wrong, which imo doesn't make sense.
> 
> With the nits addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> >   */
> >  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> >  				struct drm_pending_vblank_event *e)
> > @@ -925,8 +938,12 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	u64 seq;
> >  	unsigned int pipe = drm_crtc_index(crtc);
> > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> >  	ktime_t now;
> >  
> > +	WARN_ONCE(dev->num_crtcs > 0 && !vblank->enabled && !vblank->inmodeset,
> > +		  "sending stale vblank info\n");
> > +
> >  	if (dev->num_crtcs > 0) {
> >  		seq = drm_vblank_count_and_time(dev, pipe, &now);
> >  	} else {
> > -- 
> > 2.18.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 2/4] drm/imx: notify drm core before sending event during crtc disable

[Robert Beckett]

Notify drm core before sending pending events during crtc disable.
This fixes the first event after disable having an old stale timestamp
by having drm_crtc_vblank_off update the timestamp to now.

This was seen while debugging weston log message:
Warning: computed repaint delay is insane: -8212 msec

This occured due to:
1. driver starts up
2. fbcon comes along and restores fbdev, enabling vblank
3. vblank_disable_fn fires via timer disabling vblank, keeping vblank
seq number and time set at current value
(some time later)
4. weston starts and does a modeset
5. atomic commit disables crtc while it does the modeset
6. ipu_crtc_atomic_disable sends vblank with old seq number and time

Fixes: a474478642d5 ("drm/imx: fix crtc vblank state regression")

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 9cc1d678674f..e04d6efff1b5 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -91,14 +91,14 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
 	ipu_dc_disable(ipu);
 	ipu_prg_disable(ipu);
 
+	drm_crtc_vblank_off(crtc);
+
 	spin_lock_irq(&crtc->dev->event_lock);
 	if (crtc->state->event) {
 		drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		crtc->state->event = NULL;
 	}
 	spin_unlock_irq(&crtc->dev->event_lock);
-
-	drm_crtc_vblank_off(crtc);
 }
 
 static void imx_drm_crtc_reset(struct drm_crtc *crtc)
-- 
2.18.0

Re: [PATCH v3 2/4] drm/imx: notify drm core before sending event during crtc disable

[Daniel Vetter]

On Tue, Jun 25, 2019 at 06:59:13PM +0100, Robert Beckett wrote:
> Notify drm core before sending pending events during crtc disable.
> This fixes the first event after disable having an old stale timestamp
> by having drm_crtc_vblank_off update the timestamp to now.
> 
> This was seen while debugging weston log message:
> Warning: computed repaint delay is insane: -8212 msec
> 
> This occured due to:
> 1. driver starts up
> 2. fbcon comes along and restores fbdev, enabling vblank
> 3. vblank_disable_fn fires via timer disabling vblank, keeping vblank
> seq number and time set at current value
> (some time later)
> 4. weston starts and does a modeset
> 5. atomic commit disables crtc while it does the modeset
> 6. ipu_crtc_atomic_disable sends vblank with old seq number and time
> 
> Fixes: a474478642d5 ("drm/imx: fix crtc vblank state regression")
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>

Now that I understand what's going on here:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 9cc1d678674f..e04d6efff1b5 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -91,14 +91,14 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  	ipu_dc_disable(ipu);
>  	ipu_prg_disable(ipu);
>  
> +	drm_crtc_vblank_off(crtc);
> +
>  	spin_lock_irq(&crtc->dev->event_lock);
>  	if (crtc->state->event) {
>  		drm_crtc_send_vblank_event(crtc, crtc->state->event);
>  		crtc->state->event = NULL;
>  	}
>  	spin_unlock_irq(&crtc->dev->event_lock);
> -
> -	drm_crtc_vblank_off(crtc);
>  }
>  
>  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
> -- 
> 2.18.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 3/4] drm/vblank: estimate vblank while disabling vblank if interrupt disabled

[Robert Beckett]

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 <bob.beckett@collabora.com>
---
 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

Re: [PATCH v3 3/4] drm/vblank: estimate vblank while disabling vblank if interrupt disabled

[Daniel Vetter]

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 <bob.beckett@collabora.com>
> ---
>  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

Re: [PATCH v3 3/4] drm/vblank: estimate vblank while disabling vblank if interrupt disabled

[Ville Syrjälä]

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 <bob.beckett@collabora.com>
> ---
>  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
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 3/4] drm/vblank: estimate vblank while disabling vblank if interrupt disabled

[Daniel Vetter]

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 <bob.beckett@collabora.com>
> > ---
> >  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
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> 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

[PATCH v3 4/4] drm/imx: only send event on crtc disable if kept disabled

[Robert Beckett]

The event will be sent as part of the vblank enable during the modeset
if the crtc is not being kept disabled.

Fixes: 5f2f911578fb ("drm/imx: atomic phase 3 step 1: Use atomic configuration")

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index e04d6efff1b5..c436a28d50e4 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -94,7 +94,7 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 
 	spin_lock_irq(&crtc->dev->event_lock);
-	if (crtc->state->event) {
+	if (crtc->state->event && !crtc->state->active) {
 		drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		crtc->state->event = NULL;
 	}
-- 
2.18.0

Re: [PATCH v3 4/4] drm/imx: only send event on crtc disable if kept disabled

[Daniel Vetter]

On Tue, Jun 25, 2019 at 06:59:15PM +0100, Robert Beckett wrote:
> The event will be sent as part of the vblank enable during the modeset
> if the crtc is not being kept disabled.
> 
> Fixes: 5f2f911578fb ("drm/imx: atomic phase 3 step 1: Use atomic configuration")
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index e04d6efff1b5..c436a28d50e4 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -94,7 +94,7 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  	drm_crtc_vblank_off(crtc);
>  
>  	spin_lock_irq(&crtc->dev->event_lock);
> -	if (crtc->state->event) {
> +	if (crtc->state->event && !crtc->state->active) {
>  		drm_crtc_send_vblank_event(crtc, crtc->state->event);
>  		crtc->state->event = NULL;
>  	}
> -- 
> 2.18.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 4/4] drm/imx: only send event on crtc disable if kept disabled

[Philipp Zabel]

On Tue, 2019-06-25 at 22:22 +0200, Daniel Vetter wrote:
> On Tue, Jun 25, 2019 at 06:59:15PM +0100, Robert Beckett wrote:
> > The event will be sent as part of the vblank enable during the modeset
> > if the crtc is not being kept disabled.
> > 
> > Fixes: 5f2f911578fb ("drm/imx: atomic phase 3 step 1: Use atomic configuration")
> > 
> > Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thank you, patches 2 and 4 applied to imx-drm/fixes.

regards
Philipp