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

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

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.