Hi Erik,
On 17/10/2024 09:51, Erik Faye-Lund 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_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.
That's only true for enum classes. Plain'ol enums' values can be ORed at
will (but you will need to `static_cast` them back to the enum type,
admittedly, because they auto-"promote" to int for the arithmetic op).
I've had to use uAPI from C++ and the most painless approach, once you
finish writing it, is to wrap the whole uAPI in C++ constructs anyway.
So I wouldn't consider that angle, personally.
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?
--
Mihail Atanassov <mihail.atanassov@xxxxxxx>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
