| Message ID | 20231127185723.10348-12-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/v3d: Introduce CPU jobs | expand |
El lun, 27-11-2023 a las 15:48 -0300, Maíra Canal escribió: > From: Melissa Wen <mwen@igalia.com> > > Detach CSD job setup from CSD submission ioctl to reuse it in CPU > submission ioctl for indirect CSD job. > > Signed-off-by: Melissa Wen <mwen@igalia.com> > Co-developed-by: Maíra Canal <mcanal@igalia.com> > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > 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(&v3d->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 *)&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) > - goto fail; > - > - ret = v3d_job_allocate((void *)&clean_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, &acquire_ctx); > + ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args, > + &job, &clean_job, &se, > + &acquire_ctx); > if (ret) > goto fail; >
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 <mwen@igalia.com> >> >> Detach CSD job setup from CSD submission ioctl to reuse it in CPU >> submission ioctl for indirect CSD job. >> >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> Co-developed-by: Maíra Canal <mcanal@igalia.com> >> Signed-off-by: Maíra Canal <mcanal@igalia.com> >> --- >> 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(&v3d->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, &clean_job, &se, 46 &acquire_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(&v3d->sched_lock); 32 v3d_push_job(&job->base); 31 30 ret = drm_sched_job_add_dependency(&clean_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(&v3d->sched_lock); 23 22 v3d_attach_fences_and_unlock_reservation(file_priv, 21 clean_job, 20 &acquire_ctx, 19 args->out_sync, 18 &se, 17 clean_job->done_fence); 16 15 v3d_job_put(&job->base); 14 v3d_job_put(clean_job); 13 12 return 0; 11 10 fail_unreserve: 9 mutex_unlock(&v3d->sched_lock); 8 fail_perfmon: 7 drm_gem_unlock_reservations(clean_job->bo, clean_job->bo_count, 6 &acquire_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(&se); 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 *)&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) >> - goto fail; >> - >> - ret = v3d_job_allocate((void *)&clean_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, &acquire_ctx); >> + ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args, >> + &job, &clean_job, &se, >> + &acquire_ctx); >> if (ret) >> goto fail; >> >
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 <mwen@igalia.com> > > > > > > Detach CSD job setup from CSD submission ioctl to reuse it in CPU > > > submission ioctl for indirect CSD job. > > > > > > Signed-off-by: Melissa Wen <mwen@igalia.com> > > > Co-developed-by: Maíra Canal <mcanal@igalia.com> > > > Signed-off-by: Maíra Canal <mcanal@igalia.com> > > > --- > > > 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(&v3d->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, &clean_job, &se, > 46 &acquire_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(&v3d->sched_lock); > 32 v3d_push_job(&job->base); > 31 > 30 ret = drm_sched_job_add_dependency(&clean_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(&v3d->sched_lock); > 23 > 22 v3d_attach_fences_and_unlock_reservation(file_priv, > 21 clean_job, > 20 &acquire_ctx, > 19 args- > >out_sync, > 18 &se, > 17 > clean_job->done_fence); > 16 > 15 v3d_job_put(&job->base); > 14 v3d_job_put(clean_job); > 13 > 12 return 0; > 11 > 10 fail_unreserve: > 9 mutex_unlock(&v3d->sched_lock); > 8 fail_perfmon: > 7 drm_gem_unlock_reservations(clean_job->bo, > clean_job->bo_count, > 6 &acquire_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(&se); > 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 *)&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) > > > - goto fail; > > > - > > > - ret = v3d_job_allocate((void *)&clean_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, &acquire_ctx); > > > + ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args, > > > + &job, &clean_job, &se, > > > + &acquire_ctx); > > > if (ret) > > > goto fail; > > > > > >
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(&v3d->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 *)&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) - goto fail; - - ret = v3d_job_allocate((void *)&clean_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, &acquire_ctx); + ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args, + &job, &clean_job, &se, + &acquire_ctx); if (ret) goto fail;