* Re: [PATCH] drm/panthor: use defines for sync flags
2024-10-29 9:46 [PATCH] drm/panthor: use defines for sync flags Erik Faye-Lund
@ 2024-10-29 10:15 ` Liviu Dudau
2024-10-29 11:22 ` Erik Faye-Lund
2024-10-29 12:35 ` Mihail Atanassov
2024-10-29 13:01 ` Boris Brezillon
2 siblings, 1 reply; 5+ messages in thread
From: Liviu Dudau @ 2024-10-29 10:15 UTC (permalink / raw)
To: Erik Faye-Lund
Cc: dri-devel, Boris Brezillon, Steven Price, Robin Murphy,
Mihail Atanassov, kernel
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! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: use defines for sync flags
2024-10-29 10:15 ` Liviu Dudau
@ 2024-10-29 11:22 ` Erik Faye-Lund
0 siblings, 0 replies; 5+ messages in thread
From: Erik Faye-Lund @ 2024-10-29 11:22 UTC (permalink / raw)
To: Liviu Dudau
Cc: dri-devel, Boris Brezillon, Steven Price, Robin Murphy,
Mihail Atanassov, kernel
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
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/panthor: use defines for sync flags
2024-10-29 9:46 [PATCH] drm/panthor: use defines for sync flags Erik Faye-Lund
2024-10-29 10:15 ` Liviu Dudau
@ 2024-10-29 12:35 ` Mihail Atanassov
2024-10-29 13:01 ` Boris Brezillon
2 siblings, 0 replies; 5+ messages in thread
From: Mihail Atanassov @ 2024-10-29 12:35 UTC (permalink / raw)
To: Erik Faye-Lund, dri-devel
Cc: nd, Boris Brezillon, Steven Price, Liviu Dudau, Robin Murphy,
kernel
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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: use defines for sync flags
2024-10-29 9:46 [PATCH] drm/panthor: use defines for sync flags Erik Faye-Lund
2024-10-29 10:15 ` Liviu Dudau
2024-10-29 12:35 ` Mihail Atanassov
@ 2024-10-29 13:01 ` Boris Brezillon
2 siblings, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2024-10-29 13:01 UTC (permalink / raw)
To: Erik Faye-Lund
Cc: dri-devel, Steven Price, Liviu Dudau, Robin Murphy,
Mihail Atanassov, kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread