Re: [PATCH v3 10/17] drm/v3d: Detach the CSD job BO setup
El mar, 28-11-2023 a las 07:47 -0300, Maira Canal escribió:
> Hi Iago,
>
> On 11/28/23 05:42, Iago Toral wrote:
> > El lun, 27-11-2023 a las 15:48 -0300, Maíra Canal escribió:
> > > From: Melissa Wen
> > >
> > > Detach CSD job setup from CSD submission ioctl to reuse it in CPU
> > > submission ioctl for indirect CSD job.
> > >
> > > Signed-off-by: Melissa Wen
> > > Co-developed-by: Maíra Canal
> > > Signed-off-by: Maíra Canal
> > > ---
> > > drivers/gpu/drm/v3d/v3d_submit.c | 68 -
> > > -
> > > --
> > > 1 file changed, 42 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
> > > b/drivers/gpu/drm/v3d/v3d_submit.c
> > > index c134b113b181..eb26fe1e27e3 100644
> > > --- a/drivers/gpu/drm/v3d/v3d_submit.c
> > > +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> > > @@ -256,6 +256,45 @@
> > > v3d_attach_fences_and_unlock_reservation(struct
> > > drm_file *file_priv,
> > > }
> > > }
> > >
> > > +static int
> > > +v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
> > > + struct v3d_dev *v3d,
> > > + struct drm_v3d_submit_csd *args,
> > > + struct v3d_csd_job **job,
> > > + struct v3d_job **clean_job,
> > > + struct v3d_submit_ext *se,
> > > + struct ww_acquire_ctx *acquire_ctx)
> > > +{
> > > + int ret;
> > > +
> > > + ret = v3d_job_allocate((void *)job, sizeof(**job));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = v3d_job_init(v3d, file_priv, &(*job)->base,
> > > + v3d_job_free, args->in_sync, se,
> > > V3D_CSD);
> > > + if (ret)
> >
> >
> > We should free the job here.
> >
> > > + return ret;
> > > +
> > > + ret = v3d_job_allocate((void *)clean_job,
> > > sizeof(**clean_job));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = v3d_job_init(v3d, file_priv, *clean_job,
> > > + v3d_job_free, 0, NULL,
> > > V3D_CACHE_CLEAN);
> > > + if (ret)
> >
> > We should free job and clean_job here.
> >
> > > + return ret;
> > > +
> > > + (*job)->args = *args;
> > > +
> > > + ret = v3d_lookup_bos(>drm, file_priv, *clean_job,
> > > + args->bo_handles, args-
> > > > bo_handle_count);
> > > + if (ret)
> >
> > Same here.
> >
> > I think we probably want to have a fail label where we do this and
> > just
> > jump there from all the paths I mentioned above.
>
> Actually, we are freeing the job in `v3d_submit_csd_ioctl`. Take a
> look
> here:
>
> 48 ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
> 47 , _job, ,
> 46 _ctx);
> 45 if (ret)
> 44 goto fail;
>
> If `v3d_setup_csd_jobs_and_bos` fails, we go to fail.
>
> 43
> 42 if (args->perfmon_id) {
> 41 job->base.perfmon = v3d_perfmon_find(v3d_priv,
> 40
> args->perfmon_id);
> 39 if (!job->base.perfmon) {
> 38 ret = -ENOENT;
> 37 goto fail_perfmon;
> 36 }
> 35 }
> 34
> 33 mutex_lock(>sched_lock);
> 32 v3d_push_job(>base);
> 31
> 30 ret = drm_sched_job_add_dependency(_job->base,
> 29
> dma_fence_get(job->base.done_fence));
> 28 if (ret)
> 27 goto fail_unreserve;
> 26
> 25 v3d_push_job(clean_job);
> 24 mutex_unlock(>sched_lock);
> 23
> 22 v3d_attach_fences_and_unlock_reservation(file_priv,
> 21 clean_job,
> 20 _ctx,
> 19 args-
> >out_sync,
> 18 ,
> 17
> clean_job->done_fence);
> 16
> 15 v3d_job_put(>base);
> 14 v3d_job_put(clean_job);
> 13
> 12 return 0;
> 11
> 10 fail_unreserve:
> 9 mutex_unlock(>sched_lock);
> 8 fail_perfmon:
> 7 drm_gem_unlock_reservations(clean_job->bo,
> clean_job->bo_count,
> 6 _ctx);
> 5 fail:
> 4 v3d_job_cleanup((void *)job);
> 3 v3d_job_cleanup(clean_job);
>
> Here we cleanup `job` and `clean_job`. This will call `v3d_job_free`
> and
> free the jobs.
Ah, yes, ignore my previous comment then.
Iago
>
> Best Regards,
> - Maíra
>
> 2 v3d_put_multisync_post_deps();
> 1
> 1167 return ret;
>
> >
> > > + return ret;
> > > +
> > > + return v3d_lock_bo_reservations(*clean_job, acquire_ctx);
> > > +}
> > > +
> > > static void
>
Re: [PATCH v3 10/17] drm/v3d: Detach the CSD job BO setup
Hi Iago,
On 11/28/23 05:42, Iago Toral wrote:
El lun, 27-11-2023 a las 15:48 -0300, Maíra Canal escribió:
From: Melissa Wen
Detach CSD job setup from CSD submission ioctl to reuse it in CPU
submission ioctl for indirect CSD job.
Signed-off-by: Melissa Wen
Co-developed-by: Maíra Canal
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/v3d/v3d_submit.c | 68 --
--
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
b/drivers/gpu/drm/v3d/v3d_submit.c
index c134b113b181..eb26fe1e27e3 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -256,6 +256,45 @@ v3d_attach_fences_and_unlock_reservation(struct
drm_file *file_priv,
}
}
+static int
+v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
+ struct v3d_dev *v3d,
+ struct drm_v3d_submit_csd *args,
+ struct v3d_csd_job **job,
+ struct v3d_job **clean_job,
+ struct v3d_submit_ext *se,
+ struct ww_acquire_ctx *acquire_ctx)
+{
+ int ret;
+
+ ret = v3d_job_allocate((void *)job, sizeof(**job));
+ if (ret)
+ return ret;
+
+ ret = v3d_job_init(v3d, file_priv, &(*job)->base,
+ v3d_job_free, args->in_sync, se, V3D_CSD);
+ if (ret)
We should free the job here.
+ return ret;
+
+ ret = v3d_job_allocate((void *)clean_job,
sizeof(**clean_job));
+ if (ret)
+ return ret;
+
+ ret = v3d_job_init(v3d, file_priv, *clean_job,
+ v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
+ if (ret)
We should free job and clean_job here.
+ return ret;
+
+ (*job)->args = *args;
+
+ ret = v3d_lookup_bos(>drm, file_priv, *clean_job,
+ args->bo_handles, args-
bo_handle_count);
+ if (ret)
Same here.
I think we probably want to have a fail label where we do this and just
jump there from all the paths I mentioned above.
Actually, we are freeing the job in `v3d_submit_csd_ioctl`. Take a look
here:
48 ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
47 , _job, ,
46 _ctx);
45 if (ret)
44 goto fail;
If `v3d_setup_csd_jobs_and_bos` fails, we go to fail.
43
42 if (args->perfmon_id) {
41 job->base.perfmon = v3d_perfmon_find(v3d_priv,
40
args->perfmon_id);
39 if (!job->base.perfmon) {
38 ret = -ENOENT;
37 goto fail_perfmon;
36 }
35 }
34
33 mutex_lock(>sched_lock);
32 v3d_push_job(>base);
31
30 ret = drm_sched_job_add_dependency(_job->base,
29
dma_fence_get(job->base.done_fence));
28 if (ret)
27 goto fail_unreserve;
26
25 v3d_push_job(clean_job);
24 mutex_unlock(>sched_lock);
23
22 v3d_attach_fences_and_unlock_reservation(file_priv,
21 clean_job,
20 _ctx,
19 args->out_sync,
18 ,
17
clean_job->done_fence);
16
15 v3d_job_put(>base);
14 v3d_job_put(clean_job);
13
12 return 0;
11
10 fail_unreserve:
9 mutex_unlock(>sched_lock);
8 fail_perfmon:
7 drm_gem_unlock_reservations(clean_job->bo,
clean_job->bo_count,
6 _ctx);
5 fail:
4 v3d_job_cleanup((void *)job);
3 v3d_job_cleanup(clean_job);
Here we cleanup `job` and `clean_job`. This will call `v3d_job_free` and
free the jobs.
Best Regards,
- Maíra
2 v3d_put_multisync_post_deps();
1
1167 return ret;
+ return ret;
+
+ return v3d_lock_bo_reservations(*clean_job, acquire_ctx);
+}
+
static void
v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
{
@@ -700,32 +739,9 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
void *data,
}
}
- ret = v3d_job_allocate((void *), sizeof(*job));
- if (ret)
- return ret;
-
- ret = v3d_job_init(v3d, file_priv, >base,
- v3d_job_free, args->in_sync, ,
V3D_CSD);
- if (ret)
- goto fail;
-
- ret = v3d_job_allocate((void *)_job,
sizeof(*clean_job));
- if (ret)
- goto fail;
-
- ret = v3d_job_init(v3d, file_priv, clean_job,
- v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
- if (ret)
- goto fail;
-
- job->args = *args;
-
- ret =
Re: [PATCH v3 10/17] drm/v3d: Detach the CSD job BO setup
El lun, 27-11-2023 a las 15:48 -0300, Maíra Canal escribió:
> From: Melissa Wen
>
> Detach CSD job setup from CSD submission ioctl to reuse it in CPU
> submission ioctl for indirect CSD job.
>
> Signed-off-by: Melissa Wen
> Co-developed-by: Maíra Canal
> Signed-off-by: Maíra Canal
> ---
> drivers/gpu/drm/v3d/v3d_submit.c | 68 --
> --
> 1 file changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
> b/drivers/gpu/drm/v3d/v3d_submit.c
> index c134b113b181..eb26fe1e27e3 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -256,6 +256,45 @@ v3d_attach_fences_and_unlock_reservation(struct
> drm_file *file_priv,
> }
> }
>
> +static int
> +v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
> + struct v3d_dev *v3d,
> + struct drm_v3d_submit_csd *args,
> + struct v3d_csd_job **job,
> + struct v3d_job **clean_job,
> + struct v3d_submit_ext *se,
> + struct ww_acquire_ctx *acquire_ctx)
> +{
> + int ret;
> +
> + ret = v3d_job_allocate((void *)job, sizeof(**job));
> + if (ret)
> + return ret;
> +
> + ret = v3d_job_init(v3d, file_priv, &(*job)->base,
> + v3d_job_free, args->in_sync, se, V3D_CSD);
> + if (ret)
We should free the job here.
> + return ret;
> +
> + ret = v3d_job_allocate((void *)clean_job,
> sizeof(**clean_job));
> + if (ret)
> + return ret;
> +
> + ret = v3d_job_init(v3d, file_priv, *clean_job,
> + v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
> + if (ret)
We should free job and clean_job here.
> + return ret;
> +
> + (*job)->args = *args;
> +
> + ret = v3d_lookup_bos(>drm, file_priv, *clean_job,
> + args->bo_handles, args-
> >bo_handle_count);
> + if (ret)
Same here.
I think we probably want to have a fail label where we do this and just
jump there from all the paths I mentioned above.
> + return ret;
> +
> + return v3d_lock_bo_reservations(*clean_job, acquire_ctx);
> +}
> +
> static void
> v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
> {
> @@ -700,32 +739,9 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> void *data,
> }
> }
>
> - ret = v3d_job_allocate((void *), sizeof(*job));
> - if (ret)
> - return ret;
> -
> - ret = v3d_job_init(v3d, file_priv, >base,
> - v3d_job_free, args->in_sync, ,
> V3D_CSD);
> - if (ret)
> - goto fail;
> -
> - ret = v3d_job_allocate((void *)_job,
> sizeof(*clean_job));
> - if (ret)
> - goto fail;
> -
> - ret = v3d_job_init(v3d, file_priv, clean_job,
> - v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
> - if (ret)
> - goto fail;
> -
> - job->args = *args;
> -
> - ret = v3d_lookup_bos(dev, file_priv, clean_job,
> - args->bo_handles, args-
> >bo_handle_count);
> - if (ret)
> - goto fail;
> -
> - ret = v3d_lock_bo_reservations(clean_job, _ctx);
> + ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
> + , _job, ,
> + _ctx);
> if (ret)
> goto fail;
>
[PATCH v3 10/17] drm/v3d: Detach the CSD job BO setup
From: Melissa Wen
Detach CSD job setup from CSD submission ioctl to reuse it in CPU
submission ioctl for indirect CSD job.
Signed-off-by: Melissa Wen
Co-developed-by: Maíra Canal
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/v3d/v3d_submit.c | 68
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index c134b113b181..eb26fe1e27e3 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -256,6 +256,45 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file
*file_priv,
}
}
+static int
+v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
+ struct v3d_dev *v3d,
+ struct drm_v3d_submit_csd *args,
+ struct v3d_csd_job **job,
+ struct v3d_job **clean_job,
+ struct v3d_submit_ext *se,
+ struct ww_acquire_ctx *acquire_ctx)
+{
+ int ret;
+
+ ret = v3d_job_allocate((void *)job, sizeof(**job));
+ if (ret)
+ return ret;
+
+ ret = v3d_job_init(v3d, file_priv, &(*job)->base,
+ v3d_job_free, args->in_sync, se, V3D_CSD);
+ if (ret)
+ return ret;
+
+ ret = v3d_job_allocate((void *)clean_job, sizeof(**clean_job));
+ if (ret)
+ return ret;
+
+ ret = v3d_job_init(v3d, file_priv, *clean_job,
+ v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
+ if (ret)
+ return ret;
+
+ (*job)->args = *args;
+
+ ret = v3d_lookup_bos(>drm, file_priv, *clean_job,
+args->bo_handles, args->bo_handle_count);
+ if (ret)
+ return ret;
+
+ return v3d_lock_bo_reservations(*clean_job, acquire_ctx);
+}
+
static void
v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
{
@@ -700,32 +739,9 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
}
}
- ret = v3d_job_allocate((void *), sizeof(*job));
- if (ret)
- return ret;
-
- ret = v3d_job_init(v3d, file_priv, >base,
- v3d_job_free, args->in_sync, , V3D_CSD);
- if (ret)
- goto fail;
-
- ret = v3d_job_allocate((void *)_job, sizeof(*clean_job));
- if (ret)
- goto fail;
-
- ret = v3d_job_init(v3d, file_priv, clean_job,
- v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
- if (ret)
- goto fail;
-
- job->args = *args;
-
- ret = v3d_lookup_bos(dev, file_priv, clean_job,
-args->bo_handles, args->bo_handle_count);
- if (ret)
- goto fail;
-
- ret = v3d_lock_bo_reservations(clean_job, _ctx);
+ ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
+, _job, ,
+_ctx);
if (ret)
goto fail;
--
2.42.0
