On 31/08/2021 14:35, Boris Brezillon wrote:
> The labels are misleading. Even though they are all prefixed with 'fail_'
> the success case also takes that path, and we should definitely not
> cleanup the job if it's been queued. While at it, let's rename those
> labels so we don't do the same mistake again.
>
> Fixes: 53516280cc38 ("drm/panfrost: use scheduler dependency tracking")
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
Reviewed-by: Steven Price <steven.price@xxxxxxx>
And also unlike last time...
Tested-by: Steven Price <steven.price@xxxxxxx>
Thanks for the clean up - I should have actually tested the previous
patch, but from the diff (and the previous label names) it was obviously
correct™! But it of course blows up pretty quickly without this change.
Thanks,
Steve
> ---
> drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 16212b6b202e..077cbbfa506b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -253,7 +253,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> job = kzalloc(sizeof(*job), GFP_KERNEL);
> if (!job) {
> ret = -ENOMEM;
> - goto fail_out_sync;
> + goto out_put_syncout;
> }
>
> kref_init(&job->refcount);
> @@ -270,29 +270,30 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> &job->file_priv->sched_entity[slot],
> NULL);
> if (ret)
> - goto fail_job_put;
> + goto out_put_job;
>
> ret = panfrost_copy_in_sync(dev, file, args, job);
> if (ret)
> - goto fail_job;
> + goto out_cleanup_job;
>
> ret = panfrost_lookup_bos(dev, file, args, job);
> if (ret)
> - goto fail_job;
> + goto out_cleanup_job;
>
> ret = panfrost_job_push(job);
> if (ret)
> - goto fail_job;
> + goto out_cleanup_job;
>
> /* Update the return sync object for the job */
> if (sync_out)
> drm_syncobj_replace_fence(sync_out, job->render_done_fence);
>
> -fail_job:
> - drm_sched_job_cleanup(&job->base);
> -fail_job_put:
> +out_cleanup_job:
> + if (ret)
> + drm_sched_job_cleanup(&job->base);
> +out_put_job:
> panfrost_job_put(job);
> -fail_out_sync:
> +out_put_syncout:
> if (sync_out)
> drm_syncobj_put(sync_out);
>
>
