| Message ID | 20241029094629.1019295-1-erik.faye-lund@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: use defines for sync flags | expand |
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 >
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 > > >
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.
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.
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.