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

[zhoucm1]

On 2019年07月19日 16:13, Lionel Landwerlin wrote:
> 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.
But no one exactly points how to break userspace, doesn't it?

-David
>
> I'm not sure where this sits :/
>
>
> -Lionel
>
>
>>
>>> Did I misunderstand?
>>>
>>>
>>> Thanks,
>>>
>>>
>>> -Lionel
>>>
>>>
>>>> -David
>>>>
>>>>
>>>>> -Lionel
>>>>>
>>>>>
>