| Message ID | 20170309175718.14843-4-mperttunen@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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; }
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(-)