| Message ID | 20240710134130.17292-3-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v3d: Perfmon cleanup | expand |
On 7/10/24 10:41, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > If fetching of userspace memory fails during the main loop, all drm sync > objs looked up until that point will be leaked because of the missing > drm_syncobj_put. > > Fix it by exporting and using a common cleanup helper. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> This patch looks fine to me apart from two nits and a compilation issue. > Fixes: 9ba0ff3e083f ("drm/v3d: Create a CPU job extension for the timestamp query job") > Cc: Maíra Canal <mcanal@igalia.com> > Cc: Iago Toral Quiroga <itoral@igalia.com> > Cc: <stable@vger.kernel.org> # v6.8+ > --- > drivers/gpu/drm/v3d/v3d_drv.h | 2 ++ > drivers/gpu/drm/v3d/v3d_sched.c | 22 +++++++++++++------ > drivers/gpu/drm/v3d/v3d_submit.c | 36 ++++++++++++++++++++++---------- > 3 files changed, 43 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h > index 099b962bdfde..95651c3c926f 100644 > --- a/drivers/gpu/drm/v3d/v3d_drv.h > +++ b/drivers/gpu/drm/v3d/v3d_drv.h > @@ -563,6 +563,8 @@ void v3d_mmu_insert_ptes(struct v3d_bo *bo); > void v3d_mmu_remove_ptes(struct v3d_bo *bo); > > /* v3d_sched.c */ > +void __v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, > + unsigned int count); My two nits: I believe we never used this `__` pattern in V3D and I'm not sure how comfortable I am to introduce it now. I know it is pretty common in drivers like i915, but I wonder how much semantics we would miss to remove it. Also, any chance we could use `query_info` instead of `qinfo`? This suggestion applies to all patches in the series that uses `qinfo`. > void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); > int v3d_sched_init(struct v3d_dev *v3d); > void v3d_sched_fini(struct v3d_dev *v3d); > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > index 03df37a3acf5..e45d3ddc6f82 100644 > --- a/drivers/gpu/drm/v3d/v3d_sched.c > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > @@ -73,18 +73,28 @@ v3d_sched_job_free(struct drm_sched_job *sched_job) > v3d_job_cleanup(job); > } > > +void > +__v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, > + unsigned int count) > +{ > + if (qinfo->queries) { > + unsigned int i; > + > + for (i = 0; i < count; i++) > + drm_syncobj_put(qinfo->queries[i].syncobj); > + > + kvfree(qinfo->queries); > + } > +} > + > static void > v3d_cpu_job_free(struct drm_sched_job *sched_job) > { > struct v3d_cpu_job *job = to_cpu_job(sched_job); > - struct v3d_timestamp_query_info *timestamp_query = &job->timestamp_query; > struct v3d_performance_query_info *performance_query = &job->performance_query; > > - if (timestamp_query->queries) { > - for (int i = 0; i < timestamp_query->count; i++) > - drm_syncobj_put(timestamp_query->queries[i].syncobj); > - kvfree(timestamp_query->queries); > - } > + __v3d_timestamp_query_info_free(&job->timestamp_query, > + job->timestamp_query.count); > > if (performance_query->queries) { > for (int i = 0; i < performance_query->count; i++) > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c > index 263fefc1d04f..2818afdd4807 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c > @@ -452,6 +452,7 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, > { > u32 __user *offsets, *syncs; > struct drm_v3d_timestamp_query timestamp; > + int err; > > if (!job) { > DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); > @@ -484,15 +485,15 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, > u32 offset, sync; > > if (copy_from_user(&offset, offsets++, sizeof(offset))) { > - kvfree(job->timestamp_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->timestamp_query.queries[i].offset = offset; > > if (copy_from_user(&sync, syncs++, sizeof(sync))) { > - kvfree(job->timestamp_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > @@ -500,6 +501,10 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, > job->timestamp_query.count = timestamp.count; > > return 0; > + > +error: > + __v3d_timestamp_query_info_free(qinfo, i); I don't see where `qinfo` is declared in this function. > + return err; > } > > static int > @@ -509,6 +514,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, > { > u32 __user *syncs; > struct drm_v3d_reset_timestamp_query reset; > + int err; > > if (!job) { > DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); > @@ -539,8 +545,8 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, > job->timestamp_query.queries[i].offset = reset.offset + 8 * i; > > if (copy_from_user(&sync, syncs++, sizeof(sync))) { > - kvfree(job->timestamp_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > @@ -548,6 +554,10 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, > job->timestamp_query.count = reset.count; > > return 0; > + > +error: > + __v3d_timestamp_query_info_free(qinfo, i); Same here. > + return err; > } > > /* Get data for the copy timestamp query results job submission. */ > @@ -558,7 +568,7 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, > { > u32 __user *offsets, *syncs; > struct drm_v3d_copy_timestamp_query copy; > - int i; > + int i, err; > > if (!job) { > DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); > @@ -591,15 +601,15 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, > u32 offset, sync; > > if (copy_from_user(&offset, offsets++, sizeof(offset))) { > - kvfree(job->timestamp_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->timestamp_query.queries[i].offset = offset; > > if (copy_from_user(&sync, syncs++, sizeof(sync))) { > - kvfree(job->timestamp_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > @@ -613,6 +623,10 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, > job->copy.stride = copy.stride; > > return 0; > + > +error: > + __v3d_timestamp_query_info_free(qinfo, i); Same here. Best Regards, - Maíra > + return err; > } > > static int
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 099b962bdfde..95651c3c926f 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -563,6 +563,8 @@ void v3d_mmu_insert_ptes(struct v3d_bo *bo); void v3d_mmu_remove_ptes(struct v3d_bo *bo); /* v3d_sched.c */ +void __v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, + unsigned int count); void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); int v3d_sched_init(struct v3d_dev *v3d); void v3d_sched_fini(struct v3d_dev *v3d); diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index 03df37a3acf5..e45d3ddc6f82 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -73,18 +73,28 @@ v3d_sched_job_free(struct drm_sched_job *sched_job) v3d_job_cleanup(job); } +void +__v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, + unsigned int count) +{ + if (qinfo->queries) { + unsigned int i; + + for (i = 0; i < count; i++) + drm_syncobj_put(qinfo->queries[i].syncobj); + + kvfree(qinfo->queries); + } +} + static void v3d_cpu_job_free(struct drm_sched_job *sched_job) { struct v3d_cpu_job *job = to_cpu_job(sched_job); - struct v3d_timestamp_query_info *timestamp_query = &job->timestamp_query; struct v3d_performance_query_info *performance_query = &job->performance_query; - if (timestamp_query->queries) { - for (int i = 0; i < timestamp_query->count; i++) - drm_syncobj_put(timestamp_query->queries[i].syncobj); - kvfree(timestamp_query->queries); - } + __v3d_timestamp_query_info_free(&job->timestamp_query, + job->timestamp_query.count); if (performance_query->queries) { for (int i = 0; i < performance_query->count; i++) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 263fefc1d04f..2818afdd4807 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -452,6 +452,7 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_timestamp_query timestamp; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -484,15 +485,15 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, u32 offset, sync; if (copy_from_user(&offset, offsets++, sizeof(offset))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].offset = offset; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); @@ -500,6 +501,10 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, job->timestamp_query.count = timestamp.count; return 0; + +error: + __v3d_timestamp_query_info_free(qinfo, i); + return err; } static int @@ -509,6 +514,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, { u32 __user *syncs; struct drm_v3d_reset_timestamp_query reset; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -539,8 +545,8 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, job->timestamp_query.queries[i].offset = reset.offset + 8 * i; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); @@ -548,6 +554,10 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, job->timestamp_query.count = reset.count; return 0; + +error: + __v3d_timestamp_query_info_free(qinfo, i); + return err; } /* Get data for the copy timestamp query results job submission. */ @@ -558,7 +568,7 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_copy_timestamp_query copy; - int i; + int i, err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -591,15 +601,15 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, u32 offset, sync; if (copy_from_user(&offset, offsets++, sizeof(offset))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].offset = offset; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); @@ -613,6 +623,10 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, job->copy.stride = copy.stride; return 0; + +error: + __v3d_timestamp_query_info_free(qinfo, i); + return err; } static int