On 18 August 2018 at 13:57, Eric Engestrom <eric.engest...@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.engest...@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.veli...@collabora.com>
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
