Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes: > You can have a full reviewed-by You're too kind :-) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
On June 19, 2018 22:16:37 "Keith Packard" wrote: Jason Ekstrand writes: I don't think I have any more comments on this patch. It's gross but I think it should work. I'll mark you down as 'Acked-by' then. Neither of us loves the implementation; I'll see about creating the kernel infrastructure necessary to supplant it. You can have a full reviewed-by ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes: > I don't think I have any more comments on this patch. It's gross but I > think it should work. I'll mark you down as 'Acked-by' then. Neither of us loves the implementation; I'll see about creating the kernel infrastructure necessary to supplant it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
On Tue, Jun 19, 2018 at 6:22 PM, Keith Packard wrote: > Jason Ekstrand writes: > > > 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... > > Nope, we've already given up on that working -- it's a couple hundred > years of system uptime. Neither of us have any concerns in this area. > > >>timeout = MIN2(max_timeout, timeout); > >> > >>return (current_time + timeout); > >> } > >> > > > > Yeah, I think that's better. > > Cool. I'll wait for further review :-) > I don't think I have any more comments on this patch. It's gross but I think it should work. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes: > 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... Nope, we've already given up on that working -- it's a couple hundred years of system uptime. Neither of us have any concerns in this area. >>timeout = MIN2(max_timeout, timeout); >> >>return (current_time + timeout); >> } >> > > Yeah, I think that's better. Cool. I'll wait for further review :-) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
On Tue, Jun 19, 2018 at 5:36 PM, Keith Packard wrote: > Jason Ekstrand 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 OP implicitly casts the > 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
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand 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 OP implicitly casts the 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 signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
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. In the mean time, this is probably fine. I did see one issue below with time conversions that should be fixed though. On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard wrote: > Handle the case where the set of fences to wait for is not all of the > same type by either waiting for them sequentially (waitAll), or > polling them until the timer has expired (!waitAll). We hope the > latter case is not common. > > While the current code makes sure that it always has fences of only > one type, that will not be true when we add WSI fences. Split out this > refactoring to make merging that clearer. > > v2: Adopt Jason Ekstrand's coding conventions > > Declare variables at first use, eliminate extra whitespace between > types and names. Wrap lines to 80 columns. > > Suggested-by: Jason Ekstrand > > Signed-off-by: Keith Packard > --- > src/intel/vulkan/anv_queue.c | 105 +-- > 1 file changed, 88 insertions(+), 17 deletions(-) > > diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c > index 6fe04a0a19d..8df99c84549 100644 > --- a/src/intel/vulkan/anv_queue.c > +++ b/src/intel/vulkan/anv_queue.c > @@ -459,12 +459,32 @@ gettime_ns(void) > return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec; > } > > +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. 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. That said, I think I just saw a bug in the old code which I'll point out below. > + > +static int64_t anv_get_relative_timeout(uint64_t abs_timeout) > +{ > + uint64_t now = gettime_ns(); > + > + if (abs_timeout < now) > + return 0; > + return abs_timeout - now; > +} > + > static VkResult > anv_wait_for_syncobj_fences(struct anv_device *device, > uint32_t fenceCount, > const VkFence *pFences, > bool waitAll, > -uint64_t _timeout) > +uint64_t abs_timeout_ns) > { > uint32_t *syncobjs = vk_zalloc(&device->alloc, >sizeof(*syncobjs) * fenceCount, 8, > @@ -484,19 +504,6 @@ anv_wait_for_syncobj_fences(struct anv_device > *device, >syncobjs[i] = impl->syncobj; > } > > - int64_t abs_timeout_ns = 0; > - if (_timeout > 0) { > - uint64_t current_ns = gettime_ns(); > - > - /* Add but saturate to INT32_MAX */ > - if (current_ns + _timeout < current_ns) > - abs_timeout_ns = 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. > - abs_timeout_ns = INT64_MAX; > - else > - abs_timeout_ns = current_ns + _timeout; > - } > - > /* The gem_syncobj_wait ioctl may return early due to an inherent > * limitation in the way it computes timeouts. Loop until we've > actually > * passed the timeout. > @@ -665,6 +672,67 @@ done: > return result; > } > > +static VkResult > +anv_wait_for_fences(struct anv_device *device, > +uint32_t fenceCount, > +const VkFence *pFences, > +bool waitAll, > +uint64_t abs_timeout) > +{ > + VkResult result = VK_SUCCESS; > + > + if (fenceCount <= 1 || waitAll) { > + for (uint32_t i = 0; i < fenceCount; i++) { > + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]); > + switch (fence->permanent.type) { > + case ANV_FENCE_TYPE_BO: > +result = anv_wait_for_bo_fences( > + device, 1, &pFences[i], true, > + anv_get_relative_timeout(abs_timeout)); > +break; > + case ANV_FENCE_TYPE_SYNCOBJ: > +result = anv_wait_for_syncobj_fences(device, 1, &pFences[i], > + true, abs_timeout); > +break; > + case ANV_FENCE_TYPE_NONE: > +result = VK_SUCCESS; > +