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

[Devarsh Thakkar <devarsht@ti.com>]

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;
>   	}
>