[PATCH 5/5] drm/v3d: Use drm_sched_job_add_syncobj_dependency()

[Christian König]

Am 09.02.23 um 13:36 schrieb Melissa Wen:
> On 02/09, Christian König wrote:
>> Am 09.02.23 um 12:27 schrieb Melissa Wen:
>>> On 02/08, Maíra Canal wrote:
>>>> As v3d_job_add_deps() performs the same steps as
>>>> drm_sched_job_add_syncobj_dependency(), replace the open-coded
>>>> implementation in v3d in order to simply, using the DRM function.
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/v3d/v3d_gem.c | 9 +--------
>>>>    1 file changed, 1 insertion(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
>>>> index 5da1806f3969..f149526ec971 100644
>>>> --- a/drivers/gpu/drm/v3d/v3d_gem.c
>>>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>>>> @@ -400,14 +400,7 @@ static int
>>>>    v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
>>>>    		 u32 in_sync, u32 point)
>>>>    {
>>>> -	struct dma_fence *in_fence = NULL;
>>>> -	int ret;
>>>> -
>>>> -	ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence);
>>>> -	if (ret == -EINVAL)
>>>> -		return ret;
>>>> -
>>>> -	return drm_sched_job_add_dependency(&job->base, in_fence);
>>>> +	return drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, point);
>>> Hi Maíra,
>>>
>>> First, thanks for making this function a common-code.
>>>
>>> There are two issues to address before moving v3d to the new
>>> drm_sche_job_add_syncobj_dependency():
>>>
>>> 1. We don't need the v3d_job_add_deps one line function just wrapping
>>> the new drm_sched_job_add_syncobj_dependency() with the same parameters.
>>> We can just replace all occurrences of v3d function with drm_sched
>>> function. Except if we use v3d_job_add_deps to address the second issue:
>>>
>>> 2. Currently, v3d_job_add_deps returns 0 (success) if
>>> drm_syncobj_find_fence() doesn't find the syncobj from handle (-ENOENT),
>>> but drm_sched_job_add_syncobj_dependency() returns a negative value on
>>> this case. If it happens, job submissions will fail (and may cause a
>>> regression). One way to solve it is checking the return value of
>>> drm_sched_job_add_syncobj_dependency in v3d_job_add_deps.  The second
>>> way is to replace v3d_job_add_deps by
>>> drm_sched_job_add_syncobj_dependency and check expected return values
>>> there.
>> Oh, wait a second. This behavior is most likely a bug in V3D.
>>
>> When a syncobj can't find a timeline point aborting the IOCTL with -ENOENT
>> is mandatory or otherwise you run into trouble with wait before signal
>> handling for Vulkan.
>>
>> This should be common behavior for all drivers which at some point want to
>> support Vulkan.
> So, v3d doesn't support timeline syncobj yet, and I took a look at
> returning errors on drm_syncobj_find_fence, for timeline syncobj they
> can be `-ETIME` and `-ERESTARTSYS`.

In this case forget what I've said. This just doesn't apply to V3D for now.

> TBH, I don't exactly know the design reason for accepting a ENOENT from
> drm_syncobj_find_fence. I suspect it's only trying to deal with the
> `in_sync == 0` case (that would be better replaced by a `if then;
> continue;`).
>
> In any case, I think it isn't good to change this behavior in the same
> time we are moving to another function. I'd prefer to preserve the
> current behavior here and better investigate/address this issue in
> another patch.

I would just keep the current behavior with an if and add something like 
"TODO: Investigate why this was filtered out for the IOCTL".

Christian.

>
> Melissa
>
>> Regards,
>> Christian.
>>
>>> Melissa
>>>
>>>>    }
>>>>    static int
>>>> -- 
>>>> 2.39.1
>>>>