On Tue, Jun 19, 2018 at 5:36 PM, Keith Packard <kei...@keithp.com> wrote:
> Jason Ekstrand <ja...@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.
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
