All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] panfrost: Don't cleanup the job if it was successfully queued
@ 2021-08-31 13:35 Boris Brezillon
  2021-08-31 14:06 ` Daniel Vetter
  2021-09-01 11:29 ` Steven Price
  0 siblings, 2 replies; 3+ messages in thread
From: Boris Brezillon @ 2021-08-31 13:35 UTC (permalink / raw)
  To: Rob Herring, Tomeu Vizoso, Alyssa Rosenzweig, Steven Price, Robin Murphy
  Cc: Daniel Vetter, dri-devel, Boris Brezillon

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] panfrost: Don't cleanup the job if it was successfully queued
  2021-08-31 13:35 [PATCH] panfrost: Don't cleanup the job if it was successfully queued Boris Brezillon
@ 2021-08-31 14:06 ` Daniel Vetter
  2021-09-01 11:29 ` Steven Price
  1 sibling, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2021-08-31 14:06 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Rob Herring, Tomeu Vizoso, Alyssa Rosenzweig, Steven Price,
	Robin Murphy, Daniel Vetter, dri-devel

On Tue, Aug 31, 2021 at 03:35:56PM +0200, 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>

Oops sorry about that, I thought I've had a t-b from panfrost already?

Anyway this looks good to me:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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);
>  
> -- 
> 2.31.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] panfrost: Don't cleanup the job if it was successfully queued
  2021-08-31 13:35 [PATCH] panfrost: Don't cleanup the job if it was successfully queued Boris Brezillon
  2021-08-31 14:06 ` Daniel Vetter
@ 2021-09-01 11:29 ` Steven Price
  1 sibling, 0 replies; 3+ messages in thread
From: Steven Price @ 2021-09-01 11:29 UTC (permalink / raw)
  To: Boris Brezillon, Rob Herring, Tomeu Vizoso, Alyssa Rosenzweig,
	Robin Murphy
  Cc: Daniel Vetter, dri-devel

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-01 11:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 13:35 [PATCH] panfrost: Don't cleanup the job if it was successfully queued Boris Brezillon
2021-08-31 14:06 ` Daniel Vetter
2021-09-01 11:29 ` Steven Price

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.