[Mesa-dev] [PATCH] gallium/os: add os_wait_until_zero

[Jose Fonseca]

As others pointed, volatile and atomic are slightly different things, 
but you have point: atomic operations should probably take volatile 
pointers as arguments.

This is what C11 did

   http://en.cppreference.com/w/c/atomic/atomic_load

so I do believe that it makes sense to update p_atomic helpers to match 
(as one day hopefully we'll replace everything with stdatomic.h)

Jose


On 26/06/15 16:33, Marek Olšák wrote:
> I expect the variable will be changed using an atomic operation by the
> CPU, or using a coherent store instruction by the GPU.
>
> If this is wrong and volatile is really required here, then
> p_atomic_read is wrong too. Should we fix it? For example:
>
> #define p_atomic_read(_v) (*(volatile int*)(_v))
>
> Then, os_wait_until_zero can use p_atomic_read.
>
> Marek
>
> On Fri, Jun 26, 2015 at 4:48 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Fri, Jun 26, 2015 at 7:05 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>> From: Marek Olšák <marek.olsak at amd.com>
>>>
>>> This will be used by radeon and amdgpu winsyses.
>>> Copied from the amdgpu winsys.
>>> ---
>>>   src/gallium/auxiliary/os/os_time.c | 36 +++++++++++++++++++++++++++++++++++-
>>>   src/gallium/auxiliary/os/os_time.h | 10 ++++++++++
>>>   2 files changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/auxiliary/os/os_time.c b/src/gallium/auxiliary/os/os_time.c
>>> index f7e4ca4..63b6879 100644
>>> --- a/src/gallium/auxiliary/os/os_time.c
>>> +++ b/src/gallium/auxiliary/os/os_time.c
>>> @@ -33,11 +33,12 @@
>>>    */
>>>
>>>
>>> -#include "pipe/p_config.h"
>>> +#include "pipe/p_defines.h"
>>>
>>>   #if defined(PIPE_OS_UNIX)
>>>   #  include <time.h> /* timeval */
>>>   #  include <sys/time.h> /* timeval */
>>> +#  include <sched.h> /* sched_yield */
>>>   #elif defined(PIPE_SUBSYSTEM_WINDOWS_USER)
>>>   #  include <windows.h>
>>>   #else
>>> @@ -92,3 +93,36 @@ os_time_sleep(int64_t usecs)
>>>   }
>>>
>>>   #endif
>>> +
>>> +
>>> +bool os_wait_until_zero(int *var, uint64_t timeout)
>>
>> Does this need to be volatile?
>>
>>> +{
>>> +   if (!*var)
>>> +      return true;
>>> +
>>> +   if (!timeout)
>>> +      return false;
>>> +
>>> +   if (timeout == PIPE_TIMEOUT_INFINITE) {
>>> +      while (*var) {
>>> +#if defined(PIPE_OS_UNIX)
>>> +         sched_yield();
>>> +#endif
>>> +      }
>>> +      return true;
>>> +   }
>>> +   else {
>>> +      int64_t start_time = os_time_get_nano();
>>> +      int64_t end_time = start_time + timeout;
>>> +
>>> +      while (*var) {
>>> +         if (os_time_timeout(start_time, end_time, os_time_get_nano()))
>>> +            return false;
>>> +
>>> +#if defined(PIPE_OS_UNIX)
>>> +         sched_yield();
>>> +#endif
>>> +      }
>>> +      return true;
>>> +   }
>>> +}
>>> diff --git a/src/gallium/auxiliary/os/os_time.h b/src/gallium/auxiliary/os/os_time.h
>>> index 4fab03c..fdc8040 100644
>>> --- a/src/gallium/auxiliary/os/os_time.h
>>> +++ b/src/gallium/auxiliary/os/os_time.h
>>> @@ -94,6 +94,16 @@ os_time_timeout(int64_t start,
>>>   }
>>>
>>>
>>> +/**
>>> + * Wait until the variable at the given memory location is zero.
>>> + *
>>> + * \param var           variable
>>> + * \param timeout       timeout, can be anything from 0 (no wait) to
>>> + *                      PIPE_TIME_INFINITE (wait forever)
>>> + * \return     true if the variable is zero
>>> + */
>>> +bool os_wait_until_zero(int *var, uint64_t timeout);
>>> +
>>>   #ifdef __cplusplus
>>>   }
>>>   #endif
>>> --
>>> 2.1.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>