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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4508 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel