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

[Lionel Landwerlin]

On 18/07/2019 17:33, Chunming Zhou wrote:
> 在 2019/7/18 22:08, Lionel Landwerlin 写道:
>> On 18/07/2019 16:02, Chunming Zhou wrote:
>>> 在 2019/7/18 19:31, Lionel Landwerlin 写道:
>>>> 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;
>>>>>         }
>>>>>           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;
>>>>>                 }
>>>>>             }
>>>> This would break existing tests for binary syncobjs.
>>> How does this breaks binary syncobj?
>>
>> This is used in the submission path of several drivers.
>>
>> Changing the error code will change what the drivers are reporting to
>> userspace and could break tests.
>>
>>
>> i915 doesn't use that function so it's not affected but
>> lima/panfrost/vc4 seem to be.
>
> anyone from vc4 can confirm this? There are many place in wait_ioctl
> being able to return previous EINVAL, not sure what they use to.
>
>
>>
>>>
>>>> Is this really what we want?
>>> I want to use this meaningful return value to judge if WaitBeforeSignal
>>> happens.
>>>
>>> I think this is the cheapest change for that.
>>
>> I thought the plan was to add a new ioctl to query the last submitted
>> value.
> Yes, that is optional way too.  I just thought changing return value is
> very cheap, isn't it?
>
>
> -David


I could be misguided but I thought the kernel policy was to never break 
userspace.

I'm not sure where this sits :/


-Lionel


>
>> Did I misunderstand?
>>
>>
>> Thanks,
>>
>>
>> -Lionel
>>
>>
>>> -David
>>>
>>>
>>>> -Lionel
>>>>
>>>>