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