Re: [PATCH] drm/panthor: use defines for sync flags

[Boris Brezillon <boris.brezillon@collabora.com>]

On Tue, 29 Oct 2024 10:46:29 +0100
Erik Faye-Lund <erik.faye-lund@collabora.com> wrote:

> Enums are always signed, and assigning 1u << 31 to it invokes
> implementation defined behavior. It's not a great idea to depend on this
> in the UAPI, and it turns out no other UAPI does either.
> 
> So let's do what other UAPI does, and use defines instead. This way we
> won't get unexpected issues if compiling user-space with a compiler with
> a different implementation-defined behavior here.

Can we do the same for all flag definitions in this header
(drm_panthor_vm_bind_op_flags, drm_panthor_vm_bind_flags,
drm_panthor_bo_flags and drm_panthor_group_state_flags) to keep things
consistent, and avoid the same situation when we reach the last bit on
those too?

> ---
>  include/uapi/drm/panthor_drm.h | 44 +++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index 87c9cb555dd1d..a2e348f901376 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -209,27 +209,39 @@ struct drm_panthor_obj_array {
>  	{ .stride = sizeof((ptr)[0]), .count = (cnt), .array = (__u64)(uintptr_t)(ptr) }
>  
>  /**
> - * enum drm_panthor_sync_op_flags - Synchronization operation flags.
> + * DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_MASK
> + *
> + * Synchronization handle type mask.
>   */
> -enum drm_panthor_sync_op_flags {
> -	/** @DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_MASK: Synchronization handle type mask. */
> -	DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_MASK = 0xff,
> +#define DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_MASK              0xff
>  
> -	/** @DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_SYNCOBJ: Synchronization object type. */
> -	DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_SYNCOBJ = 0,
> +/**
> + * DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_SYNCOBJ
> + *
> + * Synchronization object type.
> + */
> +#define DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_SYNCOBJ           0
>  
> -	/**
> -	 * @DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_TIMELINE_SYNCOBJ: Timeline synchronization
> -	 * object type.
> -	 */
> -	DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_TIMELINE_SYNCOBJ = 1,
> +/**
> + * DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_TIMELINE_SYNCOBJ
> + *
> + * Timeline synchronization object type.
> + */
> +#define DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_TIMELINE_SYNCOBJ  1
>  
> -	/** @DRM_PANTHOR_SYNC_OP_WAIT: Wait operation. */
> -	DRM_PANTHOR_SYNC_OP_WAIT = 0 << 31,
> +/**
> + * DRM_PANTHOR_SYNC_OP_WAIT
> + *
> + * Wait operation.
> + */
> +#define DRM_PANTHOR_SYNC_OP_WAIT    (0 << 31)
>  
> -	/** @DRM_PANTHOR_SYNC_OP_SIGNAL: Signal operation. */
> -	DRM_PANTHOR_SYNC_OP_SIGNAL = (int)(1u << 31),
> -};
> +/**
> + * DRM_PANTHOR_SYNC_OP_SIGNAL
> + *
> + * Signal operation.
> + */
> +#define DRM_PANTHOR_SYNC_OP_SIGNAL  (1u << 31)
>  
>  /**
>   * struct drm_panthor_sync_op - Synchronization operation.