Reviewed-by: Iago Toral Quiroga <itoral@xxxxxxxxxx>
With that said, I don't like how we are doing error handling here, I
think we want to simplify this and try to make it so we centralize
error handling in one place instead of having multiple error exits
paths, each one trying to do the right thing at that point. This is
error prone, as this patch is showing.
Here is a proposal to make this better:
Make job memory allocation part of v3d_job_init. v3d_job init already
handles error conditions nicely. If we do this, we no longer need to
handle allocation errors in ioctls one by one and for any job we only
have two scenarios: v3d_job_init was successul or it failed (in which
case we know it already cleaned up after itself and we should have a
NULL job as a result). If v3d_job_init failed, then we *always* jump to
the fail tag and there we call v3d_job_cleanup for all jobs that can be
created in that ioctl. If a job is NULL then v3d_job_cleanup returns
early, otherwise, we know it is a fully initialized job, so it does
drm_sched_job_cleanup + v3d_job_put.
I think that should make error handling in these functions a lot
easier.
Iago
On Thu, 2021-09-16 at 22:27 +0100, Melissa Wen wrote:
> In a cl submission, when bin job initialization fails, sched job
> resources
> were already allocated for the render job. At this point,
> drm_sched_job_init(render) was done in v3d_job_init but the render
> job is
> aborted before drm_sched_job_arm (in v3d_job_push) happens;
> therefore, not
> only v3d_job_put but also drm_sched_job_cleanup should be called (by
> v3d_job_cleanup). A similar issue is addressed for csd and tfu
> submissions.
>
> The issue was noticed from a review by Iago Toral in a patch that
> touches
> the same part of the code.
>
> Fixes: 916044fac8623 ("drm/v3d: Move drm_sched_job_init to
> v3d_job_init")
> Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx>
> ---
> drivers/gpu/drm/v3d/v3d_gem.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
> b/drivers/gpu/drm/v3d/v3d_gem.c
> index 1953706bdaeb..ead0be8d48a7 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -567,14 +567,14 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
> if (args->bcl_start != args->bcl_end) {
> bin = kcalloc(1, sizeof(*bin), GFP_KERNEL);
> if (!bin) {
> - v3d_job_put(&render->base);
> + v3d_job_cleanup(&render->base);
>
> > return -ENOMEM;
> }
>
> ret = v3d_job_init(v3d, file_priv, &bin->base,
> v3d_job_free, args->in_sync_bcl,
> V3D_BIN);
> if (ret) {
> - v3d_job_put(&render->base);
> + v3d_job_cleanup(&render->base);
> kfree(bin);
> return ret;
> }
> @@ -716,7 +716,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> *data,
> job->base.bo = kcalloc(ARRAY_SIZE(args->bo_handles),
> sizeof(*job->base.bo), GFP_KERNEL);
> if (!job->base.bo) {
> - v3d_job_put(&job->base);
> + v3d_job_cleanup(&job->base);
> return -ENOMEM;
> }
>
> @@ -810,14 +810,13 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> void *data,
>
> clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
> if (!clean_job) {
> - v3d_job_put(&job->base);
> - kfree(job);
> + v3d_job_cleanup(&job->base);
> return -ENOMEM;
> }
>
> ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0,
> V3D_CACHE_CLEAN);
> if (ret) {
> - v3d_job_put(&job->base);
> + v3d_job_cleanup(&job->base);
> kfree(clean_job);
> return ret;
> }
