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

[Jason Ekstrand]

On Tue, Jun 19, 2018 at 5:36 PM, Keith Packard <keithp at keithp.com> wrote:

> Jason Ekstrand <jason at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180619/636130aa/attachment.html>