Jason Ekstrand <jason@xxxxxxxxxxxxxx> writes:
> I still don't really like this but I agree that this code really should not
> be getting hit very often so it's probably not too bad. Maybe one day
> we'll be able to drop the non-syncobj paths entirely. Wouldn't that be
> nice.
I agree. What I want to have is kernel-side syncobj support for all of
the WSI fences that we need here.
I thought about using syncobjs and signaling them from user-space, but
realized that we couldn't eliminate the non-syncobj path quite yet as
that requires a new enough kernel. And, if we want to get to
kernel-native WSI syncobjs, that would mean we'd eventually have three
code paths. I think it's better to have one 'legacy' path, which works
on all kernels, and then one 'modern' path which does what we want.
> In the mean time, this is probably fine. I did see one issue below
> with time conversions that should be fixed though.
Always a hard thing to get right.
>> +static uint64_t anv_get_absolute_timeout(uint64_t timeout)
>> +{
>> + if (timeout == 0)
>> + return 0;
>> + uint64_t current_time = gettime_ns();
>> +
>> + timeout = MIN2(INT64_MAX - current_time, timeout);
>> +
>> + return (current_time + timeout);
>> +}
>>
>
> This does not have the same behavior as the code it replaces. In
> particular, the old code saturates at INT64_MAX whereas this code does not
> do so properly anymore. If UINT64_MAX gets passed into timeout, I believe
> that will be implicitly casted to signed and the MIN will yield -1 which
> gets casted back to unsigned and you get UINT64_MAX again.
It won't not get implicitly casted to signed; the implicit cast works
the other way where <signed> OP <unsigned> implicitly casts the <signed>
operand to unsigned for types of the same 'rank' (where 'rank' is not
quite equivalent to size). Here's a fine article on this:
https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules
However, this is a rather obscure part of the ISO standard, and I don't
think we should expect that people reading the code know it well. Adding
a cast to make it clear what we want is a good idea. How about this?
static uint64_t anv_get_absolute_timeout(uint64_t timeout)
{
if (timeout == 0)
return 0;
uint64_t current_time = gettime_ns();
uint64_t max_timeout = (uint64_t) INT64_MAX - current_time;
timeout = MIN2(max_timeout, timeout);
return (current_time + timeout);
}
> This is a problem because the value we pass into the kernel ioctls is
> signed. The behavior of the kernel *should* be infinite when timeout
> < 0 but, on some older kernels, -1 is treated as 0 and you get no
> timeout.
Yeah, we definitely want to cap the values to INT64_MAX.
>> - else if (current_ns + _timeout > INT64_MAX)
>>
>
> I suspect we need to cast INT64_MAX to a uint64_t here to ensure we don't
> accidentally get an implicit conversion to signed.
Again, it's not necessary given the C conversion rules, but it is a good
way to clarify the intent.
--
-keith
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
