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

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

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.

Melissa

> 
> Regards,
> Christian.
> 
> > 
> > Melissa
> > 
> > >   }
> > >   static int
> > > -- 
> > > 2.39.1
> > > 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230209/b7c52a91/attachment-0001.sig>