Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]

["Keith Packard" <keithp@keithp.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3195 bytes --]

Jason Ekstrand <jason@jlekstrand.net> 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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel