[PATCH weston 2/8] shared: Add helpers to convert between protocol data and timespec
Pekka Paalanen
ppaalanen at gmail.com
Tue Dec 12 10:09:59 UTC 2017
On Mon, 4 Dec 2017 15:34:02 +0200
Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> Add helpers to safely convert between struct timespec values and
> tv_sec_hi, tv_sec_lo, tv_nsec triplets used for sending high-resolution
> timestamp data over the wayland protocol. Replace existing conversion
> code with the helper functions.
>
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> ---
> clients/presentation-shm.c | 9 +------
> libweston/compositor.c | 9 ++++---
> shared/timespec-util.h | 39 +++++++++++++++++++++++++++
> tests/presentation-test.c | 9 +------
> tests/timespec-test.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 112 insertions(+), 20 deletions(-)
Hi,
I have questions below that will have implications to the protocol
extension spec as well. I would like to require the time values in
protocol to be normalized.
>
> diff --git a/clients/presentation-shm.c b/clients/presentation-shm.c
> index c9fb66cc..d6a939e5 100644
> --- a/clients/presentation-shm.c
> +++ b/clients/presentation-shm.c
> @@ -39,6 +39,7 @@
> #include <wayland-client.h>
> #include "shared/helpers.h"
> #include "shared/zalloc.h"
> +#include "shared/timespec-util.h"
> #include "shared/os-compatibility.h"
> #include "presentation-time-client-protocol.h"
>
> @@ -383,14 +384,6 @@ timespec_to_ms(const struct timespec *ts)
> return (uint32_t)ts->tv_sec * 1000 + ts->tv_nsec / 1000000;
> }
>
> -static void
> -timespec_from_proto(struct timespec *tm, uint32_t tv_sec_hi,
> - uint32_t tv_sec_lo, uint32_t tv_nsec)
> -{
> - tm->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
> - tm->tv_nsec = tv_nsec;
> -}
> -
> static int
> timespec_diff_to_usec(const struct timespec *a, const struct timespec *b)
> {
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index 7d7a17ed..083664fd 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -341,7 +341,9 @@ weston_presentation_feedback_present(
> {
> struct wl_client *client = wl_resource_get_client(feedback->resource);
> struct wl_resource *o;
> - uint64_t secs;
> + uint32_t tv_sec_hi;
> + uint32_t tv_sec_lo;
> + uint32_t tv_nsec;
A suggestion: how about introducing
struct timespec_proto {
uint32_t sec_hi;
uint32_t sec_lo;
uint32_t nsec;
};
and using that in timespec_to_proto()?
(Not useful for timespec_from_proto() because the three variables are
already declared.)
>
> wl_resource_for_each(o, &output->resource_list) {
> if (wl_resource_get_client(o) != client)
> @@ -350,10 +352,9 @@ weston_presentation_feedback_present(
> wp_presentation_feedback_send_sync_output(feedback->resource, o);
> }
>
> - secs = ts->tv_sec;
> + timespec_to_proto(ts, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> wp_presentation_feedback_send_presented(feedback->resource,
> - secs >> 32, secs & 0xffffffff,
> - ts->tv_nsec,
> + tv_sec_hi, tv_sec_lo, tv_nsec,
> refresh_nsec,
> seq >> 32, seq & 0xffffffff,
> flags | feedback->psf_flags);
> diff --git a/shared/timespec-util.h b/shared/timespec-util.h
> index a10edf5b..c734accd 100644
> --- a/shared/timespec-util.h
> +++ b/shared/timespec-util.h
> @@ -175,6 +175,30 @@ timespec_to_usec(const struct timespec *a)
> return (int64_t)a->tv_sec * 1000000 + a->tv_nsec / 1000;
> }
>
> +/* Convert timespec to protocol data
> + *
> + * \param a timespec
> + * \param tv_sec_hi[out] the high bytes of the seconds part
> + * \param tv_sec_lo[out] the low bytes of the seconds part
> + * \param tv_nsec[out] the nanoseconds part
> + *
> + * The timespec is normalized before being converted to protocol data.
> + */
> +static inline void
> +timespec_to_proto(const struct timespec *a, uint32_t *tv_sec_hi,
> + uint32_t *tv_sec_lo, uint32_t *tv_nsec)
> +{
> + struct timespec r;
> +
> + timespec_normalize(&r, a);
> +
> + /* We check the size of tv_sec, so that we shift only if the size
> + * is 64-bits, in order to avoid sign extension on 32-bit systems. */
> + *tv_sec_hi = sizeof(r.tv_sec) == 8 ? (int64_t)r.tv_sec >> 32 : 0;
How about just casting tv_sec to uint64_t before shifting and masking
it to 32-bit pieces? I think that would be more obvious to read.
I think it would be better to assert() that the timespec is already:
- normalized, because otherwise nsec might overflow in a computation,
and
- non-negative, because the protocol cannot carry negative
timestamps.
> + *tv_sec_lo = r.tv_sec;
> + *tv_nsec = r.tv_nsec;
> +}
> +
> /* Convert nanoseconds to timespec
> *
> * \param a timespec
> @@ -209,6 +233,21 @@ timespec_from_msec(struct timespec *a, int64_t b)
> timespec_from_nsec(a, b * 1000000);
> }
>
> +/* Convert protocol data to timespec
> + *
> + * \param a[out] timespec
> + * \param tv_sec_hi the high bytes of seconds part
> + * \param tv_sec_lo the low bytes of seconds part
> + * \param tv_nsec the nanoseconds part
> + */
> +static inline void
> +timespec_from_proto(struct timespec *a, uint32_t tv_sec_hi,
> + uint32_t tv_sec_lo, uint32_t tv_nsec)
> +{
> + a->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
How to handle overflows in tv_sec would be a good question...
> + a->tv_nsec = tv_nsec;
> +}
Btw. refactoring code to introduce timespec_from_proto() with no
functional changes should probably be a patch of its own.
Thanks,
pq
> +
> /* Check if a timespec is zero
> *
> * \param a timespec
> diff --git a/tests/presentation-test.c b/tests/presentation-test.c
> index f12f8eef..f6ffe480 100644
> --- a/tests/presentation-test.c
> +++ b/tests/presentation-test.c
> @@ -34,6 +34,7 @@
>
> #include "shared/helpers.h"
> #include "shared/xalloc.h"
> +#include "shared/timespec-util.h"
> #include "weston-test-client-helper.h"
> #include "presentation-time-client-protocol.h"
>
> @@ -85,14 +86,6 @@ struct feedback {
> uint32_t flags;
> };
>
> -static void
> -timespec_from_proto(struct timespec *tm, uint32_t tv_sec_hi,
> - uint32_t tv_sec_lo, uint32_t tv_nsec)
> -{
> - tm->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
> - tm->tv_nsec = tv_nsec;
> -}
> -
> static void
> feedback_sync_output(void *data,
> struct wp_presentation_feedback *presentation_feedback,
> diff --git a/tests/timespec-test.c b/tests/timespec-test.c
> index 8c2296d1..9000295a 100644
> --- a/tests/timespec-test.c
> +++ b/tests/timespec-test.c
> @@ -38,6 +38,12 @@
> #include "shared/helpers.h"
> #include "zunitc/zunitc.h"
>
> +static uint32_t
> +get_tv_sec_hi(const struct timespec *ts)
> +{
> + return sizeof(ts->tv_sec) == 8 ? (int64_t)ts->tv_sec >> 32 : 0;
> +}
> +
> ZUC_TEST(timespec_test, timespec_normalize)
> {
> struct timespec a, r;
> @@ -143,6 +149,49 @@ ZUC_TEST(timespec_test, timespec_to_msec)
> ZUC_ASSERT_EQ(timespec_to_msec(&a), (4000ULL) + 4);
> }
>
> +ZUC_TEST(timespec_test, timespec_to_proto)
> +{
> + struct timespec a;
> + uint32_t tv_sec_hi;
> + uint32_t tv_sec_lo;
> + uint32_t tv_nsec;
> +
> + a.tv_sec = 0;
> + a.tv_nsec = 0;
> + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> + ZUC_ASSERT_EQ(0, tv_sec_hi);
> + ZUC_ASSERT_EQ(0, tv_sec_lo);
> + ZUC_ASSERT_EQ(0, tv_nsec);
> +
> + a.tv_sec = 1234;
> + a.tv_nsec = NSEC_PER_SEC - 1;
> + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> + ZUC_ASSERT_EQ(0, tv_sec_hi);
> + ZUC_ASSERT_EQ(1234, tv_sec_lo);
> + ZUC_ASSERT_EQ(NSEC_PER_SEC - 1, tv_nsec);
> +
> + a.tv_sec = 1234;
> + a.tv_nsec = NSEC_PER_SEC + 1;
> + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> + ZUC_ASSERT_EQ(0, tv_sec_hi);
> + ZUC_ASSERT_EQ(1235, tv_sec_lo);
> + ZUC_ASSERT_EQ(1, tv_nsec);
> +
> + a.tv_sec = (time_t)0x7000123470005678LL;
> + a.tv_nsec = 1;
> + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> + ZUC_ASSERT_EQ(get_tv_sec_hi(&a), tv_sec_hi);
> + ZUC_ASSERT_EQ(0x70005678, tv_sec_lo);
> + ZUC_ASSERT_EQ(1, tv_nsec);
> +
> + a.tv_sec = (time_t)0x8000123480005678LL;
> + a.tv_nsec = -1;
> + timespec_to_proto(&a, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> + ZUC_ASSERT_EQ(get_tv_sec_hi(&a), tv_sec_hi);
> + ZUC_ASSERT_EQ(0x80005678, tv_sec_lo);
> + ZUC_ASSERT_EQ((uint32_t)-1, tv_nsec);
> +}
> +
> ZUC_TEST(timespec_test, millihz_to_nsec)
> {
> ZUC_ASSERT_EQ(millihz_to_nsec(60000), 16666666);
> @@ -302,6 +351,23 @@ ZUC_TEST(timespec_test, timespec_from_msec)
> ZUC_ASSERT_EQ(1000000, a.tv_nsec);
> }
>
> +ZUC_TEST(timespec_test, timespec_from_proto)
> +{
> + struct timespec a;
> +
> + timespec_from_proto(&a, 0, 0, 0);
> + ZUC_ASSERT_EQ(0, a.tv_sec);
> + ZUC_ASSERT_EQ(0, a.tv_nsec);
> +
> + timespec_from_proto(&a, 0, 1234, 9999);
> + ZUC_ASSERT_EQ(1234, a.tv_sec);
> + ZUC_ASSERT_EQ(9999, a.tv_nsec);
> +
> + timespec_from_proto(&a, 0x1234, 0x5678, 1);
> + ZUC_ASSERT_EQ(0x0000123400005678LL, a.tv_sec);
> + ZUC_ASSERT_EQ(1, a.tv_nsec);
> +}
> +
> ZUC_TEST(timespec_test, timespec_is_zero)
> {
> struct timespec zero = { 0 };
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171212/80626969/attachment.sig>
More information about the wayland-devel
mailing list