Bug 79502

Something like http://cgit.collabora.com/git/user/pq/weston.git/commit/?h=next&id=06294b73add6fdbc42e4b28785bd9766c3161cd6

Note, that exact patch depends on at least Presentation feedback series: bug #83092

This patch :
http://cgit.freedesktop.org/~fredinfinite23/weston/commit/?h=bad_timestamp_abort&id=507cfb65fba6ed2f4e0b3973f6335ba98aa97894
is a fix for this,

but I'd like to discuss it some more, because I feel there is something missing:

1) False-positive detection:
   What if DRM driver 'sanely' returns a timestamp = 0 either by (mis)chance or because of some weird DRM initial condition/behavior (first frame ?). We should be given another chance to repaint.

2) Integer overflows:
   What if we arrived at frame_time's max_range ? Next timestamp would then break the 'cur > prev' rule and we would wrongly abort Weston.

I think those two considerations above call for some 'counter' on the detection. Chances that we 'sanely' hit (stamp==0 || cur < prev) are really low, but chances that we hit that twice in a row are practically 0. I don't see any need for a customizable counter; I would set it to 1 by default and exit Weston when it's 0.

opinions on that please :)

(In reply to Frederic Plourde from comment #2)
> 1) False-positive detection:
>    What if DRM driver 'sanely' returns a timestamp = 0 either by (mis)chance
> or because of some weird DRM initial condition/behavior (first frame ?). We
> should be given another chance to repaint.

You mean that CLOCK_MONOTONIC might actually be exactly {0, 0} once because it wraps around?

Ok, we could deal with that by checking both this and the previous timestamp against 0, and if both are 0, then decide it doesn't work and abort. But actually that's already included in the "timestamp must advance" check, so I think we could just drop the zero check.

You need to make sure the previous timestamp is initialized by a page flip event before testing advancement. For that you can use a tv_nsec value that is out of the legal range, like -1, on output init.

Btw. please do the comparison in struct timespec terms. Conversion to uint32_t in milliseconds (frame_time variable) is lossy, and wraps around every 49.7 days. We should gradually get rid of the uint32_t millisecond timestamp where possible.

> 2) Integer overflows:
>    What if we arrived at frame_time's max_range ? Next timestamp would then
> break the 'cur > prev' rule and we would wrongly abort Weston.

Oops, already replied to that above. That is why my original patch used timespec_cmp().

> I think those two considerations above call for some 'counter' on the
> detection. Chances that we 'sanely' hit (stamp==0 || cur < prev) are really
> low, but chances that we hit that twice in a row are practically 0. I don't
> see any need for a customizable counter; I would set it to 1 by default and
> exit Weston when it's 0.

No need for such counters if you follow my suggestions. It will be more accurate without such a counter, too.

PS. check you editor tab width setting. ;-)

Posted a patch for this
http://lists.freedesktop.org/archives/wayland-devel/2014-November/018069.html

We got some comments from Michel Dänzer who thinks aborting Weston is a bit harsh.

How about the following:
- Mario Kleiner confirmed that getting zero timestamp from DRM is indeed an indication of the driver needing fixing, so we should after all consider that bad.
- always log a warning on bad timestamp
- if we get a bad timestamp within the 5 first seconds of compositor uptime, exit. Otherwise try to keep going.

Yeah, that's more or less what I had understood from this last email/discussion (that I still have flagged in my inbox, btw), but haven't been able to allocate time to R&D in a short while.  I should be able to work on this shortly tho, so all good for now.

thanks

These all landed a while back.