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

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

[Erik Faye-Lund]

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.
---
 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.
-- 
2.45.2

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

[Liviu Dudau]

On Tue, Oct 29, 2024 at 10:46:29AM +0100, Erik Faye-Lund 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.

You're missing the signoff.

Best regards,
Liviu

> ---
>  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.
> -- 
> 2.45.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Erik Faye-Lund]

On Tue, 2024-10-29 at 10:15 +0000, Liviu Dudau wrote:
> On Tue, Oct 29, 2024 at 10:46:29AM +0100, Erik Faye-Lund 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.
> 
> You're missing the signoff.
> 

Whoops, apologies.

Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>

I'll add it for the next iteration, if needed.


> > ---
> >  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.
> > -- 
> > 2.45.2
> > 
> 

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

[Mihail Atanassov]

Hi Erik,

On 29/10/2024 09:46, Erik Faye-Lund 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.
> ---
>   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)

[nit] 0u << 31, to have the same signedness as SYNC_OP_SIGNAL.

>   
> -	/** @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.

-- 
Mihail Atanassov <mihail.atanassov@arm.com>

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

[Boris Brezillon]

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.