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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
