On Thu, 2024-10-17 at 12:08 +0200, Boris Brezillon wrote:
> On Thu, 17 Oct 2024 10:51:32 +0200
> Erik Faye-Lund <erik.faye-lund@xxxxxxxxxxxxx> 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@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_SYNCO
> > > > > > > > BJ =
> > > > > > > > 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?
>
> I'm leaning towards defines, because 64-bit enums are uncommon
> (FWIW, 'git grep "1ll << 31" include/uapi' returns nothing).
Agreed. Doing some grepping, it seems we're the *only* ones using the
current cast-through-int trickery. And there's *one* other driver using
the 1 << 31 with an enum (G2D_BUF_USERPTR in exynos_drm.h), which is
what was supposedly a problem with the original code.
All other 1 << 31 variants in the uapi folder are defines.
I'll type up a patch when I have some time.
