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@xxxxxxxxxx>
> > > ---
> > > 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
> > >
>
Attachment:
signature.asc
Description: PGP signature
