[Intel-gfx] [PATCH 11/13] android/sync: Fix reversed sense of signaled fence

[Tvrtko Ursulin]

On 14/12/15 11:22, John Harrison wrote:
> On 11/12/2015 15:57, Tvrtko Ursulin wrote:
>>
>> On 11/12/15 13:11, John.C.Harrison at Intel.com wrote:
>>> From: Peter Lawthers <peter.lawthers at intel.com>
>>>
>>> In the 3.14 kernel, a signaled fence was indicated by the status field
>>> == 1. In 4.x, a status == 0 indicates signaled, status < 0 indicates
>>> error,
>>> and status > 0 indicates active.
>>>
>>> This patch wraps the check for a signaled fence in a function so that
>>> callers no longer needs to know the underlying implementation.
>>>
>>> v3: New patch for series.
>>>
>>> Change-Id: I8e565e49683e3efeb9474656cd84cf4add6ad6a2
>>> Tracked-On: https://jira01.devtools.intel.com/browse/ACD-308
>>> Signed-off-by: Peter Lawthers <peter.lawthers at intel.com>
>>> ---
>>>   drivers/android/sync.h | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/android/sync.h b/drivers/android/sync.h
>>> index d57fa0a..75532d8 100644
>>> --- a/drivers/android/sync.h
>>> +++ b/drivers/android/sync.h
>>> @@ -345,6 +345,27 @@ int sync_fence_cancel_async(struct sync_fence
>>> *fence,
>>>    */
>>>   int sync_fence_wait(struct sync_fence *fence, long timeout);
>>>
>>> +/**
>>> + * sync_fence_is_signaled() - Return an indication if the fence is
>>> signaled
>>> + * @fence:    fence to check
>>> + *
>>> + * returns 1 if fence is signaled
>>> + * returns 0 if fence is not signaled
>>> + * returns < 0 if fence is in error state
>>> + */
>>> +static inline int
>>> +sync_fence_is_signaled(struct sync_fence *fence)
>>> +{
>>> +    int status;
>>> +
>>> +    status = atomic_read(&fence->status);
>>> +    if (status == 0)
>>> +        return 1;
>>> +    if (status > 0)
>>> +        return 0;
>>> +    return status;
>>> +}
>>
>> Not so important but could simply return bool, like "return status <=
>> 0"? Since it is called "is_signaled" and it is only used in boolean
>> mode in future patches.
>
> There is no point in throwing away the error code unnecessarily. It can
> be useful in debug output and indeed will show up the scheduler status
> dump via debugfs.

You could still grab it directly from that call site. Or by adding 
another accessor like sync_fence_get_error(). Just saying that it may be 
good to decouple more from sync_fence implementation, since the 
internals have changed once already.

Regards,

Tvrtko