On 09/30, Iago Toral wrote:
On Wed, 2021-09-29 at 10:43 +0100, Melissa Wen wrote:
Move job memory allocation to v3d_job_init function. This aim to facilitate error handling in job initialization, since cleanup steps are similar for all (struct v3d_job)-based types of job involved in a command submission. To generalize v3d_job_init(), this change takes into account that all job structs have the first element a struct v3d_job (bin, render, tfu, csd) or it is a v3d_job itself (clean_job) for pointer casting.
Suggested-by: Iago Toral itoral@igalia.com Signed-off-by: Melissa Wen mwen@igalia.com
drivers/gpu/drm/v3d/v3d_gem.c | 115 +++++++++++++-------------------
1 file changed, 42 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index e60fbc28ef29..9cfa6f8d4357 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -392,6 +392,9 @@ v3d_render_job_free(struct kref *ref)
void v3d_job_cleanup(struct v3d_job *job) {
- if (!job)
return;- drm_sched_job_cleanup(&job->base); v3d_job_put(job);
} @@ -450,12 +453,20 @@ v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
static int v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
struct v3d_job *job, void (*free)(struct kref *ref),
void **container, size_t size, void (*free)(struct kref*ref), u32 in_sync, enum v3d_queue queue) { struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
struct v3d_job *job; int ret;
*container = kcalloc(1, size, GFP_KERNEL);
if (!*container) {
DRM_ERROR("Cannot allocate memory for v3d job.");return -ENOMEM;}
job = *container; job->v3d = v3d; job->free = free;
Right below this hunk we have an early return that doesn't jump to fail:
ret = pm_runtime_get_sync(v3d->drm.dev); if (ret < 0) return ret;so we should kfree(*container) and set it to NULL there, right?
oh, yes. I missed it on porting downstream to upstream.
@@ -479,6 +490,9 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, drm_sched_job_cleanup(&job->base); fail: pm_runtime_put_autosuspend(v3d->drm.dev);
- kfree(*container);
- *container = NULL;
- return ret;
}
(...)
@@ -806,29 +789,15 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, return -EINVAL; }
- job = kcalloc(1, sizeof(*job), GFP_KERNEL);
- if (!job)
return -ENOMEM;- ret = v3d_job_init(v3d, file_priv, &job->base,
- ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), v3d_job_free, args->in_sync, V3D_CSD);
- if (ret) {
kfree(job);return ret;- }
- clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
- if (!clean_job) {
v3d_job_cleanup(&job->base);return -ENOMEM;- }
- if (ret)
goto fail;For this to work we need to explicitly initialize clean_job to NULL. Notice that the same applies to the bin and clean jobs in the CL ioctl, but in that case we're already initializing them to NULL.
yes, thanks for pointing it out.
Melissa
- ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0,
V3D_CACHE_CLEAN);
- if (ret) {
v3d_job_cleanup(&job->base);kfree(clean_job);return ret;- }
- ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
sizeof(*clean_job),
v3d_job_free, 0, V3D_CACHE_CLEAN);if (ret)
goto fail;job->args = *args;
@@ -877,7 +846,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, drm_gem_unlock_reservations(clean_job->bo, clean_job->bo_count, &acquire_ctx); fail:
- v3d_job_cleanup(&job->base);
v3d_job_cleanup((void *)job); v3d_job_cleanup(clean_job);
return ret;