Re: [PATCH v6 01/14] drm/panthor: Add uAPI

[Erik Faye-Lund <erik.faye-lund@collabora.com>]

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@collabora.com> 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@arm.com/
> > > > 
> > > > ...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?