Re: [PATCH] panfrost: Don't cleanup the job if it was successfully queued

[Steven Price <steven.price@arm.com>]

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@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

And also unlike last time...

Tested-by: Steven Price <steven.price@arm.com>

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);
>  
>