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

[Mihail Atanassov]

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 at 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.

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 at arm.com>

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.