[PATCH 0/2] drm/tidss: Fix issue with page flip timestamp on enable

[PATCH 0/2] drm/tidss: Fix issue with page flip timestamp on enable

[Tomi Valkeinen]

This fixes an issue with the page flip timestamp being 0 on CRTC enable.
See the second patch for in depth explanation

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Tomi Valkeinen (2):
      drm/tidss: Restructure dispc_vp_prepare() and dispc_vp_enable()
      drm/tidss: Set vblank (event) time at crtc_atomic_enable

 drivers/gpu/drm/tidss/tidss_crtc.c  |  9 +++++++--
 drivers/gpu/drm/tidss/tidss_dispc.c | 22 ++++++----------------
 drivers/gpu/drm/tidss/tidss_dispc.h |  3 +--
 3 files changed, 14 insertions(+), 20 deletions(-)
---
base-commit: 03e7ae93c6e32206797c13118659a966ae84a3bb
change-id: 20250905-tidss-fix-timestamp-f40cd82ed309

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 1/2] drm/tidss: Restructure dispc_vp_prepare() and dispc_vp_enable()

[Tomi Valkeinen]

tidss_crtc.c calls dispc_vp_prepare() and dispc_vp_enable() in that
order, next to each other. dispc_vp_prepare() does preparations for
enabling the crtc, by writing some registers, and dispc_vp_enable() does
more preparations. As the last thing, dispc_vp_enable() enables the CRTC
by writing the enable bit.

There might have been a reason at some point in the history for this
split, but I can't find any point to it. They also do a bit of
overlapping work: both call dispc_vp_find_bus_fmt(). They could as well
be a single function.

But instead of combining them, this patch moves everything from
dispc_vp_enable() to dispc_vp_prepare(), except the actual CRTC enable
bit write. The reason for this is that unlike all the preparatory
register writes, CRTC enable has an immediate effect, starting the
timing generator and the CRTC as a whole. Thus it may be important to
time the enable just right (as we do in the next patch).

No functional changes.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
 drivers/gpu/drm/tidss/tidss_dispc.c | 22 ++++++----------------
 drivers/gpu/drm/tidss/tidss_dispc.h |  3 +--
 3 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index da89fd01c337..1b767af8e1f6 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -244,7 +244,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	dispc_vp_prepare(tidss->dispc, tcrtc->hw_videoport, crtc->state);
 
-	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport, crtc->state);
+	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
 
 	spin_lock_irqsave(&ddev->event_lock, flags);
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 7c8c15a5c39b..d4762410d262 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1161,6 +1161,9 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
 {
 	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
 	const struct dispc_bus_format *fmt;
+	const struct drm_display_mode *mode = &state->adjusted_mode;
+	bool align, onoff, rf, ieo, ipc, ihs, ivs;
+	u32 hsw, hfp, hbp, vsw, vfp, vbp;
 
 	fmt = dispc_vp_find_bus_fmt(dispc, hw_videoport, tstate->bus_format,
 				    tstate->bus_flags);
@@ -1173,22 +1176,6 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
 
 		dispc_enable_am65x_oldi(dispc, hw_videoport, fmt);
 	}
-}
-
-void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
-		     const struct drm_crtc_state *state)
-{
-	const struct drm_display_mode *mode = &state->adjusted_mode;
-	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
-	bool align, onoff, rf, ieo, ipc, ihs, ivs;
-	const struct dispc_bus_format *fmt;
-	u32 hsw, hfp, hbp, vsw, vfp, vbp;
-
-	fmt = dispc_vp_find_bus_fmt(dispc, hw_videoport, tstate->bus_format,
-				    tstate->bus_flags);
-
-	if (WARN_ON(!fmt))
-		return;
 
 	dispc_set_num_datalines(dispc, hw_videoport, fmt->data_width);
 
@@ -1244,7 +1231,10 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 				  mode->crtc_hdisplay - 1) |
 		       FIELD_PREP(DISPC_VP_SIZE_SCREEN_VDISPLAY_MASK,
 				  mode->crtc_vdisplay - 1));
+}
 
+void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport)
+{
 	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1,
 		       DISPC_VP_CONTROL_ENABLE_MASK);
 }
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 60c1b400eb89..f38493a70122 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -119,8 +119,7 @@ void dispc_ovr_enable_layer(struct dispc_device *dispc,
 
 void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
 		      const struct drm_crtc_state *state);
-void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
-		     const struct drm_crtc_state *state);
+void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport);
 void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport);
 void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport);
 bool dispc_vp_go_busy(struct dispc_device *dispc, u32 hw_videoport);

-- 
2.43.0

Re: [PATCH 1/2] drm/tidss: Restructure dispc_vp_prepare() and dispc_vp_enable()

[Devarsh Thakkar]

On 05/09/25 19:28, Tomi Valkeinen wrote:
> tidss_crtc.c calls dispc_vp_prepare() and dispc_vp_enable() in that
> order, next to each other. dispc_vp_prepare() does preparations for
> enabling the crtc, by writing some registers, and dispc_vp_enable() does
> more preparations. As the last thing, dispc_vp_enable() enables the CRTC
> by writing the enable bit.
> 
> There might have been a reason at some point in the history for this
> split, but I can't find any point to it. They also do a bit of
> overlapping work: both call dispc_vp_find_bus_fmt(). They could as well
> be a single function.
> 
> But instead of combining them, this patch moves everything from
> dispc_vp_enable() to dispc_vp_prepare(), except the actual CRTC enable
> bit write. The reason for this is that unlike all the preparatory
> register writes, CRTC enable has an immediate effect, starting the
> timing generator and the CRTC as a whole. Thus it may be important to
> time the enable just right (as we do in the next patch).
> 
> No functional changes.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

> ---
>   drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
>   drivers/gpu/drm/tidss/tidss_dispc.c | 22 ++++++----------------
>   drivers/gpu/drm/tidss/tidss_dispc.h |  3 +--
>   3 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index da89fd01c337..1b767af8e1f6 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -244,7 +244,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
>   
>   	dispc_vp_prepare(tidss->dispc, tcrtc->hw_videoport, crtc->state);
>   
> -	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport, crtc->state);
> +	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
>   
>   	spin_lock_irqsave(&ddev->event_lock, flags);
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 7c8c15a5c39b..d4762410d262 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1161,6 +1161,9 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   {
>   	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
>   	const struct dispc_bus_format *fmt;
> +	const struct drm_display_mode *mode = &state->adjusted_mode;
> +	bool align, onoff, rf, ieo, ipc, ihs, ivs;
> +	u32 hsw, hfp, hbp, vsw, vfp, vbp;
>   
>   	fmt = dispc_vp_find_bus_fmt(dispc, hw_videoport, tstate->bus_format,
>   				    tstate->bus_flags);
> @@ -1173,22 +1176,6 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   
>   		dispc_enable_am65x_oldi(dispc, hw_videoport, fmt);
>   	}
> -}
> -
> -void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
> -		     const struct drm_crtc_state *state)
> -{
> -	const struct drm_display_mode *mode = &state->adjusted_mode;
> -	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
> -	bool align, onoff, rf, ieo, ipc, ihs, ivs;
> -	const struct dispc_bus_format *fmt;
> -	u32 hsw, hfp, hbp, vsw, vfp, vbp;
> -
> -	fmt = dispc_vp_find_bus_fmt(dispc, hw_videoport, tstate->bus_format,
> -				    tstate->bus_flags);
> -
> -	if (WARN_ON(!fmt))
> -		return;
>   
>   	dispc_set_num_datalines(dispc, hw_videoport, fmt->data_width);
>   
> @@ -1244,7 +1231,10 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>   				  mode->crtc_hdisplay - 1) |
>   		       FIELD_PREP(DISPC_VP_SIZE_SCREEN_VDISPLAY_MASK,
>   				  mode->crtc_vdisplay - 1));
> +}
>   
> +void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport)
> +{
>   	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1,
>   		       DISPC_VP_CONTROL_ENABLE_MASK);
>   }
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 60c1b400eb89..f38493a70122 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -119,8 +119,7 @@ void dispc_ovr_enable_layer(struct dispc_device *dispc,
>   
>   void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   		      const struct drm_crtc_state *state);
> -void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
> -		     const struct drm_crtc_state *state);
> +void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport);
>   void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport);
>   void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport);
>   bool dispc_vp_go_busy(struct dispc_device *dispc, u32 hw_videoport);
> 

[PATCH 2/2] drm/tidss: Set vblank (event) time at crtc_atomic_enable

[Tomi Valkeinen]

It was reported that Weston stops at an assert, which checks that the
page flip event timestamp is the same or newer than the previous
timestamp:

weston_output_finish_frame: Assertion `timespec_sub_to_nsec(stamp, &output->frame_time) >= 0' failed.

With manual tests, I can see that when I enable the CRTC, I get a page
flip event with a timestamp of 0. Tracking this down led to
drm_reset_vblank_timestamp() which does "t_vblank = 0" if
"high-precision query" is not available.

TI DSS does not have any hardware timestamping, and thus the default
ktime_get() is used in the DRM framework to get the vblank timestamp,
and ktime_get() is not "high precision" here.

It is not quite clear why the framework behaves this way, but I assume
the idea is that drm_crtc_vblank_on(), which calls
drm_reset_vblank_timestamp(), can be called at any time, and thus
ktime_get() wouldn't give a good timestamp. And, the idea is that the
driver would wait until next vblank after the CRTC enable, and then we
could get a good timestamp. This is hinted in the comment: "reinitialize
delayed at next vblank interrupt and assign 0 for now".

I think that makes sense. However, when we enable the CRTC in TI DSS,
i.e. we write the enable bit to the hardware, that's the exact moment
when the "vblank cycle" starts. It is the zero point in the cycle, and
thus ktime_get() would give a good timestamp.

I am not sure if this is applicable to other hardware, and if so, how
should it be solved in the framework. So, let's fix this in the tidss
driver at least for now.

This patch updates the vblank->time manually to ktime_get() just before
sending the vblank event, and we enable the crtc just before calling
ktime_get(). To get even more exact timing, the dispc_vp_enable() is
moved inside the event_lock spinlock.

With this, we get a proper timestamp for the page flip event from
enabling the CRTC, and Weston is happy.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 1b767af8e1f6..6898f12bb364 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -244,11 +244,16 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	dispc_vp_prepare(tidss->dispc, tcrtc->hw_videoport, crtc->state);
 
-	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
-
 	spin_lock_irqsave(&ddev->event_lock, flags);
 
+	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
+
 	if (crtc->state->event) {
+		unsigned int pipe = drm_crtc_index(crtc);
+		struct drm_vblank_crtc *vblank = &ddev->vblank[pipe];
+
+		vblank->time = ktime_get();
+
 		drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		crtc->state->event = NULL;
 	}

-- 
2.43.0

Re: [PATCH 2/2] drm/tidss: Set vblank (event) time at crtc_atomic_enable

[Devarsh Thakkar]

Hi Tomi,

Thanks for the patch.

On 05/09/25 19:28, Tomi Valkeinen wrote:
> It was reported that Weston stops at an assert, which checks that the
> page flip event timestamp is the same or newer than the previous
> timestamp:
> 
> weston_output_finish_frame: Assertion `timespec_sub_to_nsec(stamp, &output->frame_time) >= 0' failed.
> 

As I understand, this patch fixes above assertion error related to bad 
time-stamping. I believe it is fixing this issue [0] and probably this 
[1] ?. So I think it should be better to use a Fixes tag.

> With manual tests, I can see that when I enable the CRTC, I get a page
> flip event with a timestamp of 0. Tracking this down led to
> drm_reset_vblank_timestamp() which does "t_vblank = 0" if
> "high-precision query" is not available.
> 
> TI DSS does not have any hardware timestamping, and thus the default
> ktime_get() is used in the DRM framework to get the vblank timestamp,
> and ktime_get() is not "high precision" here.
> 
> It is not quite clear why the framework behaves this way, but I assume
> the idea is that drm_crtc_vblank_on(), which calls
> drm_reset_vblank_timestamp(), can be called at any time, and thus
> ktime_get() wouldn't give a good timestamp. And, the idea is that the
> driver would wait until next vblank after the CRTC enable, and then we
> could get a good timestamp. This is hinted in the comment: "reinitialize
> delayed at next vblank interrupt and assign 0 for now".
> 
> I think that makes sense. However, when we enable the CRTC in TI DSS,
> i.e. we write the enable bit to the hardware, that's the exact moment
> when the "vblank cycle" starts. It is the zero point in the cycle, and
> thus ktime_get() would give a good timestamp.
> 
> I am not sure if this is applicable to other hardware, and if so, how
> should it be solved in the framework. So, let's fix this in the tidss
> driver at least for now.
> 
> This patch updates the vblank->time manually to ktime_get() just before
> sending the vblank event, and we enable the crtc just before calling
> ktime_get(). To get even more exact timing, the dispc_vp_enable() is
> moved inside the event_lock spinlock.
> 
> With this, we get a proper timestamp for the page flip event from
> enabling the CRTC, and Weston is happy.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

With the Fixes tag applied,
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

[0]: 
https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1553964/processor-sdk-am62x-weston-fails-to-wake-from-idle-time-sleep-restarts-after-sigterm
[1]: 
https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1416342/am625-am625-doesn-t-wake-up-from-standy-when-idle-time-is-configured-in-weston-ini

Regards
Devarsh


> ---
>   drivers/gpu/drm/tidss/tidss_crtc.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 1b767af8e1f6..6898f12bb364 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -244,11 +244,16 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
>   
>   	dispc_vp_prepare(tidss->dispc, tcrtc->hw_videoport, crtc->state);
>   
> -	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
> -
>   	spin_lock_irqsave(&ddev->event_lock, flags);
>   
> +	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
> +
>   	if (crtc->state->event) {
> +		unsigned int pipe = drm_crtc_index(crtc);
> +		struct drm_vblank_crtc *vblank = &ddev->vblank[pipe];
> +
> +		vblank->time = ktime_get();
> +
>   		drm_crtc_send_vblank_event(crtc, crtc->state->event);
>   		crtc->state->event = NULL;
>   	}
>