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

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

[Chunming Zhou]

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@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;
 			}
 		}
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Chunming Zhou (2019-07-18 12:13:39)
> 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.

Block device required?

I presume you tried the EWOULDBLOCK which would be implied by your
sentence and found that would be an interesting experience.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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.

Is this really what we want?


-Lionel


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chunming Zhou]

在 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@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?


>
> 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.

-David


>
>
> -Lionel
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chunming Zhou]

在 2019/7/18 19:18, Chris Wilson 写道:
> Quoting Chunming Zhou (2019-07-18 12:13:39)
>> 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.
> Block device required?

Yes, if WAIT_FOR_SUBMIT is set, then that will block device.


-David

>
> I presume you tried the EWOULDBLOCK which would be implied by your
> sentence and found that would be an interesting experience.
> -Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Chunming Zhou (2019-07-18 14:04:09)
> 
> 在 2019/7/18 19:18, Chris Wilson 写道:
> > Quoting Chunming Zhou (2019-07-18 12:13:39)
> >> 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.
> > Block device required?
> 
> Yes, if WAIT_FOR_SUBMIT is set, then that will block device.

No, the error message is that it requires a "block device", not that it
will block the device, which is EWOULDBLOCK.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chunming Zhou]

在 2019/7/18 21:10, Chris Wilson 写道:
> Quoting Chunming Zhou (2019-07-18 14:04:09)
>> 在 2019/7/18 19:18, Chris Wilson 写道:
>>> Quoting Chunming Zhou (2019-07-18 12:13:39)
>>>> 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.
>>> Block device required?
>> Yes, if WAIT_FOR_SUBMIT is set, then that will block device.
> No, the error message is that it requires a "block device", not that it
> will block the device, which is EWOULDBLOCK.

OK, I got your mean.

Any other possile value suggestted?

-David

> -Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Chunming Zhou (2019-07-18 14:15:49)
> 
> 在 2019/7/18 21:10, Chris Wilson 写道:
> > Quoting Chunming Zhou (2019-07-18 14:04:09)
> >> 在 2019/7/18 19:18, Chris Wilson 写道:
> >>> Quoting Chunming Zhou (2019-07-18 12:13:39)
> >>>> 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.
> >>> Block device required?
> >> Yes, if WAIT_FOR_SUBMIT is set, then that will block device.
> > No, the error message is that it requires a "block device", not that it
> > will block the device, which is EWOULDBLOCK.
> 
> OK, I got your mean.
> 
> Any other possile value suggestted?

ENXIO -- for the non-existent "address" along the timeline.
EOPNOTSUPP -- also makes sense, but we've started to use that for
interface not supported, so would be a bit inconsistent across drm.

Or ENOTBLK, being very clear in the commit message how it doesn't reflect
the original meaning (as would be given by strerror()) and why the
seemingly more natural EWOULDBLOCK doesn't work for drm in this case,
and what use case that needs to distinguish this particular case (i.e.
why EINVAL isn't good enough).
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chunming Zhou]

在 2019/7/18 21:15, Zhou, David(ChunMing) 写道:
> 在 2019/7/18 21:10, Chris Wilson 写道:
>> Quoting Chunming Zhou (2019-07-18 14:04:09)
>>> 在 2019/7/18 19:18, Chris Wilson 写道:
>>>> Quoting Chunming Zhou (2019-07-18 12:13:39)
>>>>> 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.
>>>> Block device required?
>>> Yes, if WAIT_FOR_SUBMIT is set, then that will block device.
>> No, the error message is that it requires a "block device", not that it
>> will block the device, which is EWOULDBLOCK.

Ah, I think I don't want EWOULDBLOCK, which will try again and again 
ioctl, that is not my movitation.

I just need a meaningful value to identify the underlying fence isn't 
ready on syncobj.

Because it is in 'else' case, ENOTBLK is correct to say here need block 
but WAIT_FOR_SUBMIT isn't set.


-David

> OK, I got your mean.
>
> Any other possile value suggestted?
>
> -David
>
>> -Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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.


>
>
>> 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.

Did I misunderstand?


Thanks,


-Lionel


>
> -David
>
>
>>
>> -Lionel
>>
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chunming Zhou]

在 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@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

>
> Did I misunderstand?
>
>
> Thanks,
>
>
> -Lionel
>
>
>>
>> -David
>>
>>
>>>
>>> -Lionel
>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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
>>>>
>>>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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
>>>>>
>>>>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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?

Don't you want the new error code after dma_fence_chain_find_seqno() too?


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.


-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;
>   			}
>   		}


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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;
>>               }
>>           }
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel