On Tue, Jun 19, 2018 at 5:36 PM, Keith Packard <keithp@xxxxxxxxxx> wrote:
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) It won't not get implicitly casted to signed; the implicit cast works
>> +{
>> + 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.
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.
This is why I insert casts like mad in these scenarios. :-)
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;
I suppose we probably shouldn't worry about current_time being greater than INT64_MAX? I guess if that happens we have pretty big problems...
timeout = MIN2(max_timeout, timeout);
return (current_time + timeout);
}
Yeah, I think that's better.
--Jason
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
