On Wed, 2024-10-16 at 16:18 +0200, Boris Brezillon wrote:
> On Wed, 16 Oct 2024 16:05:55 +0200
> Erik Faye-Lund <erik.faye-lund@xxxxxxxxxxxxx> wrote:
>
> > On Wed, 2024-10-16 at 15:02 +0100, Robin Murphy wrote:
> > > On 2024-10-16 2:50 pm, Erik Faye-Lund wrote:
> > > > On Wed, 2024-10-16 at 15:16 +0200, Erik Faye-Lund wrote:
> > > > > On Thu, 2024-02-29 at 17:22 +0100, Boris Brezillon wrote:
> > > > > > +/**
> > > > > > + * enum drm_panthor_sync_op_flags - Synchronization
> > > > > > operation
> > > > > > flags.
> > > > > > + */
> > > > > > +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,
> > > > > > +
> > > > > > + /** @DRM_PANTHOR_SYNC_OP_HANDLE_TYPE_SYNCOBJ:
> > > > > > Synchronization object type. */
> > > > > > + 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_WAIT: Wait operation. */
> > > > > > + DRM_PANTHOR_SYNC_OP_WAIT = 0 << 31,
> > > > > > +
> > > > > > + /** @DRM_PANTHOR_SYNC_OP_SIGNAL: Signal operation.
> > > > > > */
> > > > > > + DRM_PANTHOR_SYNC_OP_SIGNAL = (int)(1u << 31),
> > > > >
> > > > > Why do we cast to int here? 1u << 31 doesn't fit in a 32-bit
> > > > > signed
> > > > > integer, so isn't this undefined behavior in C?
> > > > >
> > > >
> > > > Seems this was proposed here:
> > > > https://lore.kernel.org/dri-devel/89be8f8f-7c4e-4efd-0b7b-c30bcfbf1d23@xxxxxxx/
> > > >
> > > > ...that kinda sounds like bad advice to me.
> > > >
> > > > Also, it's been pointed out to me elsewhere that this isn't
> > > > *technically speaking* undefined, it's "implementation
> > > > defined".
> > > > But as
> > > > far as kernel interfaces goes, that's pretty much the same; we
> > > > can't
> > > > guarantee that the kernel and the user-space is using the same
> > > > implementation.
> > > >
> > > > Here's the quote from the C99 spec, section 6.3.1.3 "Signed and
> > > > unsigned integers":
> > > >
> > > > """
> > > > Otherwise, the new type is signed and the value cannot be
> > > > represented
> > > > in it; either the result is implementation-defined or an
> > > > implementation-defined signal is raised
> > > > """"
> > > >
> > > > I think a better approach be to use -1 << 31, which is well-
> > > > defined.
> > > > But the problem then becomes assigning it into
> > > > drm_panthor_sync_op::flags in a well-defined way... Could we
> > > > make
> > > > the
> > > > field signed? That seems a bit bad as well...
> > >
> > > Is that a problem? Signed->unsigned conversion is always well-
> > > defined
> > > (6.3.1.3 again), since it doesn't depend on how the signed type
> > > represents negatives.
> > >
> > > Robin.
> >
> > Ah, you're right. So that could fix the problem, indeed.
>
> On the other hand, I hate the idea of having -1 << 31 to encode
> bit31-set. That's even worse for DRM_PANTHOR_VM_BIND_OP_TYPE_xxx when
> we'll reach a value above 0x7, because then the negative value is
> hard
> to map to its unsigned representation. If we really care about this
> corner case, I'd rather go full-defines for flags and call it a day.
>
Yeah, I suppose it can get ugly for some other cases.
If we rule that out, I think there's only two options I can think of
left:
1. Using #defines instead, like Boris suggested
2. Using 64 bit signed enums (e.g "1ll << 31" instead)
Again, #2 here would be the smaller change. But I kinda think I lean
towards #1, because... These aren't really enumerators. They are flags.
...Yeah, sure. In C the practical difference isn't huge. But if we ever
wanted to support using these enums from C++ code, we'd need to add
overloaded operators, because C++ doesn't allow ORing together enums
out of the box.
I'm not saying I have any plans on using the uAPI from C++, just saying
that if we're going to tackle this, we might as well tackle it
completely...
Also, expanding the enum-type to 64 bits might have some additional
consequences, like needlessly needing more stack-space to pass values
around etc.
Thoughts? Surely there must be some precedence on using the top bit for
flags in the kernel, no?
