[Mesa-dev] [PATCH mesa] egl: add helper to combine two u32 into one u64

[Emil Velikov]

On 18 August 2018 at 13:57, Eric Engestrom <eric.engestrom at intel.com> wrote:
> The original issue spotted was an upcast done after a bitwise ops, but
> since the same logic is done in multiple place, Emil suggested adding
> a helper to avoid mistakes.
>
Thank you.

> Signed-off-by: Eric Engestrom <eric.engestrom at intel.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.c         | 4 ++--
>  src/egl/drivers/dri2/egl_dri2.h         | 6 ++++++
>  src/egl/drivers/dri2/platform_wayland.c | 6 ++----
>  src/egl/drivers/dri2/platform_x11.c     | 2 +-
>  4 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 1208ebb31568991e532d..ab4f134b7de71295ef9c 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -2424,8 +2424,8 @@ dri2_create_image_dma_buf(_EGLDisplay *disp, _EGLContext *ctx,
>      * will be present in attrs.DMABufPlaneModifiersLo[0] and
>      * attrs.DMABufPlaneModifiersHi[0] */
>     if (attrs.DMABufPlaneModifiersLo[0].IsPresent) {
> -      modifier = (uint64_t) attrs.DMABufPlaneModifiersHi[0].Value << 32;
> -      modifier |= (uint64_t) (attrs.DMABufPlaneModifiersLo[0].Value & 0xffffffff);
> +      modifier = combine_u32_into_u64(attrs.DMABufPlaneModifiersHi[0].Value,
> +                                      attrs.DMABufPlaneModifiersLo[0].Value);
>        has_modifier = true;
>     }
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index a6588632f776de58df48..955725658239e99d8ebe 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -528,4 +528,10 @@ dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
>  void
>  dri2_fini_surface(_EGLSurface *surf);
>
> +static inline uint64_t
> +combine_u32_into_u64(uint32_t hi, uint32_t lo)
> +{
> +   return (((uint64_t) hi) << 32) | (((uint64_t) lo) & 0xffffffff);
> +}
> +
>  #endif /* EGL_DRI2_INCLUDED */
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index 43cf00b8ac05aaefb2ec..11c57de0f8906455cc41 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -818,8 +818,7 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
>                                             __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
>                                             &mod_lo);
>        if (query) {
> -         modifier = (uint64_t) mod_hi << 32;
> -         modifier |= (uint64_t) (mod_lo & 0xffffffff);
> +         modifier = combine_u32_into_u64(mod_hi, mod_lo);
>        }
>     }
>
> @@ -1192,8 +1191,7 @@ dmabuf_handle_modifier(void *data, struct zwp_linux_dmabuf_v1 *dmabuf,
>     dri2_dpy->formats |= (1u << visual_idx);
>
>     mod = u_vector_add(&dri2_dpy->wl_modifiers[visual_idx]);
> -   *mod = (uint64_t) modifier_hi << 32;
> -   *mod |= (uint64_t) (modifier_lo & 0xffffffff);
> +   *mod = combine_u32_into_u64(modifier_hi, modifier_lo);
>  }
>
>  static const struct zwp_linux_dmabuf_v1_listener dmabuf_listener = {
> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
> index cc912d2b71f58558f454..c525b58341159ae4e480 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -915,7 +915,7 @@ dri2_x11_swap_buffers_msc(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw,
>        reply = xcb_dri2_swap_buffers_reply(dri2_dpy->conn, cookie, NULL);
>
>        if (reply) {
> -         swap_count = (((int64_t)reply->swap_hi) << 32) | reply->swap_lo;
> +         swap_count = combine_u32_into_u64(reply->swap_hi, reply->swap_lo);

These here are signed. Looking at the way it's used - it's safe.
Personally, I'd drop this hunk and simplify/fixup the function at some
random future point.

Either way
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil