[PATCH] drm/syncobj: return meaningful value to user space

[zhoucm1]

On 2019年07月22日 16:46, Lionel Landwerlin wrote:
> On 18/07/2019 14:13, Chunming Zhou wrote:
>> if WAIT_FOR_SUBMIT isn't set and in the meanwhile no underlying fence 
>> on syncobj,
>> then return non-block error code to user sapce.
>>
>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>> b/drivers/gpu/drm/drm_syncobj.c
>> index 361a01a08c18..929f7c64f9a2 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -252,7 +252,7 @@ int drm_syncobj_find_fence(struct drm_file 
>> *file_private,
>>               return 0;
>>           dma_fence_put(*fence);
>>       } else {
>> -        ret = -EINVAL;
>> +        ret = -ENOTBLK;
>
>
> This will only return the new error when there is no chain fence in 
> the syncobj?
If all of you agree, that's best.
I've checked orginal EINVAL，there are 3 situations which would return 
EINVAL:
a. invalid flags
b. count_handles
c. failed to find fence in syncobj.

If user can make sure sanitization for paramters, then EINVAL can be 
used to identify "lack of fence in syncobj", which is waitBeforeSignal. 
I use it in my current implementation.

>
> Don't you want the new error code after dma_fence_chain_find_seqno() too?
No, I don't want to that, I just want to a meaningful and unique error 
code for umd.

>
>
> Which make me realize there is probably a bug with this code :
>
>
> ret = dma_fence_chain_find_seqno(fence, point);
> if (!ret)
>         return 0;
> dma_fence_put(*fence);
>
>
> Sounds like the condition should be
>
> if (ret)
>
>         return ret;
>
>
> I realize we have introduced a blocking behavior on the transfer ioctl.
>
> If we're going to change this to return EWOULDBLOCK, we might want to 
> get rid of it.
Sounds right, but I think current implementation is acceptable as well.

-David
>
>
> -Lionel
>
>
>>       }
>>         if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
>> @@ -832,7 +832,7 @@ static signed long 
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>               if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>                   continue;
>>               } else {
>> -                timeout = -EINVAL;
>> +                timeout = -ENOTBLK;
>>                   goto cleanup_entries;
>>               }
>>           }
>
>