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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 17 Oct 2024 10:51:32 +0200
Erik Faye-Lund <erik.faye-lund@collabora.com> wrote:

> 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?

I'm leaning towards defines, because 64-bit enums are uncommon
(FWIW, 'git grep "1ll << 31" include/uapi' returns nothing).