[3/3] drm/tegra: Support for sync file-based fences in submit

Message ID 20170309175718.14843-4-mperttunen@nvidia.com
State New
Headers show

Commit Message

Mikko Perttunen March 9, 2017, 5:57 p.m.
Add support for sync file-based prefences and postfences
to job submission. Fences are passed to the Host1x implementation.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 69 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 10 deletions(-)

Comments

Thierry Reding March 13, 2017, 7:15 a.m. | #1
On Thu, Mar 09, 2017 at 07:57:18PM +0200, Mikko Perttunen wrote:
> Add support for sync file-based prefences and postfences
> to job submission. Fences are passed to the Host1x implementation.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 69 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 59 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 64dff8530403..bf4a2a13c17d 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -10,6 +10,7 @@
>  #include <linux/bitops.h>
>  #include <linux/host1x.h>
>  #include <linux/iommu.h>
> +#include <linux/sync_file.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -344,6 +345,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  		     struct drm_tegra_submit *args, struct drm_device *drm,
>  		     struct drm_file *file)
>  {
> +	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
>  	unsigned int num_cmdbufs = args->num_cmdbufs;
>  	unsigned int num_relocs = args->num_relocs;
>  	unsigned int num_waitchks = args->num_waitchks;
> @@ -361,6 +363,11 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  	if (args->num_syncpts != 1)
>  		return -EINVAL;
>  
> +	/* Check for unrecognized flags */
> +	if (args->flags & ~(DRM_TEGRA_SUBMIT_WAIT_FENCE_FD |
> +	                    DRM_TEGRA_SUBMIT_CREATE_FENCE_FD))
> +		return -EINVAL;
> +
>  	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
>  			       args->num_relocs, args->num_waitchks);
>  	if (!job)
> @@ -372,19 +379,27 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  	job->class = context->client->base.class;
>  	job->serialize = true;
>  
> +	if (args->flags & DRM_TEGRA_SUBMIT_WAIT_FENCE_FD) {
> +		job->prefence = sync_file_get_fence(args->fence);
> +		if (!job->prefence) {
> +			err = -ENOENT;
> +			goto put_job;
> +		}
> +	}
> +
>  	while (num_cmdbufs) {
>  		struct drm_tegra_cmdbuf cmdbuf;
>  		struct host1x_bo *bo;
>  
>  		if (copy_from_user(&cmdbuf, cmdbufs, sizeof(cmdbuf))) {
>  			err = -EFAULT;
> -			goto fail;
> +			goto put_fence;
>  		}
>  
>  		bo = host1x_bo_lookup(file, cmdbuf.handle);
>  		if (!bo) {
>  			err = -ENOENT;
> -			goto fail;
> +			goto put_fence;
>  		}
>  
>  		host1x_job_add_gather(job, bo, cmdbuf.words, cmdbuf.offset);
> @@ -398,19 +413,19 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  						  &relocs[num_relocs], drm,
>  						  file);
>  		if (err < 0)
> -			goto fail;
> +			goto put_fence;
>  	}
>  
>  	if (copy_from_user(job->waitchk, waitchks,
>  			   sizeof(*waitchks) * num_waitchks)) {
>  		err = -EFAULT;
> -		goto fail;
> +		goto put_fence;
>  	}
>  
>  	if (copy_from_user(&syncpt, (void __user *)(uintptr_t)args->syncpts,
>  			   sizeof(syncpt))) {
>  		err = -EFAULT;
> -		goto fail;
> +		goto put_fence;
>  	}
>  
>  	job->is_addr_reg = context->client->ops->is_addr_reg;
> @@ -423,20 +438,54 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  
>  	err = host1x_job_pin(job, context->client->base.dev);
>  	if (err)
> -		goto fail;
> +		goto put_fence;
>  
>  	err = host1x_job_submit(job);
>  	if (err)
> -		goto fail_submit;
> +		goto unpin_job;

Shouldn't all error-unwinding gotos after this jump to the unpin_job
label as well? Seems like they all jump to put_fence instead, which I
think would leave the job pinned on failure.

>  
> -	args->fence = job->syncpt_end;
> +	if (args->flags & DRM_TEGRA_SUBMIT_CREATE_FENCE_FD) {
> +		struct dma_fence *fence;
> +		struct sync_file *file;
> +
> +		fence = host1x_fence_create(
> +			host1x, host1x_syncpt_get(host1x, job->syncpt_id),
> +			job->syncpt_end);
> +		if (!fence) {
> +			err = -ENOMEM;
> +			goto put_fence;
> +		}
> +
> +		file = sync_file_create(fence);
> +		if (!file) {
> +			dma_fence_put(fence);
> +			err = -ENOMEM;
> +			goto put_fence;
> +		}
> +
> +		err = get_unused_fd_flags(O_CLOEXEC);
> +		if (err < 0) {
> +			dma_fence_put(fence);
> +			goto put_fence;
> +		}
> +
> +		fd_install(err, file->file);
> +		args->fence = err;
> +	} else {
> +		args->fence = job->syncpt_end;
> +	}
>  
> +	if (job->prefence)
> +		dma_fence_put(job->prefence);
>  	host1x_job_put(job);
>  	return 0;
>  
> -fail_submit:
> +unpin_job:
>  	host1x_job_unpin(job);
> -fail:
> +put_fence:
> +	if (job->prefence)
> +		dma_fence_put(job->prefence);

Since we already have a conditional to check for usage of fence, I'm
wondering if we can simplify this a little and leave out the put_fence
label altogether, like so:

	unpin_job:
		host1x_job_unpin(job);
	put_job:
		if (job->prefence)
			dma_fence_put(job->prefence);

		host1x_job_put(job);

Thierry
Mikko Perttunen March 13, 2017, 9:07 a.m. | #2
On 13.03.2017 09:15, Thierry Reding wrote:
> On Thu, Mar 09, 2017 at 07:57:18PM +0200, Mikko Perttunen wrote:
>> Add support for sync file-based prefences and postfences
>> to job submission. Fences are passed to the Host1x implementation.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>  drivers/gpu/drm/tegra/drm.c | 69 ++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index 64dff8530403..bf4a2a13c17d 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/bitops.h>
>>  #include <linux/host1x.h>
>>  #include <linux/iommu.h>
>> +#include <linux/sync_file.h>
>>
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_atomic_helper.h>
>> @@ -344,6 +345,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  		     struct drm_tegra_submit *args, struct drm_device *drm,
>>  		     struct drm_file *file)
>>  {
>> +	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
>>  	unsigned int num_cmdbufs = args->num_cmdbufs;
>>  	unsigned int num_relocs = args->num_relocs;
>>  	unsigned int num_waitchks = args->num_waitchks;
>> @@ -361,6 +363,11 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  	if (args->num_syncpts != 1)
>>  		return -EINVAL;
>>
>> +	/* Check for unrecognized flags */
>> +	if (args->flags & ~(DRM_TEGRA_SUBMIT_WAIT_FENCE_FD |
>> +	                    DRM_TEGRA_SUBMIT_CREATE_FENCE_FD))
>> +		return -EINVAL;
>> +
>>  	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
>>  			       args->num_relocs, args->num_waitchks);
>>  	if (!job)
>> @@ -372,19 +379,27 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  	job->class = context->client->base.class;
>>  	job->serialize = true;
>>
>> +	if (args->flags & DRM_TEGRA_SUBMIT_WAIT_FENCE_FD) {
>> +		job->prefence = sync_file_get_fence(args->fence);
>> +		if (!job->prefence) {
>> +			err = -ENOENT;
>> +			goto put_job;
>> +		}
>> +	}
>> +
>>  	while (num_cmdbufs) {
>>  		struct drm_tegra_cmdbuf cmdbuf;
>>  		struct host1x_bo *bo;
>>
>>  		if (copy_from_user(&cmdbuf, cmdbufs, sizeof(cmdbuf))) {
>>  			err = -EFAULT;
>> -			goto fail;
>> +			goto put_fence;
>>  		}
>>
>>  		bo = host1x_bo_lookup(file, cmdbuf.handle);
>>  		if (!bo) {
>>  			err = -ENOENT;
>> -			goto fail;
>> +			goto put_fence;
>>  		}
>>
>>  		host1x_job_add_gather(job, bo, cmdbuf.words, cmdbuf.offset);
>> @@ -398,19 +413,19 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  						  &relocs[num_relocs], drm,
>>  						  file);
>>  		if (err < 0)
>> -			goto fail;
>> +			goto put_fence;
>>  	}
>>
>>  	if (copy_from_user(job->waitchk, waitchks,
>>  			   sizeof(*waitchks) * num_waitchks)) {
>>  		err = -EFAULT;
>> -		goto fail;
>> +		goto put_fence;
>>  	}
>>
>>  	if (copy_from_user(&syncpt, (void __user *)(uintptr_t)args->syncpts,
>>  			   sizeof(syncpt))) {
>>  		err = -EFAULT;
>> -		goto fail;
>> +		goto put_fence;
>>  	}
>>
>>  	job->is_addr_reg = context->client->ops->is_addr_reg;
>> @@ -423,20 +438,54 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>
>>  	err = host1x_job_pin(job, context->client->base.dev);
>>  	if (err)
>> -		goto fail;
>> +		goto put_fence;
>>
>>  	err = host1x_job_submit(job);
>>  	if (err)
>> -		goto fail_submit;
>> +		goto unpin_job;
>
> Shouldn't all error-unwinding gotos after this jump to the unpin_job
> label as well? Seems like they all jump to put_fence instead, which I
> think would leave the job pinned on failure.

After host1x_job_submit is succesfully called, host1x's job tracking 
owns the job and will call unpin on it once it finishes or times out, so 
we cannot unpin it from here.

>
>>
>> -	args->fence = job->syncpt_end;
>> +	if (args->flags & DRM_TEGRA_SUBMIT_CREATE_FENCE_FD) {
>> +		struct dma_fence *fence;
>> +		struct sync_file *file;
>> +
>> +		fence = host1x_fence_create(
>> +			host1x, host1x_syncpt_get(host1x, job->syncpt_id),
>> +			job->syncpt_end);
>> +		if (!fence) {
>> +			err = -ENOMEM;
>> +			goto put_fence;
>> +		}
>> +
>> +		file = sync_file_create(fence);
>> +		if (!file) {
>> +			dma_fence_put(fence);
>> +			err = -ENOMEM;
>> +			goto put_fence;
>> +		}
>> +
>> +		err = get_unused_fd_flags(O_CLOEXEC);
>> +		if (err < 0) {
>> +			dma_fence_put(fence);
>> +			goto put_fence;
>> +		}
>> +
>> +		fd_install(err, file->file);
>> +		args->fence = err;
>> +	} else {
>> +		args->fence = job->syncpt_end;
>> +	}
>>
>> +	if (job->prefence)
>> +		dma_fence_put(job->prefence);
>>  	host1x_job_put(job);
>>  	return 0;
>>
>> -fail_submit:
>> +unpin_job:
>>  	host1x_job_unpin(job);
>> -fail:
>> +put_fence:
>> +	if (job->prefence)
>> +		dma_fence_put(job->prefence);
>
> Since we already have a conditional to check for usage of fence, I'm
> wondering if we can simplify this a little and leave out the put_fence
> label altogether, like so:
>
> 	unpin_job:
> 		host1x_job_unpin(job);
> 	put_job:
> 		if (job->prefence)
> 			dma_fence_put(job->prefence);
>
> 		host1x_job_put(job);

Yes, that seems like a good idea.

>
> Thierry
>

Cheers,
Mikko.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 13, 2017, 5:46 p.m. | #3
On Mon, Mar 13, 2017 at 11:07:28AM +0200, Mikko Perttunen wrote:
> 
> 
> On 13.03.2017 09:15, Thierry Reding wrote:
> > On Thu, Mar 09, 2017 at 07:57:18PM +0200, Mikko Perttunen wrote:
> > > Add support for sync file-based prefences and postfences
> > > to job submission. Fences are passed to the Host1x implementation.
> > > 
> > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> > > ---
> > >  drivers/gpu/drm/tegra/drm.c | 69 ++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 59 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > > index 64dff8530403..bf4a2a13c17d 100644
> > > --- a/drivers/gpu/drm/tegra/drm.c
> > > +++ b/drivers/gpu/drm/tegra/drm.c
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/bitops.h>
> > >  #include <linux/host1x.h>
> > >  #include <linux/iommu.h>
> > > +#include <linux/sync_file.h>
> > > 
> > >  #include <drm/drm_atomic.h>
> > >  #include <drm/drm_atomic_helper.h>
> > > @@ -344,6 +345,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
> > >  		     struct drm_tegra_submit *args, struct drm_device *drm,
> > >  		     struct drm_file *file)
> > >  {
> > > +	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
> > >  	unsigned int num_cmdbufs = args->num_cmdbufs;
> > >  	unsigned int num_relocs = args->num_relocs;
> > >  	unsigned int num_waitchks = args->num_waitchks;
> > > @@ -361,6 +363,11 @@ int tegra_drm_submit(struct tegra_drm_context *context,
> > >  	if (args->num_syncpts != 1)
> > >  		return -EINVAL;
> > > 
> > > +	/* Check for unrecognized flags */
> > > +	if (args->flags & ~(DRM_TEGRA_SUBMIT_WAIT_FENCE_FD |
> > > +	                    DRM_TEGRA_SUBMIT_CREATE_FENCE_FD))
> > > +		return -EINVAL;
> > > +
> > >  	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
> > >  			       args->num_relocs, args->num_waitchks);
> > >  	if (!job)
> > > @@ -372,19 +379,27 @@ int tegra_drm_submit(struct tegra_drm_context *context,
> > >  	job->class = context->client->base.class;
> > >  	job->serialize = true;
> > > 
> > > +	if (args->flags & DRM_TEGRA_SUBMIT_WAIT_FENCE_FD) {
> > > +		job->prefence = sync_file_get_fence(args->fence);
> > > +		if (!job->prefence) {
> > > +			err = -ENOENT;
> > > +			goto put_job;
> > > +		}
> > > +	}
> > > +
> > >  	while (num_cmdbufs) {
> > >  		struct drm_tegra_cmdbuf cmdbuf;
> > >  		struct host1x_bo *bo;
> > > 
> > >  		if (copy_from_user(&cmdbuf, cmdbufs, sizeof(cmdbuf))) {
> > >  			err = -EFAULT;
> > > -			goto fail;
> > > +			goto put_fence;
> > >  		}
> > > 
> > >  		bo = host1x_bo_lookup(file, cmdbuf.handle);
> > >  		if (!bo) {
> > >  			err = -ENOENT;
> > > -			goto fail;
> > > +			goto put_fence;
> > >  		}
> > > 
> > >  		host1x_job_add_gather(job, bo, cmdbuf.words, cmdbuf.offset);
> > > @@ -398,19 +413,19 @@ int tegra_drm_submit(struct tegra_drm_context *context,
> > >  						  &relocs[num_relocs], drm,
> > >  						  file);
> > >  		if (err < 0)
> > > -			goto fail;
> > > +			goto put_fence;
> > >  	}
> > > 
> > >  	if (copy_from_user(job->waitchk, waitchks,
> > >  			   sizeof(*waitchks) * num_waitchks)) {
> > >  		err = -EFAULT;
> > > -		goto fail;
> > > +		goto put_fence;
> > >  	}
> > > 
> > >  	if (copy_from_user(&syncpt, (void __user *)(uintptr_t)args->syncpts,
> > >  			   sizeof(syncpt))) {
> > >  		err = -EFAULT;
> > > -		goto fail;
> > > +		goto put_fence;
> > >  	}
> > > 
> > >  	job->is_addr_reg = context->client->ops->is_addr_reg;
> > > @@ -423,20 +438,54 @@ int tegra_drm_submit(struct tegra_drm_context *context,
> > > 
> > >  	err = host1x_job_pin(job, context->client->base.dev);
> > >  	if (err)
> > > -		goto fail;
> > > +		goto put_fence;
> > > 
> > >  	err = host1x_job_submit(job);
> > >  	if (err)
> > > -		goto fail_submit;
> > > +		goto unpin_job;
> > 
> > Shouldn't all error-unwinding gotos after this jump to the unpin_job
> > label as well? Seems like they all jump to put_fence instead, which I
> > think would leave the job pinned on failure.
> 
> After host1x_job_submit is succesfully called, host1x's job tracking owns
> the job and will call unpin on it once it finishes or times out, so we
> cannot unpin it from here.

Okay, might be worth explaining that in a comment above the call to
host1x_job_submit() because it's not obvious and I'm pretty sure people
would send in patches to "fix" this.

Thierry

Patch

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 64dff8530403..bf4a2a13c17d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -10,6 +10,7 @@ 
 #include <linux/bitops.h>
 #include <linux/host1x.h>
 #include <linux/iommu.h>
+#include <linux/sync_file.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -344,6 +345,7 @@  int tegra_drm_submit(struct tegra_drm_context *context,
 		     struct drm_tegra_submit *args, struct drm_device *drm,
 		     struct drm_file *file)
 {
+	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
 	unsigned int num_cmdbufs = args->num_cmdbufs;
 	unsigned int num_relocs = args->num_relocs;
 	unsigned int num_waitchks = args->num_waitchks;
@@ -361,6 +363,11 @@  int tegra_drm_submit(struct tegra_drm_context *context,
 	if (args->num_syncpts != 1)
 		return -EINVAL;
 
+	/* Check for unrecognized flags */
+	if (args->flags & ~(DRM_TEGRA_SUBMIT_WAIT_FENCE_FD |
+	                    DRM_TEGRA_SUBMIT_CREATE_FENCE_FD))
+		return -EINVAL;
+
 	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
 			       args->num_relocs, args->num_waitchks);
 	if (!job)
@@ -372,19 +379,27 @@  int tegra_drm_submit(struct tegra_drm_context *context,
 	job->class = context->client->base.class;
 	job->serialize = true;
 
+	if (args->flags & DRM_TEGRA_SUBMIT_WAIT_FENCE_FD) {
+		job->prefence = sync_file_get_fence(args->fence);
+		if (!job->prefence) {
+			err = -ENOENT;
+			goto put_job;
+		}
+	}
+
 	while (num_cmdbufs) {
 		struct drm_tegra_cmdbuf cmdbuf;
 		struct host1x_bo *bo;
 
 		if (copy_from_user(&cmdbuf, cmdbufs, sizeof(cmdbuf))) {
 			err = -EFAULT;
-			goto fail;
+			goto put_fence;
 		}
 
 		bo = host1x_bo_lookup(file, cmdbuf.handle);
 		if (!bo) {
 			err = -ENOENT;
-			goto fail;
+			goto put_fence;
 		}
 
 		host1x_job_add_gather(job, bo, cmdbuf.words, cmdbuf.offset);
@@ -398,19 +413,19 @@  int tegra_drm_submit(struct tegra_drm_context *context,
 						  &relocs[num_relocs], drm,
 						  file);
 		if (err < 0)
-			goto fail;
+			goto put_fence;
 	}
 
 	if (copy_from_user(job->waitchk, waitchks,
 			   sizeof(*waitchks) * num_waitchks)) {
 		err = -EFAULT;
-		goto fail;
+		goto put_fence;
 	}
 
 	if (copy_from_user(&syncpt, (void __user *)(uintptr_t)args->syncpts,
 			   sizeof(syncpt))) {
 		err = -EFAULT;
-		goto fail;
+		goto put_fence;
 	}
 
 	job->is_addr_reg = context->client->ops->is_addr_reg;
@@ -423,20 +438,54 @@  int tegra_drm_submit(struct tegra_drm_context *context,
 
 	err = host1x_job_pin(job, context->client->base.dev);
 	if (err)
-		goto fail;
+		goto put_fence;
 
 	err = host1x_job_submit(job);
 	if (err)
-		goto fail_submit;
+		goto unpin_job;
 
-	args->fence = job->syncpt_end;
+	if (args->flags & DRM_TEGRA_SUBMIT_CREATE_FENCE_FD) {
+		struct dma_fence *fence;
+		struct sync_file *file;
+
+		fence = host1x_fence_create(
+			host1x, host1x_syncpt_get(host1x, job->syncpt_id),
+			job->syncpt_end);
+		if (!fence) {
+			err = -ENOMEM;
+			goto put_fence;
+		}
+
+		file = sync_file_create(fence);
+		if (!file) {
+			dma_fence_put(fence);
+			err = -ENOMEM;
+			goto put_fence;
+		}
+
+		err = get_unused_fd_flags(O_CLOEXEC);
+		if (err < 0) {
+			dma_fence_put(fence);
+			goto put_fence;
+		}
+
+		fd_install(err, file->file);
+		args->fence = err;
+	} else {
+		args->fence = job->syncpt_end;
+	}
 
+	if (job->prefence)
+		dma_fence_put(job->prefence);
 	host1x_job_put(job);
 	return 0;
 
-fail_submit:
+unpin_job:
 	host1x_job_unpin(job);
-fail:
+put_fence:
+	if (job->prefence)
+		dma_fence_put(job->prefence);
+put_job:
 	host1x_job_put(job);
 	return err;
 }