[PATCH 0/6] Fixes and improvements to syncobj handling in TegraDRM submit path

[PATCH 0/6] Fixes and improvements to syncobj handling in TegraDRM submit path

[Mikko Perttunen]

The syncobj code in the TegraDRM submit path has been quite broken for a 
while. Let's get it in (better) shape for unstaging.

The first part of the series, patches 1-4 are fixes and and a cosmetic 
improvement. They fix the syncobj handling, though it remains unoptimal 
- CPU waited.

Patch 5 adds host1x_fence_extract, necessary to allow other drivers to 
see inside syncpoint fences and program hardware to do direct waits.

Patch 6 adds support to the TegraDRM submit path for waiting such fences 
directly using Host1x channel commands.

Tested on Tegra234.

Best regards
Mikko

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
Mikko Perttunen (6):
      drm/tegra: Fix syncobj_in wait return value check
      drm/tegra: Don't pass an ERR_PTR to drm_syncobj_replace_fence()
      drm/tegra: Drop reference on postfence
      drm/tegra: Remove unused includes from submit.c
      gpu: host1x: Add host1x_fence_extract()
      drm/tegra: Wait for syncpoint syncobj_in fences in hardware

 drivers/gpu/drm/tegra/submit.c | 109 +++++++++++++++++++++++++++++++++++------
 drivers/gpu/host1x/fence.c     |  43 ++++++++++++++++
 include/linux/host1x.h         |   2 +
 include/uapi/drm/tegra_drm.h   |   4 +-
 4 files changed, 140 insertions(+), 18 deletions(-)
---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260727-b4-host1x-syncobj-wait-84284ac2838c

[PATCH 1/6] drm/tegra: Fix syncobj_in wait return value check

[Mikko Perttunen]

dma_fence_wait_timeout() returns the remaining timeout in jiffies on
success, zero on timeout, and a negative error code if interrupted. The
return value was assigned to 'err' and checked as if it were an errno,
the opposite of the correct logic.

Fix the code to use a properly typed variable and check each condition
correctly.

Fixes: 13abe0bb15ce ("drm/tegra: Implement job submission part of new UAPI")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aJM_te551jUwnRv7@stanley.mountain/
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/submit.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index e5841857c937..5dad6dc4eb8c 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -530,6 +530,7 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 
 	if (args->syncobj_in) {
 		struct dma_fence *fence;
+		long wait_err;
 
 		err = drm_syncobj_find_fence(file, args->syncobj_in, 0, 0, &fence);
 		if (err) {
@@ -537,10 +538,15 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 			goto unlock;
 		}
 
-		err = dma_fence_wait_timeout(fence, true, msecs_to_jiffies(10000));
+		wait_err = dma_fence_wait_timeout(fence, true, msecs_to_jiffies(10000));
 		dma_fence_put(fence);
-		if (err) {
+		if (wait_err == 0) {
 			SUBMIT_ERR(context, "wait for syncobj_in timed out");
+			err = -ETIMEDOUT;
+			goto unlock;
+		} else if (wait_err < 0) {
+			/* In practice, -ERESTARTSYS */
+			err = wait_err;
 			goto unlock;
 		}
 	}

-- 
2.53.0

[PATCH 2/6] drm/tegra: Don't pass an ERR_PTR to drm_syncobj_replace_fence()

[Mikko Perttunen]

When host1x_fence_create() fails, avoid trying to attach the error
pointer to the out syncobj.

Fixes: 13abe0bb15ce ("drm/tegra: Implement job submission part of new UAPI")
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/submit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index 5dad6dc4eb8c..3f20b5a24447 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -655,6 +655,7 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 		if (IS_ERR(fence)) {
 			err = PTR_ERR(fence);
 			SUBMIT_ERR(context, "failed to create postfence: %d", err);
+			goto put_job;
 		}
 
 		drm_syncobj_replace_fence(syncobj, fence);

-- 
2.53.0

[PATCH 3/6] drm/tegra: Drop reference on postfence

[Mikko Perttunen]

drm_syncobj_replace_fence() calls dma_fence_get() on the parameter
fence, so we need to drop ours. Add a call to dma_fence_put() to do
that.

Fixes: 13abe0bb15ce ("drm/tegra: Implement job submission part of new UAPI")
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/submit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index 3f20b5a24447..658396f2e954 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -659,6 +659,7 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 		}
 
 		drm_syncobj_replace_fence(syncobj, fence);
+		dma_fence_put(fence);
 	}
 
 	goto put_job;

-- 
2.53.0

[PATCH 4/6] drm/tegra: Remove unused includes from submit.c

[Mikko Perttunen]

The dma-fence-array.h and sync_file.h headers are not used. Drop the
includes.

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

diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index 658396f2e954..1e5332eb6e26 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2020 NVIDIA Corporation */
 
-#include <linux/dma-fence-array.h>
 #include <linux/dma-mapping.h>
 #include <linux/file.h>
 #include <linux/host1x.h>
@@ -12,7 +11,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
-#include <linux/sync_file.h>
 
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>

-- 
2.53.0

[PATCH 5/6] gpu: host1x: Add host1x_fence_extract()

[Mikko Perttunen]

Syncpoint fences are backed by a hardware syncpoint / threshold, which
various hardware is capable of waiting for directly without CPU
intervention. Add a function to allow drivers for such devices to
extract the details to allow programming the hardware accordingly.

The function also provides the host1x instance corresponding to the
syncpoint since a system may contain multiple instances of host1x with
separate sets of syncpoints.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/host1x/fence.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/host1x.h     |  2 ++
 2 files changed, 45 insertions(+)

diff --git a/drivers/gpu/host1x/fence.c b/drivers/gpu/host1x/fence.c
index b9a7d0bf91f8..1acf579c562b 100644
--- a/drivers/gpu/host1x/fence.c
+++ b/drivers/gpu/host1x/fence.c
@@ -152,3 +152,46 @@ void host1x_fence_cancel(struct dma_fence *f)
 	flush_delayed_work(&sf->timeout_work);
 }
 EXPORT_SYMBOL(host1x_fence_cancel);
+
+/**
+ * host1x_fence_extract() - extract underlying syncpoint/threshold from fence
+ * @fence: fence to extract information from
+ * @host1x: out pointer for host1x instance corresponding to the syncpoint
+ * @id: out pointer for the syncpoint ID the fence pends on
+ * @threshold: out pointer for the syncpoint value the fence pends on
+ *
+ * Extracts underlying host1x hardware related information from a host1x
+ * syncpoint fence. This information can be used by devices that have direct
+ * access to host1x syncpoints to wait the fence in hardware without CPU
+ * interaction.
+ *
+ * A system might have multiple instances of host1x with separate sets of
+ * syncpoints. Callers must ensure any hardware that is programmed is accessing
+ * the correct set of syncpoints as identified by @host1x.
+ *
+ * Note that a fence that has already signalled may no longer be identifiable as
+ * a syncpoint fence, as the dma_fence framework can detach fence ops on
+ * signalled fences.
+ *
+ * Returns 0 on success, or -EINVAL if @fence is not a syncpoint fence.
+ */
+int host1x_fence_extract(struct dma_fence *fence, struct host1x **host1x,
+			 u32 *id, u32 *threshold)
+{
+	struct host1x_syncpt_fence *f;
+
+	if (rcu_access_pointer(fence->ops) != &host1x_syncpt_fence_ops)
+		return -EINVAL;
+
+	f = to_host1x_fence(fence);
+
+	if (host1x)
+		*host1x = f->sp->host;
+	if (id)
+		*id = f->sp->id;
+	if (threshold)
+		*threshold = f->threshold;
+
+	return 0;
+}
+EXPORT_SYMBOL(host1x_fence_extract);
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index a7a675783136..1221acf83942 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -237,6 +237,8 @@ void host1x_syncpt_release_vblank_reservation(struct host1x_client *client,
 struct dma_fence *host1x_fence_create(struct host1x_syncpt *sp, u32 threshold,
 				      bool timeout);
 void host1x_fence_cancel(struct dma_fence *fence);
+int host1x_fence_extract(struct dma_fence *fence, struct host1x **host1x,
+			 u32 *id, u32 *threshold);
 
 /*
  * host1x channel

-- 
2.53.0

[PATCH 6/6] drm/tegra: Wait for syncpoint syncobj_in fences in hardware

[Mikko Perttunen]

Currently all syncobj_in fences are waited synchronously on the CPU.
However, since Host1x channels are capable of waiting for syncpoint
fences on the hardware, let's use that capability.

This filters the any non-syncpoint (or different host1x instance) fences
in syncobj_in, waits only those on the CPU, and adds the rest as waits
to the channel before the actual job contents.

Also make minor clarifications to the doc comments in the UAPI header.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/submit.c | 111 +++++++++++++++++++++++++++++++++--------
 include/uapi/drm/tegra_drm.h   |   4 +-
 2 files changed, 93 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index 1e5332eb6e26..54312922f7b2 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2020 NVIDIA Corporation */
 
+#include <linux/dma-fence-unwrap.h>
 #include <linux/dma-mapping.h>
 #include <linux/file.h>
 #include <linux/host1x.h>
@@ -344,6 +345,71 @@ static int submit_get_syncpt(struct tegra_drm_context *context, struct host1x_jo
 	return 0;
 }
 
+static int submit_handle_in_fence(struct tegra_drm_context *context, struct host1x *host1x,
+				  struct dma_fence *fence, u32 *num_waits)
+{
+	struct dma_fence_unwrap iter;
+	struct host1x *fence_host1x;
+	struct dma_fence *f;
+	int err = 0;
+
+	*num_waits = 0;
+
+	dma_fence_unwrap_for_each(f, &iter, fence) {
+		long wait_err;
+
+		if (err)
+			continue;
+
+		if (dma_fence_is_signaled(f))
+			continue;
+
+		err = host1x_fence_extract(f, &fence_host1x, NULL, NULL);
+		if (!err && fence_host1x == host1x) {
+			(*num_waits)++;
+			continue;
+		}
+
+		wait_err = dma_fence_wait_timeout(f, true, msecs_to_jiffies(10000));
+		if (wait_err == 0) {
+			SUBMIT_ERR(context, "wait for syncobj_in timed out");
+			err = -ETIMEDOUT;
+			continue;
+		} else if (wait_err < 0) {
+			/* In practice, -ERESTARTSYS */
+			err = wait_err;
+			continue;
+		} else {
+			err = 0;
+		}
+	}
+
+	return err;
+}
+
+static void submit_job_add_prefence(struct host1x_job *job, struct dma_fence *fence, u32 class)
+{
+	struct dma_fence_unwrap iter;
+	struct dma_fence *f;
+
+	dma_fence_unwrap_for_each(f, &iter, fence) {
+		u32 id, threshold;
+
+		if (dma_fence_is_signaled(f))
+			continue;
+
+		if (host1x_fence_extract(f, NULL, &id, &threshold)) {
+			/*
+			 * The fence may have signalled since the check above, and
+			 * the dma_fence framework then detached its ops.
+			 */
+			continue;
+		}
+
+		host1x_job_add_wait(job, id, threshold, false, class);
+	}
+}
+
 static int submit_job_add_gather(struct host1x_job *job, struct tegra_drm_context *context,
 				 struct drm_tegra_submit_cmd_gather_uptr *cmd,
 				 struct gather_bo *bo, u32 *offset,
@@ -389,7 +455,7 @@ static int submit_job_add_gather(struct host1x_job *job, struct tegra_drm_contex
 static struct host1x_job *
 submit_create_job(struct tegra_drm_context *context, struct gather_bo *bo,
 		  struct drm_tegra_channel_submit *args, struct tegra_drm_submit_data *job_data,
-		  struct xarray *syncpoints)
+		  struct xarray *syncpoints, struct dma_fence *in_fence, u32 num_prefence_waits)
 {
 	struct drm_tegra_submit_cmd *cmds;
 	u32 i, gather_offset = 0, class;
@@ -406,7 +472,7 @@ submit_create_job(struct tegra_drm_context *context, struct gather_bo *bo,
 		return ERR_CAST(cmds);
 	}
 
-	job = host1x_job_alloc(context->channel, args->num_cmds, 0, true);
+	job = host1x_job_alloc(context->channel, args->num_cmds + num_prefence_waits, 0, true);
 	if (!job) {
 		SUBMIT_ERR(context, "failed to allocate memory for job");
 		job = ERR_PTR(-ENOMEM);
@@ -421,6 +487,9 @@ submit_create_job(struct tegra_drm_context *context, struct gather_bo *bo,
 	job->class = context->client->base.class;
 	job->serialize = true;
 
+	if (in_fence)
+		submit_job_add_prefence(job, in_fence, class);
+
 	for (i = 0; i < args->num_cmds; i++) {
 		struct drm_tegra_submit_cmd *cmd = &cmds[i];
 
@@ -506,11 +575,14 @@ static void release_job(struct host1x_job *job)
 int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 				   struct drm_file *file)
 {
+	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
 	struct tegra_drm_file *fpriv = file->driver_priv;
 	struct drm_tegra_channel_submit *args = data;
 	struct tegra_drm_submit_data *job_data;
 	struct drm_syncobj *syncobj = NULL;
+	struct dma_fence *in_fence = NULL;
 	struct tegra_drm_context *context;
+	u32 num_prefence_waits = 0;
 	struct host1x_job *job;
 	struct gather_bo *bo;
 	u32 i;
@@ -527,26 +599,15 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 	}
 
 	if (args->syncobj_in) {
-		struct dma_fence *fence;
-		long wait_err;
-
-		err = drm_syncobj_find_fence(file, args->syncobj_in, 0, 0, &fence);
+		err = drm_syncobj_find_fence(file, args->syncobj_in, 0, 0, &in_fence);
 		if (err) {
 			SUBMIT_ERR(context, "invalid syncobj_in '%#x'", args->syncobj_in);
 			goto unlock;
 		}
 
-		wait_err = dma_fence_wait_timeout(fence, true, msecs_to_jiffies(10000));
-		dma_fence_put(fence);
-		if (wait_err == 0) {
-			SUBMIT_ERR(context, "wait for syncobj_in timed out");
-			err = -ETIMEDOUT;
-			goto unlock;
-		} else if (wait_err < 0) {
-			/* In practice, -ERESTARTSYS */
-			err = wait_err;
-			goto unlock;
-		}
+		err = submit_handle_in_fence(context, host1x, in_fence, &num_prefence_waits);
+		if (err)
+			goto put_in_fence;
 	}
 
 	if (args->syncobj_out) {
@@ -554,14 +615,14 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 		if (!syncobj) {
 			SUBMIT_ERR(context, "invalid syncobj_out '%#x'", args->syncobj_out);
 			err = -ENOENT;
-			goto unlock;
+			goto put_in_fence;
 		}
 	}
 
 	/* Allocate gather BO and copy gather words in. */
 	err = submit_copy_gather_data(&bo, drm->dev, context, args);
 	if (err)
-		goto unlock;
+		goto put_in_fence;
 
 	job_data = kzalloc_obj(*job_data);
 	if (!job_data) {
@@ -576,12 +637,20 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 		goto free_job_data;
 
 	/* Allocate host1x_job and add gathers and waits to it. */
-	job = submit_create_job(context, bo, args, job_data, &fpriv->syncpoints);
+	job = submit_create_job(context, bo, args, job_data, &fpriv->syncpoints, in_fence,
+				num_prefence_waits);
 	if (IS_ERR(job)) {
 		err = PTR_ERR(job);
 		goto free_job_data;
 	}
 
+	/*
+	 * The prefence has been recorded as plain syncpoint waits, so the fence
+	 * itself is not needed anymore.
+	 */
+	dma_fence_put(in_fence);
+	in_fence = NULL;
+
 	/* Map gather data for Host1x. */
 	err = host1x_job_pin(job, context->client->base.dev);
 	if (err) {
@@ -680,6 +749,8 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
 	kfree(job_data);
 put_bo:
 	gather_bo_put(&bo->base);
+put_in_fence:
+	dma_fence_put(in_fence);
 unlock:
 	if (syncobj)
 		drm_syncobj_put(syncobj);
diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
index 8f21f3a44832..b17472103e91 100644
--- a/include/uapi/drm/tegra_drm.h
+++ b/include/uapi/drm/tegra_drm.h
@@ -986,8 +986,8 @@ struct drm_tegra_channel_submit {
 	/**
 	 * @syncobj_in: [in]
 	 *
-	 * Handle for DRM syncobj that will be waited before submission.
-	 * Ignored if zero.
+	 * Handle for DRM syncobj that will be waited for before the job
+	 * executes. Ignored if zero.
 	 */
 	__u32 syncobj_in;
 

-- 
2.53.0