| Message ID | 20230208194817.199932-6-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sched: Create wrapper to add a syncobj dependency to job | expand |
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@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. Melissa > } > > static int > -- > 2.39.1 >
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@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. Regards, Christian. > > Melissa > >> } >> >> static int >> -- >> 2.39.1 >>
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@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 > > > >
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@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 >>>>
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); } static int
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@igalia.com> --- drivers/gpu/drm/v3d/v3d_gem.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)