[PATCH 0/7] gpu: host1x: Preparation work for destaging ABI

[PATCH 0/7] gpu: host1x: Preparation work for destaging ABI

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

These couple of patches clean up some things in the host1x driver and
prepares the way for adding a revamped job submission ABI in a future
patch series.

The ultimate goal is to destage the job submission ABI, so that it is
enabled in default builds of the kernel (currently they are protected
by the STAGING option). The current staging ABI is no longer adequate
for existing use-cases, so we may want to remove it when destaging so
we don't have to keep maintaining it indefinitely. For now, the old
ABI will stick around at least until the new ABI is finalized.

Thierry

Thierry Reding (7):
  gpu: host1x: Remove wait check support
  gpu: host1x: Store pointer to client in jobs
  gpu: host1x: Cleanup loop variable usage
  gpu: host1x: Drop unnecessary host1x argument
  gpu: host1x: Rename relocarray -> relocs for consistency
  gpu: host1x: Use not explicitly sized types
  gpu: host1x: Track client version

 drivers/gpu/drm/tegra/drm.c        |  71 ++------------
 drivers/gpu/host1x/cdma.c          |   2 +-
 drivers/gpu/host1x/cdma.h          |   2 +-
 drivers/gpu/host1x/debug.c         |   2 +-
 drivers/gpu/host1x/dev.h           |   8 --
 drivers/gpu/host1x/hw/channel_hw.c |   5 +-
 drivers/gpu/host1x/hw/syncpt_hw.c  |  11 ---
 drivers/gpu/host1x/intr.c          |  16 ++--
 drivers/gpu/host1x/intr.h          |   8 +-
 drivers/gpu/host1x/job.c           | 149 ++++-------------------------
 drivers/gpu/host1x/job.h           |   4 +-
 drivers/gpu/host1x/syncpt.c        |  10 +-
 drivers/gpu/host1x/syncpt.h        |   3 -
 include/linux/host1x.h             |  27 ++----
 include/trace/events/host1x.h      |  16 ++--
 15 files changed, 61 insertions(+), 273 deletions(-)

-- 
2.17.0

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

[PATCH 1/7] gpu: host1x: Remove wait check support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The job submission userspace ABI doesn't support this and there are no
plans to implement it, so all of this code is dead and can be removed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c        |  62 +--------------
 drivers/gpu/host1x/dev.h           |   8 --
 drivers/gpu/host1x/hw/channel_hw.c |   3 +-
 drivers/gpu/host1x/hw/syncpt_hw.c  |  11 ---
 drivers/gpu/host1x/job.c           | 124 +----------------------------
 drivers/gpu/host1x/syncpt.c        |   6 --
 drivers/gpu/host1x/syncpt.h        |   3 -
 include/linux/host1x.h             |  15 +---
 include/trace/events/host1x.h      |  16 ++--
 9 files changed, 14 insertions(+), 234 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 7afe2f635f74..33e479182773 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -313,46 +313,14 @@ static int host1x_reloc_copy_from_user(struct host1x_reloc *dest,
 	return 0;
 }
 
-static int host1x_waitchk_copy_from_user(struct host1x_waitchk *dest,
-					 struct drm_tegra_waitchk __user *src,
-					 struct drm_file *file)
-{
-	u32 cmdbuf;
-	int err;
-
-	err = get_user(cmdbuf, &src->handle);
-	if (err < 0)
-		return err;
-
-	err = get_user(dest->offset, &src->offset);
-	if (err < 0)
-		return err;
-
-	err = get_user(dest->syncpt_id, &src->syncpt);
-	if (err < 0)
-		return err;
-
-	err = get_user(dest->thresh, &src->thresh);
-	if (err < 0)
-		return err;
-
-	dest->bo = host1x_bo_lookup(file, cmdbuf);
-	if (!dest->bo)
-		return -ENOENT;
-
-	return 0;
-}
-
 int tegra_drm_submit(struct tegra_drm_context *context,
 		     struct drm_tegra_submit *args, struct drm_device *drm,
 		     struct drm_file *file)
 {
 	unsigned int num_cmdbufs = args->num_cmdbufs;
 	unsigned int num_relocs = args->num_relocs;
-	unsigned int num_waitchks = args->num_waitchks;
 	struct drm_tegra_cmdbuf __user *user_cmdbufs;
 	struct drm_tegra_reloc __user *user_relocs;
-	struct drm_tegra_waitchk __user *user_waitchks;
 	struct drm_tegra_syncpt __user *user_syncpt;
 	struct drm_tegra_syncpt syncpt;
 	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
@@ -364,7 +332,6 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 
 	user_cmdbufs = u64_to_user_ptr(args->cmdbufs);
 	user_relocs = u64_to_user_ptr(args->relocs);
-	user_waitchks = u64_to_user_ptr(args->waitchks);
 	user_syncpt = u64_to_user_ptr(args->syncpts);
 
 	/* We don't yet support other than one syncpt_incr struct per submit */
@@ -376,12 +343,11 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 		return -EINVAL;
 
 	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
-			       args->num_relocs, args->num_waitchks);
+			       args->num_relocs);
 	if (!job)
 		return -ENOMEM;
 
 	job->num_relocs = args->num_relocs;
-	job->num_waitchk = args->num_waitchks;
 	job->client = (u32)args->context;
 	job->class = context->client->base.class;
 	job->serialize = true;
@@ -390,7 +356,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 	 * Track referenced BOs so that they can be unreferenced after the
 	 * submission is complete.
 	 */
-	num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
+	num_refs = num_cmdbufs + num_relocs * 2;
 
 	refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
 	if (!refs) {
@@ -481,30 +447,6 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 		}
 	}
 
-	/* copy and resolve waitchks from submit */
-	while (num_waitchks--) {
-		struct host1x_waitchk *wait = &job->waitchk[num_waitchks];
-		struct tegra_bo *obj;
-
-		err = host1x_waitchk_copy_from_user(
-			wait, &user_waitchks[num_waitchks], file);
-		if (err < 0)
-			goto fail;
-
-		obj = host1x_to_tegra_bo(wait->bo);
-		refs[num_refs++] = &obj->gem;
-
-		/*
-		 * The unaligned offset will cause an unaligned write during
-		 * of the waitchks patching, corrupting the commands stream.
-		 */
-		if (wait->offset & 3 ||
-		    wait->offset >= obj->gem.size) {
-			err = -EINVAL;
-			goto fail;
-		}
-	}
-
 	if (copy_from_user(&syncpt, user_syncpt, sizeof(syncpt))) {
 		err = -EFAULT;
 		goto fail;
diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
index 43e9fabb43a1..36f44ffebe73 100644
--- a/drivers/gpu/host1x/dev.h
+++ b/drivers/gpu/host1x/dev.h
@@ -78,7 +78,6 @@ struct host1x_syncpt_ops {
 	void (*load_wait_base)(struct host1x_syncpt *syncpt);
 	u32 (*load)(struct host1x_syncpt *syncpt);
 	int (*cpu_incr)(struct host1x_syncpt *syncpt);
-	int (*patch_wait)(struct host1x_syncpt *syncpt, void *patch_addr);
 	void (*assign_to_channel)(struct host1x_syncpt *syncpt,
 	                          struct host1x_channel *channel);
 	void (*enable_protection)(struct host1x *host);
@@ -183,13 +182,6 @@ static inline int host1x_hw_syncpt_cpu_incr(struct host1x *host,
 	return host->syncpt_op->cpu_incr(sp);
 }
 
-static inline int host1x_hw_syncpt_patch_wait(struct host1x *host,
-					      struct host1x_syncpt *sp,
-					      void *patch_addr)
-{
-	return host->syncpt_op->patch_wait(sp, patch_addr);
-}
-
 static inline void host1x_hw_syncpt_assign_to_channel(
 	struct host1x *host, struct host1x_syncpt *sp,
 	struct host1x_channel *ch)
diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
index 9af758785a11..4c9555038a95 100644
--- a/drivers/gpu/host1x/hw/channel_hw.c
+++ b/drivers/gpu/host1x/hw/channel_hw.c
@@ -104,8 +104,7 @@ static int channel_submit(struct host1x_job *job)
 	sp = host->syncpt + job->syncpt_id;
 	trace_host1x_channel_submit(dev_name(ch->dev),
 				    job->num_gathers, job->num_relocs,
-				    job->num_waitchk, job->syncpt_id,
-				    job->syncpt_incrs);
+				    job->syncpt_id, job->syncpt_incrs);
 
 	/* before error checks, return current max */
 	prev_max = job->syncpt_end = host1x_syncpt_read_max(sp);
diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c
index 7dfd47d74f89..a23bb3352d02 100644
--- a/drivers/gpu/host1x/hw/syncpt_hw.c
+++ b/drivers/gpu/host1x/hw/syncpt_hw.c
@@ -96,16 +96,6 @@ static int syncpt_cpu_incr(struct host1x_syncpt *sp)
 	return 0;
 }
 
-/* remove a wait pointed to by patch_addr */
-static int syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)
-{
-	u32 override = host1x_class_host_wait_syncpt(HOST1X_SYNCPT_RESERVED, 0);
-
-	*((u32 *)patch_addr) = override;
-
-	return 0;
-}
-
 /**
  * syncpt_assign_to_channel() - Assign syncpoint to channel
  * @sp: syncpoint
@@ -156,7 +146,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = {
 	.load_wait_base = syncpt_read_wait_base,
 	.load = syncpt_load,
 	.cpu_incr = syncpt_cpu_incr,
-	.patch_wait = syncpt_patch_wait,
 	.assign_to_channel = syncpt_assign_to_channel,
 	.enable_protection = syncpt_enable_protection,
 };
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index db509ab8874e..3cbfc6e37668 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -34,8 +34,7 @@
 #define HOST1X_WAIT_SYNCPT_OFFSET 0x8
 
 struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
-				    u32 num_cmdbufs, u32 num_relocs,
-				    u32 num_waitchks)
+				    u32 num_cmdbufs, u32 num_relocs)
 {
 	struct host1x_job *job = NULL;
 	unsigned int num_unpins = num_cmdbufs + num_relocs;
@@ -46,7 +45,6 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
 	total = sizeof(struct host1x_job) +
 		(u64)num_relocs * sizeof(struct host1x_reloc) +
 		(u64)num_unpins * sizeof(struct host1x_job_unpin_data) +
-		(u64)num_waitchks * sizeof(struct host1x_waitchk) +
 		(u64)num_cmdbufs * sizeof(struct host1x_job_gather) +
 		(u64)num_unpins * sizeof(dma_addr_t) +
 		(u64)num_unpins * sizeof(u32 *);
@@ -66,8 +64,6 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
 	mem += num_relocs * sizeof(struct host1x_reloc);
 	job->unpins = num_unpins ? mem : NULL;
 	mem += num_unpins * sizeof(struct host1x_job_unpin_data);
-	job->waitchk = num_waitchks ? mem : NULL;
-	mem += num_waitchks * sizeof(struct host1x_waitchk);
 	job->gathers = num_cmdbufs ? mem : NULL;
 	mem += num_cmdbufs * sizeof(struct host1x_job_gather);
 	job->addr_phys = num_unpins ? mem : NULL;
@@ -111,73 +107,6 @@ void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *bo,
 }
 EXPORT_SYMBOL(host1x_job_add_gather);
 
-/*
- * NULL an already satisfied WAIT_SYNCPT host method, by patching its
- * args in the command stream. The method data is changed to reference
- * a reserved (never given out or incr) HOST1X_SYNCPT_RESERVED syncpt
- * with a matching threshold value of 0, so is guaranteed to be popped
- * by the host HW.
- */
-static void host1x_syncpt_patch_offset(struct host1x_syncpt *sp,
-				       struct host1x_bo *h, u32 offset)
-{
-	void *patch_addr = NULL;
-
-	/* patch the wait */
-	patch_addr = host1x_bo_kmap(h, offset >> PAGE_SHIFT);
-	if (patch_addr) {
-		host1x_syncpt_patch_wait(sp,
-					 patch_addr + (offset & ~PAGE_MASK));
-		host1x_bo_kunmap(h, offset >> PAGE_SHIFT, patch_addr);
-	} else
-		pr_err("Could not map cmdbuf for wait check\n");
-}
-
-/*
- * Check driver supplied waitchk structs for syncpt thresholds
- * that have already been satisfied and NULL the comparison (to
- * avoid a wrap condition in the HW).
- */
-static int do_waitchks(struct host1x_job *job, struct host1x *host,
-		       struct host1x_job_gather *g)
-{
-	struct host1x_bo *patch = g->bo;
-	int i;
-
-	/* compare syncpt vs wait threshold */
-	for (i = 0; i < job->num_waitchk; i++) {
-		struct host1x_waitchk *wait = &job->waitchk[i];
-		struct host1x_syncpt *sp =
-			host1x_syncpt_get(host, wait->syncpt_id);
-
-		/* validate syncpt id */
-		if (wait->syncpt_id > host1x_syncpt_nb_pts(host))
-			continue;
-
-		/* skip all other gathers */
-		if (patch != wait->bo)
-			continue;
-
-		trace_host1x_syncpt_wait_check(wait->bo, wait->offset,
-					       wait->syncpt_id, wait->thresh,
-					       host1x_syncpt_read_min(sp));
-
-		if (host1x_syncpt_is_expired(sp, wait->thresh)) {
-			dev_dbg(host->dev,
-				"drop WAIT id %u (%s) thresh 0x%x, min 0x%x\n",
-				wait->syncpt_id, sp->name, wait->thresh,
-				host1x_syncpt_read_min(sp));
-
-			host1x_syncpt_patch_offset(sp, patch,
-						   g->offset + wait->offset);
-		}
-
-		wait->bo = NULL;
-	}
-
-	return 0;
-}
-
 static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 {
 	unsigned int i;
@@ -331,17 +260,6 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
 	return true;
 }
 
-static bool check_wait(struct host1x_waitchk *wait, struct host1x_bo *cmdbuf,
-		       unsigned int offset)
-{
-	offset *= sizeof(u32);
-
-	if (wait->bo != cmdbuf || wait->offset != offset)
-		return false;
-
-	return true;
-}
-
 struct host1x_firewall {
 	struct host1x_job *job;
 	struct device *dev;
@@ -349,9 +267,6 @@ struct host1x_firewall {
 	unsigned int num_relocs;
 	struct host1x_reloc *reloc;
 
-	unsigned int num_waitchks;
-	struct host1x_waitchk *waitchk;
-
 	struct host1x_bo *cmdbuf;
 	unsigned int offset;
 
@@ -378,20 +293,6 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
 		fw->reloc++;
 	}
 
-	if (offset == HOST1X_WAIT_SYNCPT_OFFSET) {
-		if (fw->class != HOST1X_CLASS_HOST1X)
-			return -EINVAL;
-
-		if (!fw->num_waitchks)
-			return -EINVAL;
-
-		if (!check_wait(fw->waitchk, fw->cmdbuf, fw->offset))
-			return -EINVAL;
-
-		fw->num_waitchks--;
-		fw->waitchk++;
-	}
-
 	return 0;
 }
 
@@ -556,8 +457,6 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
 	fw.dev = dev;
 	fw.reloc = job->relocarray;
 	fw.num_relocs = job->num_relocs;
-	fw.waitchk = job->waitchk;
-	fw.num_waitchks = job->num_waitchk;
 	fw.class = job->class;
 
 	for (i = 0; i < job->num_gathers; i++) {
@@ -604,8 +503,8 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
 		offset += g->words * sizeof(u32);
 	}
 
-	/* No relocs and waitchks should remain at this point */
-	if (fw.num_relocs || fw.num_waitchks)
+	/* No relocs should remain at this point */
+	if (fw.num_relocs)
 		return -EINVAL;
 
 	return 0;
@@ -616,19 +515,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 	int err;
 	unsigned int i, j;
 	struct host1x *host = dev_get_drvdata(dev->parent);
-	DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
-
-	bitmap_zero(waitchk_mask, host1x_syncpt_nb_pts(host));
-	for (i = 0; i < job->num_waitchk; i++) {
-		u32 syncpt_id = job->waitchk[i].syncpt_id;
-
-		if (syncpt_id < host1x_syncpt_nb_pts(host))
-			set_bit(syncpt_id, waitchk_mask);
-	}
-
-	/* get current syncpt values for waitchk */
-	for_each_set_bit(i, waitchk_mask, host1x_syncpt_nb_pts(host))
-		host1x_syncpt_load(host->syncpt + i);
 
 	/* pin memory */
 	err = pin_job(host, job);
@@ -663,10 +549,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 		err = do_relocs(job, g);
 		if (err)
 			break;
-
-		err = do_waitchks(job, host, g);
-		if (err)
-			break;
 	}
 
 out:
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index a2a952adc136..a108669188e8 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -373,12 +373,6 @@ bool host1x_syncpt_is_expired(struct host1x_syncpt *sp, u32 thresh)
 		return (s32)(current_val - thresh) >= 0;
 }
 
-/* remove a wait pointed to by patch_addr */
-int host1x_syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)
-{
-	return host1x_hw_syncpt_patch_wait(sp->host, sp, patch_addr);
-}
-
 int host1x_syncpt_init(struct host1x *host)
 {
 	struct host1x_syncpt_base *bases;
diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h
index 9d88d37c2397..d98e22325e9d 100644
--- a/drivers/gpu/host1x/syncpt.h
+++ b/drivers/gpu/host1x/syncpt.h
@@ -124,7 +124,4 @@ static inline int host1x_syncpt_is_valid(struct host1x_syncpt *sp)
 	return sp->id < host1x_syncpt_nb_pts(sp->host);
 }
 
-/* Patch a wait by replacing it with a wait for syncpt 0 value 0 */
-int host1x_syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr);
-
 #endif
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index ddf7f9ca86cc..f66bece1e1b7 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -192,13 +192,6 @@ struct host1x_reloc {
 	unsigned long shift;
 };
 
-struct host1x_waitchk {
-	struct host1x_bo *bo;
-	u32 offset;
-	u32 syncpt_id;
-	u32 thresh;
-};
-
 struct host1x_job {
 	/* When refcount goes to zero, job can be freed */
 	struct kref ref;
@@ -215,11 +208,6 @@ struct host1x_job {
 	struct host1x_job_gather *gathers;
 	unsigned int num_gathers;
 
-	/* Wait checks to be processed at submit time */
-	struct host1x_waitchk *waitchk;
-	unsigned int num_waitchk;
-	u32 waitchk_mask;
-
 	/* Array of handles to be pinned & unpinned */
 	struct host1x_reloc *relocarray;
 	unsigned int num_relocs;
@@ -261,8 +249,7 @@ struct host1x_job {
 };
 
 struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
-				    u32 num_cmdbufs, u32 num_relocs,
-				    u32 num_waitchks);
+				    u32 num_cmdbufs, u32 num_relocs);
 void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *mem_id,
 			   u32 words, u32 offset);
 struct host1x_job *host1x_job_get(struct host1x_job *job);
diff --git a/include/trace/events/host1x.h b/include/trace/events/host1x.h
index 63116362543c..a37ef73092e5 100644
--- a/include/trace/events/host1x.h
+++ b/include/trace/events/host1x.h
@@ -115,16 +115,15 @@ TRACE_EVENT(host1x_cdma_push_gather,
 );
 
 TRACE_EVENT(host1x_channel_submit,
-	TP_PROTO(const char *name, u32 cmdbufs, u32 relocs, u32 waitchks,
-			u32 syncpt_id, u32 syncpt_incrs),
+	TP_PROTO(const char *name, u32 cmdbufs, u32 relocs, u32 syncpt_id,
+		 u32 syncpt_incrs),
 
-	TP_ARGS(name, cmdbufs, relocs, waitchks, syncpt_id, syncpt_incrs),
+	TP_ARGS(name, cmdbufs, relocs, syncpt_id, syncpt_incrs),
 
 	TP_STRUCT__entry(
 		__field(const char *, name)
 		__field(u32, cmdbufs)
 		__field(u32, relocs)
-		__field(u32, waitchks)
 		__field(u32, syncpt_id)
 		__field(u32, syncpt_incrs)
 	),
@@ -133,15 +132,14 @@ TRACE_EVENT(host1x_channel_submit,
 		__entry->name = name;
 		__entry->cmdbufs = cmdbufs;
 		__entry->relocs = relocs;
-		__entry->waitchks = waitchks;
 		__entry->syncpt_id = syncpt_id;
 		__entry->syncpt_incrs = syncpt_incrs;
 	),
 
-	TP_printk("name=%s, cmdbufs=%u, relocs=%u, waitchks=%d,"
-		"syncpt_id=%u, syncpt_incrs=%u",
-	  __entry->name, __entry->cmdbufs, __entry->relocs, __entry->waitchks,
-	  __entry->syncpt_id, __entry->syncpt_incrs)
+	TP_printk("name=%s, cmdbufs=%u, relocs=%u, syncpt_id=%u, "
+		  "syncpt_incrs=%u",
+		  __entry->name, __entry->cmdbufs, __entry->relocs,
+		  __entry->syncpt_id, __entry->syncpt_incrs)
 );
 
 TRACE_EVENT(host1x_channel_submitted,
-- 
2.17.0

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

[PATCH 2/7] gpu: host1x: Store pointer to client in jobs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Rather than storing some identifier derived from the application
context that can't be used concretely anywhere, store a pointer to the
client directly so that accesses can be made directly through that
client object.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 5 +++--
 drivers/gpu/host1x/cdma.c   | 2 +-
 drivers/gpu/host1x/cdma.h   | 2 +-
 include/linux/host1x.h      | 3 ++-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 33e479182773..e92e44e69871 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -317,6 +317,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_client *client = &context->client->base;
 	unsigned int num_cmdbufs = args->num_cmdbufs;
 	unsigned int num_relocs = args->num_relocs;
 	struct drm_tegra_cmdbuf __user *user_cmdbufs;
@@ -348,8 +349,8 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 		return -ENOMEM;
 
 	job->num_relocs = args->num_relocs;
-	job->client = (u32)args->context;
-	job->class = context->client->base.class;
+	job->client = client;
+	job->class = client->class;
 	job->serialize = true;
 
 	/*
diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
index 28541b280739..cb590ba886ec 100644
--- a/drivers/gpu/host1x/cdma.c
+++ b/drivers/gpu/host1x/cdma.c
@@ -247,7 +247,7 @@ static void cdma_start_timer_locked(struct host1x_cdma *cdma,
 static void stop_cdma_timer_locked(struct host1x_cdma *cdma)
 {
 	cancel_delayed_work(&cdma->timeout.wq);
-	cdma->timeout.client = 0;
+	cdma->timeout.client = NULL;
 }
 
 /*
diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h
index 286d49386be9..1825091bc0ed 100644
--- a/drivers/gpu/host1x/cdma.h
+++ b/drivers/gpu/host1x/cdma.h
@@ -58,7 +58,7 @@ struct buffer_timeout {
 	u32 syncpt_val;			/* syncpt value when completed */
 	ktime_t start_ktime;		/* starting time */
 	/* context timeout information */
-	int client;
+	struct host1x_client *client;
 };
 
 enum cdma_event {
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index f66bece1e1b7..0632010f47fb 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -202,7 +202,8 @@ struct host1x_job {
 	/* Channel where job is submitted to */
 	struct host1x_channel *channel;
 
-	u32 client;
+	/* client where the job originated */
+	struct host1x_client *client;
 
 	/* Gathers and their memory */
 	struct host1x_job_gather *gathers;
-- 
2.17.0

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

[PATCH 3/7] gpu: host1x: Cleanup loop variable usage

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use unsigned int where possible and don't unnecessarily initialize the
loop variable.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/host1x/debug.c  | 2 +-
 drivers/gpu/host1x/intr.c   | 2 +-
 drivers/gpu/host1x/job.c    | 4 ++--
 drivers/gpu/host1x/syncpt.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
index dc77ec452ffc..329e4a3d8ae7 100644
--- a/drivers/gpu/host1x/debug.c
+++ b/drivers/gpu/host1x/debug.c
@@ -103,7 +103,7 @@ static void show_syncpts(struct host1x *m, struct output *o)
 
 static void show_all(struct host1x *m, struct output *o, bool show_fifo)
 {
-	int i;
+	unsigned int i;
 
 	host1x_hw_show_mlocks(m, o);
 	show_syncpts(m, o);
diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
index 8b4fad0ab35d..6028cf7b681f 100644
--- a/drivers/gpu/host1x/intr.c
+++ b/drivers/gpu/host1x/intr.c
@@ -144,7 +144,7 @@ static const action_handler action_handlers[HOST1X_INTR_ACTION_COUNT] = {
 static void run_handlers(struct list_head completed[HOST1X_INTR_ACTION_COUNT])
 {
 	struct list_head *head = completed;
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < HOST1X_INTR_ACTION_COUNT; ++i, ++head) {
 		action_handler handler = action_handlers[i];
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index 3cbfc6e37668..2be0bcaf8288 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -196,10 +196,10 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 
 static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
 {
-	int i = 0;
 	u32 last_page = ~0;
 	void *cmdbuf_page_addr = NULL;
 	struct host1x_bo *cmdbuf = g->bo;
+	unsigned int i;
 
 	/* pin & patch the relocs for one gather */
 	for (i = 0; i < job->num_relocs; i++) {
@@ -451,7 +451,7 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
 	struct host1x_firewall fw;
 	size_t size = 0;
 	size_t offset = 0;
-	int i;
+	unsigned int i;
 
 	fw.job = job;
 	fw.dev = dev;
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index a108669188e8..088c05dd884c 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -57,8 +57,8 @@ static struct host1x_syncpt *host1x_syncpt_alloc(struct host1x *host,
 						 struct host1x_client *client,
 						 unsigned long flags)
 {
-	int i;
 	struct host1x_syncpt *sp = host->syncpt;
+	unsigned int i;
 	char *name;
 
 	mutex_lock(&host->syncpt_mutex);
-- 
2.17.0

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

[PATCH 4/7] gpu: host1x: Drop unnecessary host1x argument

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Functions taking a pointer to a host1x syncpoint as an argument don't
need to specify a pointer to a host1x instance because it can be
obtained from the syncpoint.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/host1x/hw/channel_hw.c |  2 +-
 drivers/gpu/host1x/intr.c          | 14 ++++++--------
 drivers/gpu/host1x/intr.h          |  8 +++++---
 drivers/gpu/host1x/syncpt.c        |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
index 4c9555038a95..d188f9068b91 100644
--- a/drivers/gpu/host1x/hw/channel_hw.c
+++ b/drivers/gpu/host1x/hw/channel_hw.c
@@ -164,7 +164,7 @@ static int channel_submit(struct host1x_job *job)
 	trace_host1x_channel_submitted(dev_name(ch->dev), prev_max, syncval);
 
 	/* schedule a submit complete interrupt */
-	err = host1x_intr_add_action(host, job->syncpt_id, syncval,
+	err = host1x_intr_add_action(host, sp, syncval,
 				     HOST1X_INTR_ACTION_SUBMIT_COMPLETE, ch,
 				     completed_waiter, NULL);
 	completed_waiter = NULL;
diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
index 6028cf7b681f..9629c009d10f 100644
--- a/drivers/gpu/host1x/intr.c
+++ b/drivers/gpu/host1x/intr.c
@@ -211,11 +211,11 @@ static void syncpt_thresh_work(struct work_struct *work)
 				host1x_syncpt_load(host->syncpt + id));
 }
 
-int host1x_intr_add_action(struct host1x *host, unsigned int id, u32 thresh,
-			   enum host1x_intr_action action, void *data,
-			   struct host1x_waitlist *waiter, void **ref)
+int host1x_intr_add_action(struct host1x *host, struct host1x_syncpt *syncpt,
+			   u32 thresh, enum host1x_intr_action action,
+			   void *data, struct host1x_waitlist *waiter,
+			   void **ref)
 {
-	struct host1x_syncpt *syncpt;
 	int queue_was_empty;
 
 	if (waiter == NULL) {
@@ -234,19 +234,17 @@ int host1x_intr_add_action(struct host1x *host, unsigned int id, u32 thresh,
 	waiter->data = data;
 	waiter->count = 1;
 
-	syncpt = host->syncpt + id;
-
 	spin_lock(&syncpt->intr.lock);
 
 	queue_was_empty = list_empty(&syncpt->intr.wait_head);
 
 	if (add_waiter_to_queue(waiter, &syncpt->intr.wait_head)) {
 		/* added at head of list - new threshold value */
-		host1x_hw_intr_set_syncpt_threshold(host, id, thresh);
+		host1x_hw_intr_set_syncpt_threshold(host, syncpt->id, thresh);
 
 		/* added as first waiter - enable interrupt */
 		if (queue_was_empty)
-			host1x_hw_intr_enable_syncpt_intr(host, id);
+			host1x_hw_intr_enable_syncpt_intr(host, syncpt->id);
 	}
 
 	spin_unlock(&syncpt->intr.lock);
diff --git a/drivers/gpu/host1x/intr.h b/drivers/gpu/host1x/intr.h
index 1370c2bb75b8..6db96af484fe 100644
--- a/drivers/gpu/host1x/intr.h
+++ b/drivers/gpu/host1x/intr.h
@@ -22,6 +22,7 @@
 #include <linux/interrupt.h>
 #include <linux/workqueue.h>
 
+struct host1x_syncpt;
 struct host1x;
 
 enum host1x_intr_action {
@@ -75,9 +76,10 @@ struct host1x_waitlist {
  *
  * This is a non-blocking api.
  */
-int host1x_intr_add_action(struct host1x *host, unsigned int id, u32 thresh,
-	enum host1x_intr_action action, void *data,
-	struct host1x_waitlist *waiter, void **ref);
+int host1x_intr_add_action(struct host1x *host, struct host1x_syncpt *syncpt,
+			   u32 thresh, enum host1x_intr_action action,
+			   void *data, struct host1x_waitlist *waiter,
+			   void **ref);
 
 /*
  * Unreference an action submitted to host1x_intr_add_action().
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 088c05dd884c..a5dbf1ba4645 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -255,7 +255,7 @@ int host1x_syncpt_wait(struct host1x_syncpt *sp, u32 thresh, long timeout,
 	}
 
 	/* schedule a wakeup when the syncpoint value is reached */
-	err = host1x_intr_add_action(sp->host, sp->id, thresh,
+	err = host1x_intr_add_action(sp->host, sp, thresh,
 				     HOST1X_INTR_ACTION_WAKEUP_INTERRUPTIBLE,
 				     &wq, waiter, &ref);
 	if (err)
-- 
2.17.0

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

[PATCH 5/7] gpu: host1x: Rename relocarray -> relocs for consistency

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

All other array variables use a plural, and this is the only one using
the *array suffix. This is confusing, so rename it for consistency.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 4 ++--
 drivers/gpu/host1x/job.c    | 8 ++++----
 include/linux/host1x.h      | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index e92e44e69871..f9d6c059792f 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -418,13 +418,13 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 		struct host1x_reloc *reloc;
 		struct tegra_bo *obj;
 
-		err = host1x_reloc_copy_from_user(&job->relocarray[num_relocs],
+		err = host1x_reloc_copy_from_user(&job->relocs[num_relocs],
 						  &user_relocs[num_relocs], drm,
 						  file);
 		if (err < 0)
 			goto fail;
 
-		reloc = &job->relocarray[num_relocs];
+		reloc = &job->relocs[num_relocs];
 		obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
 		refs[num_refs++] = &obj->gem;
 
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index 2be0bcaf8288..9d6d3e151291 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -60,7 +60,7 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
 
 	/* Redistribute memory to the structs  */
 	mem += sizeof(struct host1x_job);
-	job->relocarray = num_relocs ? mem : NULL;
+	job->relocs = num_relocs ? mem : NULL;
 	mem += num_relocs * sizeof(struct host1x_reloc);
 	job->unpins = num_unpins ? mem : NULL;
 	mem += num_unpins * sizeof(struct host1x_job_unpin_data);
@@ -115,7 +115,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 	job->num_unpins = 0;
 
 	for (i = 0; i < job->num_relocs; i++) {
-		struct host1x_reloc *reloc = &job->relocarray[i];
+		struct host1x_reloc *reloc = &job->relocs[i];
 		struct sg_table *sgt;
 		dma_addr_t phys_addr;
 
@@ -203,7 +203,7 @@ static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
 
 	/* pin & patch the relocs for one gather */
 	for (i = 0; i < job->num_relocs; i++) {
-		struct host1x_reloc *reloc = &job->relocarray[i];
+		struct host1x_reloc *reloc = &job->relocs[i];
 		u32 reloc_addr = (job->reloc_addr_phys[i] +
 				  reloc->target.offset) >> reloc->shift;
 		u32 *target;
@@ -455,7 +455,7 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
 
 	fw.job = job;
 	fw.dev = dev;
-	fw.reloc = job->relocarray;
+	fw.reloc = job->relocs;
 	fw.num_relocs = job->num_relocs;
 	fw.class = job->class;
 
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 0632010f47fb..dcb6140d39d7 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -210,7 +210,7 @@ struct host1x_job {
 	unsigned int num_gathers;
 
 	/* Array of handles to be pinned & unpinned */
-	struct host1x_reloc *relocarray;
+	struct host1x_reloc *relocs;
 	unsigned int num_relocs;
 	struct host1x_job_unpin_data *unpins;
 	unsigned int num_unpins;
-- 
2.17.0

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

[PATCH 6/7] gpu: host1x: Use not explicitly sized types

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The number of words and the offset in a gather don't need to be
explicitly sized, so make them unsigned int instead.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/host1x/job.c | 13 ++++++++-----
 drivers/gpu/host1x/job.h |  4 ++--
 include/linux/host1x.h   |  4 ++--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index 9d6d3e151291..1a8344dbdb2b 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -96,13 +96,16 @@ void host1x_job_put(struct host1x_job *job)
 EXPORT_SYMBOL(host1x_job_put);
 
 void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *bo,
-			   u32 words, u32 offset)
+			   unsigned int words, unsigned int offset)
 {
-	struct host1x_job_gather *cur_gather = &job->gathers[job->num_gathers];
+	struct host1x_job_gather *gather = &job->gathers[job->num_gathers];
+
+	WARN_ON(job->num_gathers >= job->num_cmdbufs);
+
+	gather->words = words;
+	gather->bo = bo;
+	gather->offset = offset;
 
-	cur_gather->words = words;
-	cur_gather->bo = bo;
-	cur_gather->offset = offset;
 	job->num_gathers++;
 }
 EXPORT_SYMBOL(host1x_job_add_gather);
diff --git a/drivers/gpu/host1x/job.h b/drivers/gpu/host1x/job.h
index 4bda51d503ec..188400e00192 100644
--- a/drivers/gpu/host1x/job.h
+++ b/drivers/gpu/host1x/job.h
@@ -20,10 +20,10 @@
 #define __HOST1X_JOB_H
 
 struct host1x_job_gather {
-	u32 words;
+	unsigned int words;
 	dma_addr_t base;
 	struct host1x_bo *bo;
-	u32 offset;
+	unsigned int offset;
 	bool handled;
 };
 
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index dcb6140d39d7..89110d896d72 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -251,8 +251,8 @@ struct host1x_job {
 
 struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
 				    u32 num_cmdbufs, u32 num_relocs);
-void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *mem_id,
-			   u32 words, u32 offset);
+void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *bo,
+			   unsigned int words, unsigned int offset);
 struct host1x_job *host1x_job_get(struct host1x_job *job);
 void host1x_job_put(struct host1x_job *job);
 int host1x_job_pin(struct host1x_job *job, struct device *dev);
-- 
2.17.0

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

[PATCH 7/7] gpu: host1x: Track client version

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Userspace needs to know the version of the interface implemented by a
client so it can create the proper command streams. Allow individual
drivers to store this version along with the client so that it can be
returned to userspace upon opening a channel.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/linux/host1x.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 89110d896d72..57d26406bdfd 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -49,6 +49,7 @@ struct host1x_client_ops {
  * @dev: pointer to struct device backing this host1x client
  * @ops: host1x client operations
  * @class: host1x class represented by this client
+ * @version: interface version implemented by this client
  * @channel: host1x channel associated with this client
  * @syncpts: array of syncpoints requested for this client
  * @num_syncpts: number of syncpoints requested for this client
@@ -61,6 +62,8 @@ struct host1x_client {
 	const struct host1x_client_ops *ops;
 
 	enum host1x_class class;
+	unsigned int version;
+
 	struct host1x_channel *channel;
 
 	struct host1x_syncpt **syncpts;
-- 
2.17.0

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

Re: [PATCH 7/7] gpu: host1x: Track client version

[Mikko Perttunen]

On 05/17/2018 06:34 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Userspace needs to know the version of the interface implemented by a
> client so it can create the proper command streams. Allow individual
> drivers to store this version along with the client so that it can be
> returned to userspace upon opening a channel.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   include/linux/host1x.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 89110d896d72..57d26406bdfd 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -49,6 +49,7 @@ struct host1x_client_ops {
>    * @dev: pointer to struct device backing this host1x client
>    * @ops: host1x client operations
>    * @class: host1x class represented by this client
> + * @version: interface version implemented by this client
>    * @channel: host1x channel associated with this client
>    * @syncpts: array of syncpoints requested for this client
>    * @num_syncpts: number of syncpoints requested for this client
> @@ -61,6 +62,8 @@ struct host1x_client {
>   	const struct host1x_client_ops *ops;
>   
>   	enum host1x_class class;
> +	unsigned int version;
> +

It doesn't seem to me that this fits here - Host1x doesn't provide any 
userspace interface, TegraDRM does. We will (hopefully) have clients in 
the future that use a different userspace interface, or don't have one 
at all. So this property should be on TegraDRM side instead.

Mikko

>   	struct host1x_channel *channel;
>   
>   	struct host1x_syncpt **syncpts;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] gpu: host1x: Track client version

[Thierry Reding]

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

On Fri, May 18, 2018 at 03:21:11PM +0300, Mikko Perttunen wrote:
> On 05/17/2018 06:34 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Userspace needs to know the version of the interface implemented by a
> > client so it can create the proper command streams. Allow individual
> > drivers to store this version along with the client so that it can be
> > returned to userspace upon opening a channel.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   include/linux/host1x.h | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> > index 89110d896d72..57d26406bdfd 100644
> > --- a/include/linux/host1x.h
> > +++ b/include/linux/host1x.h
> > @@ -49,6 +49,7 @@ struct host1x_client_ops {
> >    * @dev: pointer to struct device backing this host1x client
> >    * @ops: host1x client operations
> >    * @class: host1x class represented by this client
> > + * @version: interface version implemented by this client
> >    * @channel: host1x channel associated with this client
> >    * @syncpts: array of syncpoints requested for this client
> >    * @num_syncpts: number of syncpoints requested for this client
> > @@ -61,6 +62,8 @@ struct host1x_client {
> >   	const struct host1x_client_ops *ops;
> >   	enum host1x_class class;
> > +	unsigned int version;
> > +
> 
> It doesn't seem to me that this fits here - Host1x doesn't provide any
> userspace interface, TegraDRM does. We will (hopefully) have clients in the
> future that use a different userspace interface, or don't have one at all.
> So this property should be on TegraDRM side instead.

I think the line is somewhat blurry. Technically it is the host1x
clients that define the version of the interface that they provide. By
interface I mean the "class" of the command stream supported. So gr2d
will define what methods are valid to use on a specific version of the
module. The same goes for gr3d and VIC. The hardware module is where
that specification lives.

So if we ever had a client that didn't provide a userspace interface via
Tegra DRM we'd still want to represent what version of the (command
stream) interface is expected. By moving this to drm/tegra, we move the
hardware specific bits into the userspace related parts, so I'm not sure
it is a really good fit.

However, I do understand where you're coming from. Ultimately where
userspace gets involved is at the ABI level that drm/tegra exposes. And
for drivers that don't expose a userspace ABI, they'd likely need to
deal with the version number internally, so storing this in host1x
clients wouldn't fit very well either.

I don't really care all that much how exactly we store the version
information so that it can be reported back to userspace, and moving it
to drm/tegra makes the code somewhat more elegant. I came up with the
below, which is completely untested but should work.

Thierry

--- >8 ---
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 4330d5d48801..5dc02340007e 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1389,7 +1389,7 @@ static int tegra_open_channel(struct drm_device *drm, void *data,
 				break;
 
 			args->syncpts = client->base.num_syncpts;
-			args->version = client->base.version;
+			args->version = client->version;
 			args->context = context->id;
 			break;
 		}
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 13660f6225a2..f493e0bf2172 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -105,6 +105,7 @@ struct tegra_drm_client {
 	struct host1x_client base;
 	struct list_head list;
 
+	unsigned int version;
 	const struct tegra_drm_client_ops *ops;
 };
 
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index 7cae8252f0fa..165a1cece039 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -196,11 +196,11 @@ static int gr2d_probe(struct platform_device *pdev)
 	gr2d->client.base.ops = &gr2d_client_ops;
 	gr2d->client.base.dev = dev;
 	gr2d->client.base.class = HOST1X_CLASS_GR2D;
-	gr2d->client.base.version = gr2d->soc->version;
 	gr2d->client.base.syncpts = syncpts;
 	gr2d->client.base.num_syncpts = 1;
 
 	INIT_LIST_HEAD(&gr2d->client.list);
+	gr2d->client.version = gr2d->soc->version;
 	gr2d->client.ops = &gr2d_ops;
 
 	err = host1x_client_register(&gr2d->client.base);
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 9bd89916d2ef..0d72e78bd6e8 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -325,11 +325,11 @@ static int gr3d_probe(struct platform_device *pdev)
 	gr3d->client.base.ops = &gr3d_client_ops;
 	gr3d->client.base.dev = &pdev->dev;
 	gr3d->client.base.class = HOST1X_CLASS_GR3D;
-	gr3d->client.base.version = gr3d->soc->version;
 	gr3d->client.base.syncpts = syncpts;
 	gr3d->client.base.num_syncpts = 1;
 
 	INIT_LIST_HEAD(&gr3d->client.list);
+	gr3d->client.version = gr3d->soc->version;
 	gr3d->client.ops = &gr3d_ops;
 
 	err = host1x_client_register(&gr3d->client.base);
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
index b9df467ca71e..9fa77405db01 100644
--- a/drivers/gpu/drm/tegra/vic.c
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -342,12 +342,12 @@ static int vic_probe(struct platform_device *pdev)
 	vic->client.base.ops = &vic_client_ops;
 	vic->client.base.dev = dev;
 	vic->client.base.class = HOST1X_CLASS_VIC;
-	vic->client.base.version = vic->config->version;
 	vic->client.base.syncpts = syncpts;
 	vic->client.base.num_syncpts = 1;
 	vic->dev = dev;
 
 	INIT_LIST_HEAD(&vic->client.list);
+	vic->client.version = vic->config->version;
 	vic->client.ops = &vic_ops;
 
 	err = host1x_client_register(&vic->client.base);
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 6dc819b1ab64..518864cfbd16 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -62,7 +62,6 @@ struct host1x_client {
 	const struct host1x_client_ops *ops;
 
 	enum host1x_class class;
-	unsigned int version;
 
 	struct host1x_channel *channel;
 

[-- 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

Re: [PATCH 7/7] gpu: host1x: Track client version

[Mikko Perttunen]

On 05/18/2018 03:39 PM, Thierry Reding wrote:
> On Fri, May 18, 2018 at 03:21:11PM +0300, Mikko Perttunen wrote:
>> On 05/17/2018 06:34 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Userspace needs to know the version of the interface implemented by a
>>> client so it can create the proper command streams. Allow individual
>>> drivers to store this version along with the client so that it can be
>>> returned to userspace upon opening a channel.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>    include/linux/host1x.h | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
>>> index 89110d896d72..57d26406bdfd 100644
>>> --- a/include/linux/host1x.h
>>> +++ b/include/linux/host1x.h
>>> @@ -49,6 +49,7 @@ struct host1x_client_ops {
>>>     * @dev: pointer to struct device backing this host1x client
>>>     * @ops: host1x client operations
>>>     * @class: host1x class represented by this client
>>> + * @version: interface version implemented by this client
>>>     * @channel: host1x channel associated with this client
>>>     * @syncpts: array of syncpoints requested for this client
>>>     * @num_syncpts: number of syncpoints requested for this client
>>> @@ -61,6 +62,8 @@ struct host1x_client {
>>>    	const struct host1x_client_ops *ops;
>>>    	enum host1x_class class;
>>> +	unsigned int version;
>>> +
>>
>> It doesn't seem to me that this fits here - Host1x doesn't provide any
>> userspace interface, TegraDRM does. We will (hopefully) have clients in the
>> future that use a different userspace interface, or don't have one at all.
>> So this property should be on TegraDRM side instead.
> 
> I think the line is somewhat blurry. Technically it is the host1x
> clients that define the version of the interface that they provide. By
> interface I mean the "class" of the command stream supported. So gr2d
> will define what methods are valid to use on a specific version of the
> module. The same goes for gr3d and VIC. The hardware module is where
> that specification lives.
> 
> So if we ever had a client that didn't provide a userspace interface via
> Tegra DRM we'd still want to represent what version of the (command
> stream) interface is expected. By moving this to drm/tegra, we move the
> hardware specific bits into the userspace related parts, so I'm not sure
> it is a really good fit.

The way I look at it, TegraDRM contains the engine driver 
implementations and should contain all knowledge about those - Host1x 
should know as little as possible about the engines, and as it doesn't 
need to know the interface version, we should keep that information on 
the TegraDRM side. As a comparison, we could think of Host1x as a PCI 
host controller and the engines as connected devices.

> 
> However, I do understand where you're coming from. Ultimately where
> userspace gets involved is at the ABI level that drm/tegra exposes. And
> for drivers that don't expose a userspace ABI, they'd likely need to
> deal with the version number internally, so storing this in host1x
> clients wouldn't fit very well either.
> 
> I don't really care all that much how exactly we store the version
> information so that it can be reported back to userspace, and moving it
> to drm/tegra makes the code somewhat more elegant. I came up with the
> below, which is completely untested but should work.

Yep, this looks good to me.

Thanks!
Mikko

> 
> Thierry
> 
> --- >8 ---
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 4330d5d48801..5dc02340007e 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1389,7 +1389,7 @@ static int tegra_open_channel(struct drm_device *drm, void *data,
>   				break;
>   
>   			args->syncpts = client->base.num_syncpts;
> -			args->version = client->base.version;
> +			args->version = client->version;
>   			args->context = context->id;
>   			break;
>   		}
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 13660f6225a2..f493e0bf2172 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -105,6 +105,7 @@ struct tegra_drm_client {
>   	struct host1x_client base;
>   	struct list_head list;
>   
> +	unsigned int version;
>   	const struct tegra_drm_client_ops *ops;
>   };
>   
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index 7cae8252f0fa..165a1cece039 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -196,11 +196,11 @@ static int gr2d_probe(struct platform_device *pdev)
>   	gr2d->client.base.ops = &gr2d_client_ops;
>   	gr2d->client.base.dev = dev;
>   	gr2d->client.base.class = HOST1X_CLASS_GR2D;
> -	gr2d->client.base.version = gr2d->soc->version;
>   	gr2d->client.base.syncpts = syncpts;
>   	gr2d->client.base.num_syncpts = 1;
>   
>   	INIT_LIST_HEAD(&gr2d->client.list);
> +	gr2d->client.version = gr2d->soc->version;
>   	gr2d->client.ops = &gr2d_ops;
>   
>   	err = host1x_client_register(&gr2d->client.base);
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 9bd89916d2ef..0d72e78bd6e8 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -325,11 +325,11 @@ static int gr3d_probe(struct platform_device *pdev)
>   	gr3d->client.base.ops = &gr3d_client_ops;
>   	gr3d->client.base.dev = &pdev->dev;
>   	gr3d->client.base.class = HOST1X_CLASS_GR3D;
> -	gr3d->client.base.version = gr3d->soc->version;
>   	gr3d->client.base.syncpts = syncpts;
>   	gr3d->client.base.num_syncpts = 1;
>   
>   	INIT_LIST_HEAD(&gr3d->client.list);
> +	gr3d->client.version = gr3d->soc->version;
>   	gr3d->client.ops = &gr3d_ops;
>   
>   	err = host1x_client_register(&gr3d->client.base);
> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> index b9df467ca71e..9fa77405db01 100644
> --- a/drivers/gpu/drm/tegra/vic.c
> +++ b/drivers/gpu/drm/tegra/vic.c
> @@ -342,12 +342,12 @@ static int vic_probe(struct platform_device *pdev)
>   	vic->client.base.ops = &vic_client_ops;
>   	vic->client.base.dev = dev;
>   	vic->client.base.class = HOST1X_CLASS_VIC;
> -	vic->client.base.version = vic->config->version;
>   	vic->client.base.syncpts = syncpts;
>   	vic->client.base.num_syncpts = 1;
>   	vic->dev = dev;
>   
>   	INIT_LIST_HEAD(&vic->client.list);
> +	vic->client.version = vic->config->version;
>   	vic->client.ops = &vic_ops;
>   
>   	err = host1x_client_register(&vic->client.base);
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 6dc819b1ab64..518864cfbd16 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -62,7 +62,6 @@ struct host1x_client {
>   	const struct host1x_client_ops *ops;
>   
>   	enum host1x_class class;
> -	unsigned int version;
>   
>   	struct host1x_channel *channel;
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] gpu: host1x: Remove wait check support

[Dmitry Osipenko]

On 17.05.2018 18:34, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The job submission userspace ABI doesn't support this and there are no
> plans to implement it, so all of this code is dead and can be removed.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c        |  62 +--------------
>  drivers/gpu/host1x/dev.h           |   8 --
>  drivers/gpu/host1x/hw/channel_hw.c |   3 +-
>  drivers/gpu/host1x/hw/syncpt_hw.c  |  11 ---
>  drivers/gpu/host1x/job.c           | 124 +----------------------------
>  drivers/gpu/host1x/syncpt.c        |   6 --
>  drivers/gpu/host1x/syncpt.h        |   3 -
>  include/linux/host1x.h             |  15 +---
>  include/trace/events/host1x.h      |  16 ++--
>  9 files changed, 14 insertions(+), 234 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 7afe2f635f74..33e479182773 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -313,46 +313,14 @@ static int host1x_reloc_copy_from_user(struct host1x_reloc *dest,
>  	return 0;
>  }
>  
> -static int host1x_waitchk_copy_from_user(struct host1x_waitchk *dest,
> -					 struct drm_tegra_waitchk __user *src,
> -					 struct drm_file *file)
> -{
> -	u32 cmdbuf;
> -	int err;
> -
> -	err = get_user(cmdbuf, &src->handle);
> -	if (err < 0)
> -		return err;
> -
> -	err = get_user(dest->offset, &src->offset);
> -	if (err < 0)
> -		return err;
> -
> -	err = get_user(dest->syncpt_id, &src->syncpt);
> -	if (err < 0)
> -		return err;
> -
> -	err = get_user(dest->thresh, &src->thresh);
> -	if (err < 0)
> -		return err;
> -
> -	dest->bo = host1x_bo_lookup(file, cmdbuf);
> -	if (!dest->bo)
> -		return -ENOENT;
> -
> -	return 0;
> -}
> -
>  int tegra_drm_submit(struct tegra_drm_context *context,
>  		     struct drm_tegra_submit *args, struct drm_device *drm,
>  		     struct drm_file *file)
>  {
>  	unsigned int num_cmdbufs = args->num_cmdbufs;
>  	unsigned int num_relocs = args->num_relocs;
> -	unsigned int num_waitchks = args->num_waitchks;
>  	struct drm_tegra_cmdbuf __user *user_cmdbufs;
>  	struct drm_tegra_reloc __user *user_relocs;
> -	struct drm_tegra_waitchk __user *user_waitchks;
>  	struct drm_tegra_syncpt __user *user_syncpt;
>  	struct drm_tegra_syncpt syncpt;
>  	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
> @@ -364,7 +332,6 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  
>  	user_cmdbufs = u64_to_user_ptr(args->cmdbufs);
>  	user_relocs = u64_to_user_ptr(args->relocs);
> -	user_waitchks = u64_to_user_ptr(args->waitchks);
>  	user_syncpt = u64_to_user_ptr(args->syncpts);
>  
>  	/* We don't yet support other than one syncpt_incr struct per submit */
> @@ -376,12 +343,11 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  		return -EINVAL;
>  
>  	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
> -			       args->num_relocs, args->num_waitchks);
> +			       args->num_relocs);
>  	if (!job)
>  		return -ENOMEM;
>  
>  	job->num_relocs = args->num_relocs;
> -	job->num_waitchk = args->num_waitchks;
>  	job->client = (u32)args->context;
>  	job->class = context->client->base.class;
>  	job->serialize = true;
> @@ -390,7 +356,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  	 * Track referenced BOs so that they can be unreferenced after the
>  	 * submission is complete.
>  	 */
> -	num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
> +	num_refs = num_cmdbufs + num_relocs * 2;
>  
>  	refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
>  	if (!refs) {
> @@ -481,30 +447,6 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  		}
>  	}
>  
> -	/* copy and resolve waitchks from submit */
> -	while (num_waitchks--) {
> -		struct host1x_waitchk *wait = &job->waitchk[num_waitchks];
> -		struct tegra_bo *obj;
> -
> -		err = host1x_waitchk_copy_from_user(
> -			wait, &user_waitchks[num_waitchks], file);
> -		if (err < 0)
> -			goto fail;
> -
> -		obj = host1x_to_tegra_bo(wait->bo);
> -		refs[num_refs++] = &obj->gem;
> -
> -		/*
> -		 * The unaligned offset will cause an unaligned write during
> -		 * of the waitchks patching, corrupting the commands stream.
> -		 */
> -		if (wait->offset & 3 ||
> -		    wait->offset >= obj->gem.size) {
> -			err = -EINVAL;
> -			goto fail;
> -		}
> -	}
> -
>  	if (copy_from_user(&syncpt, user_syncpt, sizeof(syncpt))) {
>  		err = -EFAULT;
>  		goto fail;
> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
> index 43e9fabb43a1..36f44ffebe73 100644
> --- a/drivers/gpu/host1x/dev.h
> +++ b/drivers/gpu/host1x/dev.h
> @@ -78,7 +78,6 @@ struct host1x_syncpt_ops {
>  	void (*load_wait_base)(struct host1x_syncpt *syncpt);
>  	u32 (*load)(struct host1x_syncpt *syncpt);
>  	int (*cpu_incr)(struct host1x_syncpt *syncpt);
> -	int (*patch_wait)(struct host1x_syncpt *syncpt, void *patch_addr);
>  	void (*assign_to_channel)(struct host1x_syncpt *syncpt,
>  	                          struct host1x_channel *channel);
>  	void (*enable_protection)(struct host1x *host);
> @@ -183,13 +182,6 @@ static inline int host1x_hw_syncpt_cpu_incr(struct host1x *host,
>  	return host->syncpt_op->cpu_incr(sp);
>  }
>  
> -static inline int host1x_hw_syncpt_patch_wait(struct host1x *host,
> -					      struct host1x_syncpt *sp,
> -					      void *patch_addr)
> -{
> -	return host->syncpt_op->patch_wait(sp, patch_addr);
> -}
> -
>  static inline void host1x_hw_syncpt_assign_to_channel(
>  	struct host1x *host, struct host1x_syncpt *sp,
>  	struct host1x_channel *ch)
> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
> index 9af758785a11..4c9555038a95 100644
> --- a/drivers/gpu/host1x/hw/channel_hw.c
> +++ b/drivers/gpu/host1x/hw/channel_hw.c
> @@ -104,8 +104,7 @@ static int channel_submit(struct host1x_job *job)
>  	sp = host->syncpt + job->syncpt_id;
>  	trace_host1x_channel_submit(dev_name(ch->dev),
>  				    job->num_gathers, job->num_relocs,
> -				    job->num_waitchk, job->syncpt_id,
> -				    job->syncpt_incrs);
> +				    job->syncpt_id, job->syncpt_incrs);
>  
>  	/* before error checks, return current max */
>  	prev_max = job->syncpt_end = host1x_syncpt_read_max(sp);
> diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c
> index 7dfd47d74f89..a23bb3352d02 100644
> --- a/drivers/gpu/host1x/hw/syncpt_hw.c
> +++ b/drivers/gpu/host1x/hw/syncpt_hw.c
> @@ -96,16 +96,6 @@ static int syncpt_cpu_incr(struct host1x_syncpt *sp)
>  	return 0;
>  }
>  
> -/* remove a wait pointed to by patch_addr */
> -static int syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)
> -{
> -	u32 override = host1x_class_host_wait_syncpt(HOST1X_SYNCPT_RESERVED, 0);
> -
> -	*((u32 *)patch_addr) = override;
> -
> -	return 0;
> -}
> -
>  /**
>   * syncpt_assign_to_channel() - Assign syncpoint to channel
>   * @sp: syncpoint
> @@ -156,7 +146,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = {
>  	.load_wait_base = syncpt_read_wait_base,
>  	.load = syncpt_load,
>  	.cpu_incr = syncpt_cpu_incr,
> -	.patch_wait = syncpt_patch_wait,
>  	.assign_to_channel = syncpt_assign_to_channel,
>  	.enable_protection = syncpt_enable_protection,
>  };
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index db509ab8874e..3cbfc6e37668 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -34,8 +34,7 @@
>  #define HOST1X_WAIT_SYNCPT_OFFSET 0x8
>  
>  struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
> -				    u32 num_cmdbufs, u32 num_relocs,
> -				    u32 num_waitchks)
> +				    u32 num_cmdbufs, u32 num_relocs)
>  {
>  	struct host1x_job *job = NULL;
>  	unsigned int num_unpins = num_cmdbufs + num_relocs;
> @@ -46,7 +45,6 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
>  	total = sizeof(struct host1x_job) +
>  		(u64)num_relocs * sizeof(struct host1x_reloc) +
>  		(u64)num_unpins * sizeof(struct host1x_job_unpin_data) +
> -		(u64)num_waitchks * sizeof(struct host1x_waitchk) +
>  		(u64)num_cmdbufs * sizeof(struct host1x_job_gather) +
>  		(u64)num_unpins * sizeof(dma_addr_t) +
>  		(u64)num_unpins * sizeof(u32 *);
> @@ -66,8 +64,6 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
>  	mem += num_relocs * sizeof(struct host1x_reloc);
>  	job->unpins = num_unpins ? mem : NULL;
>  	mem += num_unpins * sizeof(struct host1x_job_unpin_data);
> -	job->waitchk = num_waitchks ? mem : NULL;
> -	mem += num_waitchks * sizeof(struct host1x_waitchk);
>  	job->gathers = num_cmdbufs ? mem : NULL;
>  	mem += num_cmdbufs * sizeof(struct host1x_job_gather);
>  	job->addr_phys = num_unpins ? mem : NULL;
> @@ -111,73 +107,6 @@ void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *bo,
>  }
>  EXPORT_SYMBOL(host1x_job_add_gather);
>  
> -/*
> - * NULL an already satisfied WAIT_SYNCPT host method, by patching its
> - * args in the command stream. The method data is changed to reference
> - * a reserved (never given out or incr) HOST1X_SYNCPT_RESERVED syncpt
> - * with a matching threshold value of 0, so is guaranteed to be popped
> - * by the host HW.
> - */
> -static void host1x_syncpt_patch_offset(struct host1x_syncpt *sp,
> -				       struct host1x_bo *h, u32 offset)
> -{
> -	void *patch_addr = NULL;
> -
> -	/* patch the wait */
> -	patch_addr = host1x_bo_kmap(h, offset >> PAGE_SHIFT);
> -	if (patch_addr) {
> -		host1x_syncpt_patch_wait(sp,
> -					 patch_addr + (offset & ~PAGE_MASK));
> -		host1x_bo_kunmap(h, offset >> PAGE_SHIFT, patch_addr);
> -	} else
> -		pr_err("Could not map cmdbuf for wait check\n");
> -}
> -
> -/*
> - * Check driver supplied waitchk structs for syncpt thresholds
> - * that have already been satisfied and NULL the comparison (to
> - * avoid a wrap condition in the HW).
> - */
> -static int do_waitchks(struct host1x_job *job, struct host1x *host,
> -		       struct host1x_job_gather *g)
> -{
> -	struct host1x_bo *patch = g->bo;
> -	int i;
> -
> -	/* compare syncpt vs wait threshold */
> -	for (i = 0; i < job->num_waitchk; i++) {
> -		struct host1x_waitchk *wait = &job->waitchk[i];
> -		struct host1x_syncpt *sp =
> -			host1x_syncpt_get(host, wait->syncpt_id);
> -
> -		/* validate syncpt id */
> -		if (wait->syncpt_id > host1x_syncpt_nb_pts(host))
> -			continue;
> -
> -		/* skip all other gathers */
> -		if (patch != wait->bo)
> -			continue;
> -
> -		trace_host1x_syncpt_wait_check(wait->bo, wait->offset,
> -					       wait->syncpt_id, wait->thresh,
> -					       host1x_syncpt_read_min(sp));
> -
> -		if (host1x_syncpt_is_expired(sp, wait->thresh)) {
> -			dev_dbg(host->dev,
> -				"drop WAIT id %u (%s) thresh 0x%x, min 0x%x\n",
> -				wait->syncpt_id, sp->name, wait->thresh,
> -				host1x_syncpt_read_min(sp));
> -
> -			host1x_syncpt_patch_offset(sp, patch,
> -						   g->offset + wait->offset);
> -		}
> -
> -		wait->bo = NULL;
> -	}
> -
> -	return 0;
> -}
> -
>  static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>  {
>  	unsigned int i;
> @@ -331,17 +260,6 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
>  	return true;
>  }
>  
> -static bool check_wait(struct host1x_waitchk *wait, struct host1x_bo *cmdbuf,
> -		       unsigned int offset)
> -{
> -	offset *= sizeof(u32);
> -
> -	if (wait->bo != cmdbuf || wait->offset != offset)
> -		return false;
> -
> -	return true;
> -}
> -
>  struct host1x_firewall {
>  	struct host1x_job *job;
>  	struct device *dev;
> @@ -349,9 +267,6 @@ struct host1x_firewall {
>  	unsigned int num_relocs;
>  	struct host1x_reloc *reloc;
>  
> -	unsigned int num_waitchks;
> -	struct host1x_waitchk *waitchk;
> -
>  	struct host1x_bo *cmdbuf;
>  	unsigned int offset;
>  
> @@ -378,20 +293,6 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
>  		fw->reloc++;
>  	}
>  
> -	if (offset == HOST1X_WAIT_SYNCPT_OFFSET) {
> -		if (fw->class != HOST1X_CLASS_HOST1X)
> -			return -EINVAL;
> -
> -		if (!fw->num_waitchks)
> -			return -EINVAL;
> -
> -		if (!check_wait(fw->waitchk, fw->cmdbuf, fw->offset))
> -			return -EINVAL;
> -
> -		fw->num_waitchks--;
> -		fw->waitchk++;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -556,8 +457,6 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
>  	fw.dev = dev;
>  	fw.reloc = job->relocarray;
>  	fw.num_relocs = job->num_relocs;
> -	fw.waitchk = job->waitchk;
> -	fw.num_waitchks = job->num_waitchk;
>  	fw.class = job->class;
>  
>  	for (i = 0; i < job->num_gathers; i++) {
> @@ -604,8 +503,8 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
>  		offset += g->words * sizeof(u32);
>  	}
>  
> -	/* No relocs and waitchks should remain at this point */
> -	if (fw.num_relocs || fw.num_waitchks)
> +	/* No relocs should remain at this point */
> +	if (fw.num_relocs)
>  		return -EINVAL;
>  
>  	return 0;
> @@ -616,19 +515,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
>  	int err;
>  	unsigned int i, j;
>  	struct host1x *host = dev_get_drvdata(dev->parent);
> -	DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
> -
> -	bitmap_zero(waitchk_mask, host1x_syncpt_nb_pts(host));
> -	for (i = 0; i < job->num_waitchk; i++) {
> -		u32 syncpt_id = job->waitchk[i].syncpt_id;
> -
> -		if (syncpt_id < host1x_syncpt_nb_pts(host))
> -			set_bit(syncpt_id, waitchk_mask);
> -	}
> -
> -	/* get current syncpt values for waitchk */
> -	for_each_set_bit(i, waitchk_mask, host1x_syncpt_nb_pts(host))
> -		host1x_syncpt_load(host->syncpt + i);
>  
>  	/* pin memory */
>  	err = pin_job(host, job);
> @@ -663,10 +549,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
>  		err = do_relocs(job, g);
>  		if (err)
>  			break;
> -
> -		err = do_waitchks(job, host, g);
> -		if (err)
> -			break;
>  	}
>  
>  out:
> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
> index a2a952adc136..a108669188e8 100644
> --- a/drivers/gpu/host1x/syncpt.c
> +++ b/drivers/gpu/host1x/syncpt.c
> @@ -373,12 +373,6 @@ bool host1x_syncpt_is_expired(struct host1x_syncpt *sp, u32 thresh)
>  		return (s32)(current_val - thresh) >= 0;
>  }
>  
> -/* remove a wait pointed to by patch_addr */
> -int host1x_syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)
> -{
> -	return host1x_hw_syncpt_patch_wait(sp->host, sp, patch_addr);
> -}
> -
>  int host1x_syncpt_init(struct host1x *host)
>  {
>  	struct host1x_syncpt_base *bases;
> diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h
> index 9d88d37c2397..d98e22325e9d 100644
> --- a/drivers/gpu/host1x/syncpt.h
> +++ b/drivers/gpu/host1x/syncpt.h
> @@ -124,7 +124,4 @@ static inline int host1x_syncpt_is_valid(struct host1x_syncpt *sp)
>  	return sp->id < host1x_syncpt_nb_pts(sp->host);
>  }
>  
> -/* Patch a wait by replacing it with a wait for syncpt 0 value 0 */
> -int host1x_syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr);
> -
>  #endif
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index ddf7f9ca86cc..f66bece1e1b7 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -192,13 +192,6 @@ struct host1x_reloc {
>  	unsigned long shift;
>  };
>  
> -struct host1x_waitchk {
> -	struct host1x_bo *bo;
> -	u32 offset;
> -	u32 syncpt_id;
> -	u32 thresh;
> -};
> -
>  struct host1x_job {
>  	/* When refcount goes to zero, job can be freed */
>  	struct kref ref;
> @@ -215,11 +208,6 @@ struct host1x_job {
>  	struct host1x_job_gather *gathers;
>  	unsigned int num_gathers;
>  
> -	/* Wait checks to be processed at submit time */
> -	struct host1x_waitchk *waitchk;
> -	unsigned int num_waitchk;
> -	u32 waitchk_mask;
> -
>  	/* Array of handles to be pinned & unpinned */
>  	struct host1x_reloc *relocarray;
>  	unsigned int num_relocs;
> @@ -261,8 +249,7 @@ struct host1x_job {
>  };
>  
>  struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
> -				    u32 num_cmdbufs, u32 num_relocs,
> -				    u32 num_waitchks);
> +				    u32 num_cmdbufs, u32 num_relocs);
>  void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *mem_id,
>  			   u32 words, u32 offset);
>  struct host1x_job *host1x_job_get(struct host1x_job *job);
> diff --git a/include/trace/events/host1x.h b/include/trace/events/host1x.h
> index 63116362543c..a37ef73092e5 100644
> --- a/include/trace/events/host1x.h
> +++ b/include/trace/events/host1x.h
> @@ -115,16 +115,15 @@ TRACE_EVENT(host1x_cdma_push_gather,
>  );
>  
>  TRACE_EVENT(host1x_channel_submit,
> -	TP_PROTO(const char *name, u32 cmdbufs, u32 relocs, u32 waitchks,
> -			u32 syncpt_id, u32 syncpt_incrs),
> +	TP_PROTO(const char *name, u32 cmdbufs, u32 relocs, u32 syncpt_id,
> +		 u32 syncpt_incrs),
>  
> -	TP_ARGS(name, cmdbufs, relocs, waitchks, syncpt_id, syncpt_incrs),
> +	TP_ARGS(name, cmdbufs, relocs, syncpt_id, syncpt_incrs),
>  
>  	TP_STRUCT__entry(
>  		__field(const char *, name)
>  		__field(u32, cmdbufs)
>  		__field(u32, relocs)
> -		__field(u32, waitchks)
>  		__field(u32, syncpt_id)
>  		__field(u32, syncpt_incrs)
>  	),
> @@ -133,15 +132,14 @@ TRACE_EVENT(host1x_channel_submit,
>  		__entry->name = name;
>  		__entry->cmdbufs = cmdbufs;
>  		__entry->relocs = relocs;
> -		__entry->waitchks = waitchks;
>  		__entry->syncpt_id = syncpt_id;
>  		__entry->syncpt_incrs = syncpt_incrs;
>  	),
>  
> -	TP_printk("name=%s, cmdbufs=%u, relocs=%u, waitchks=%d,"
> -		"syncpt_id=%u, syncpt_incrs=%u",
> -	  __entry->name, __entry->cmdbufs, __entry->relocs, __entry->waitchks,
> -	  __entry->syncpt_id, __entry->syncpt_incrs)
> +	TP_printk("name=%s, cmdbufs=%u, relocs=%u, syncpt_id=%u, "
> +		  "syncpt_incrs=%u",
> +		  __entry->name, __entry->cmdbufs, __entry->relocs,
> +		  __entry->syncpt_id, __entry->syncpt_incrs)
>  );
>  
>  TRACE_EVENT(host1x_channel_submitted,
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] gpu: host1x: Cleanup loop variable usage

[Dmitry Osipenko]

On 17.05.2018 18:34, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Use unsigned int where possible and don't unnecessarily initialize the
> loop variable.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/host1x/debug.c  | 2 +-
>  drivers/gpu/host1x/intr.c   | 2 +-
>  drivers/gpu/host1x/job.c    | 4 ++--
>  drivers/gpu/host1x/syncpt.c | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
> index dc77ec452ffc..329e4a3d8ae7 100644
> --- a/drivers/gpu/host1x/debug.c
> +++ b/drivers/gpu/host1x/debug.c
> @@ -103,7 +103,7 @@ static void show_syncpts(struct host1x *m, struct output *o)
>  
>  static void show_all(struct host1x *m, struct output *o, bool show_fifo)
>  {
> -	int i;
> +	unsigned int i;
>  
>  	host1x_hw_show_mlocks(m, o);
>  	show_syncpts(m, o);
> diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
> index 8b4fad0ab35d..6028cf7b681f 100644
> --- a/drivers/gpu/host1x/intr.c
> +++ b/drivers/gpu/host1x/intr.c
> @@ -144,7 +144,7 @@ static const action_handler action_handlers[HOST1X_INTR_ACTION_COUNT] = {
>  static void run_handlers(struct list_head completed[HOST1X_INTR_ACTION_COUNT])
>  {
>  	struct list_head *head = completed;
> -	int i;
> +	unsigned int i;
>  
>  	for (i = 0; i < HOST1X_INTR_ACTION_COUNT; ++i, ++head) {
>  		action_handler handler = action_handlers[i];
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 3cbfc6e37668..2be0bcaf8288 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -196,10 +196,10 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>  
>  static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
>  {
> -	int i = 0;
>  	u32 last_page = ~0;
>  	void *cmdbuf_page_addr = NULL;
>  	struct host1x_bo *cmdbuf = g->bo;
> +	unsigned int i;
>  
>  	/* pin & patch the relocs for one gather */
>  	for (i = 0; i < job->num_relocs; i++) {
> @@ -451,7 +451,7 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
>  	struct host1x_firewall fw;
>  	size_t size = 0;
>  	size_t offset = 0;
> -	int i;
> +	unsigned int i;
>  
>  	fw.job = job;
>  	fw.dev = dev;
> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
> index a108669188e8..088c05dd884c 100644
> --- a/drivers/gpu/host1x/syncpt.c
> +++ b/drivers/gpu/host1x/syncpt.c
> @@ -57,8 +57,8 @@ static struct host1x_syncpt *host1x_syncpt_alloc(struct host1x *host,
>  						 struct host1x_client *client,
>  						 unsigned long flags)
>  {
> -	int i;
>  	struct host1x_syncpt *sp = host->syncpt;
> +	unsigned int i;
>  	char *name;
>  
>  	mutex_lock(&host->syncpt_mutex);
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] gpu: host1x: Drop unnecessary host1x argument

[Dmitry Osipenko]

On 17.05.2018 18:34, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Functions taking a pointer to a host1x syncpoint as an argument don't
> need to specify a pointer to a host1x instance because it can be
> obtained from the syncpoint.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/host1x/hw/channel_hw.c |  2 +-
>  drivers/gpu/host1x/intr.c          | 14 ++++++--------
>  drivers/gpu/host1x/intr.h          |  8 +++++---
>  drivers/gpu/host1x/syncpt.c        |  2 +-
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
> index 4c9555038a95..d188f9068b91 100644
> --- a/drivers/gpu/host1x/hw/channel_hw.c
> +++ b/drivers/gpu/host1x/hw/channel_hw.c
> @@ -164,7 +164,7 @@ static int channel_submit(struct host1x_job *job)
>  	trace_host1x_channel_submitted(dev_name(ch->dev), prev_max, syncval);
>  
>  	/* schedule a submit complete interrupt */
> -	err = host1x_intr_add_action(host, job->syncpt_id, syncval,
> +	err = host1x_intr_add_action(host, sp, syncval,
>  				     HOST1X_INTR_ACTION_SUBMIT_COMPLETE, ch,
>  				     completed_waiter, NULL);
>  	completed_waiter = NULL;
> diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
> index 6028cf7b681f..9629c009d10f 100644
> --- a/drivers/gpu/host1x/intr.c
> +++ b/drivers/gpu/host1x/intr.c
> @@ -211,11 +211,11 @@ static void syncpt_thresh_work(struct work_struct *work)
>  				host1x_syncpt_load(host->syncpt + id));
>  }
>  
> -int host1x_intr_add_action(struct host1x *host, unsigned int id, u32 thresh,
> -			   enum host1x_intr_action action, void *data,
> -			   struct host1x_waitlist *waiter, void **ref)
> +int host1x_intr_add_action(struct host1x *host, struct host1x_syncpt *syncpt,
> +			   u32 thresh, enum host1x_intr_action action,
> +			   void *data, struct host1x_waitlist *waiter,
> +			   void **ref)
>  {
> -	struct host1x_syncpt *syncpt;
>  	int queue_was_empty;
>  
>  	if (waiter == NULL) {
> @@ -234,19 +234,17 @@ int host1x_intr_add_action(struct host1x *host, unsigned int id, u32 thresh,
>  	waiter->data = data;
>  	waiter->count = 1;
>  
> -	syncpt = host->syncpt + id;
> -
>  	spin_lock(&syncpt->intr.lock);
>  
>  	queue_was_empty = list_empty(&syncpt->intr.wait_head);
>  
>  	if (add_waiter_to_queue(waiter, &syncpt->intr.wait_head)) {
>  		/* added at head of list - new threshold value */
> -		host1x_hw_intr_set_syncpt_threshold(host, id, thresh);
> +		host1x_hw_intr_set_syncpt_threshold(host, syncpt->id, thresh);
>  
>  		/* added as first waiter - enable interrupt */
>  		if (queue_was_empty)
> -			host1x_hw_intr_enable_syncpt_intr(host, id);
> +			host1x_hw_intr_enable_syncpt_intr(host, syncpt->id);
>  	}
>  
>  	spin_unlock(&syncpt->intr.lock);
> diff --git a/drivers/gpu/host1x/intr.h b/drivers/gpu/host1x/intr.h
> index 1370c2bb75b8..6db96af484fe 100644
> --- a/drivers/gpu/host1x/intr.h
> +++ b/drivers/gpu/host1x/intr.h
> @@ -22,6 +22,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/workqueue.h>
>  
> +struct host1x_syncpt;
>  struct host1x;
>  
>  enum host1x_intr_action {
> @@ -75,9 +76,10 @@ struct host1x_waitlist {
>   *
>   * This is a non-blocking api.
>   */
> -int host1x_intr_add_action(struct host1x *host, unsigned int id, u32 thresh,
> -	enum host1x_intr_action action, void *data,
> -	struct host1x_waitlist *waiter, void **ref);
> +int host1x_intr_add_action(struct host1x *host, struct host1x_syncpt *syncpt,
> +			   u32 thresh, enum host1x_intr_action action,
> +			   void *data, struct host1x_waitlist *waiter,
> +			   void **ref);
>  
>  /*
>   * Unreference an action submitted to host1x_intr_add_action().
> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
> index 088c05dd884c..a5dbf1ba4645 100644
> --- a/drivers/gpu/host1x/syncpt.c
> +++ b/drivers/gpu/host1x/syncpt.c
> @@ -255,7 +255,7 @@ int host1x_syncpt_wait(struct host1x_syncpt *sp, u32 thresh, long timeout,
>  	}
>  
>  	/* schedule a wakeup when the syncpoint value is reached */
> -	err = host1x_intr_add_action(sp->host, sp->id, thresh,
> +	err = host1x_intr_add_action(sp->host, sp, thresh,
>  				     HOST1X_INTR_ACTION_WAKEUP_INTERRUPTIBLE,
>  				     &wq, waiter, &ref);
>  	if (err)
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] gpu: host1x: Rename relocarray -> relocs for consistency

[Dmitry Osipenko]

On 17.05.2018 18:34, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> All other array variables use a plural, and this is the only one using
> the *array suffix. This is confusing, so rename it for consistency.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 4 ++--
>  drivers/gpu/host1x/job.c    | 8 ++++----
>  include/linux/host1x.h      | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index e92e44e69871..f9d6c059792f 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -418,13 +418,13 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  		struct host1x_reloc *reloc;
>  		struct tegra_bo *obj;
>  
> -		err = host1x_reloc_copy_from_user(&job->relocarray[num_relocs],
> +		err = host1x_reloc_copy_from_user(&job->relocs[num_relocs],
>  						  &user_relocs[num_relocs], drm,
>  						  file);
>  		if (err < 0)
>  			goto fail;
>  
> -		reloc = &job->relocarray[num_relocs];
> +		reloc = &job->relocs[num_relocs];
>  		obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
>  		refs[num_refs++] = &obj->gem;
>  
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 2be0bcaf8288..9d6d3e151291 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -60,7 +60,7 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
>  
>  	/* Redistribute memory to the structs  */
>  	mem += sizeof(struct host1x_job);
> -	job->relocarray = num_relocs ? mem : NULL;
> +	job->relocs = num_relocs ? mem : NULL;
>  	mem += num_relocs * sizeof(struct host1x_reloc);
>  	job->unpins = num_unpins ? mem : NULL;
>  	mem += num_unpins * sizeof(struct host1x_job_unpin_data);
> @@ -115,7 +115,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>  	job->num_unpins = 0;
>  
>  	for (i = 0; i < job->num_relocs; i++) {
> -		struct host1x_reloc *reloc = &job->relocarray[i];
> +		struct host1x_reloc *reloc = &job->relocs[i];
>  		struct sg_table *sgt;
>  		dma_addr_t phys_addr;
>  
> @@ -203,7 +203,7 @@ static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
>  
>  	/* pin & patch the relocs for one gather */
>  	for (i = 0; i < job->num_relocs; i++) {
> -		struct host1x_reloc *reloc = &job->relocarray[i];
> +		struct host1x_reloc *reloc = &job->relocs[i];
>  		u32 reloc_addr = (job->reloc_addr_phys[i] +
>  				  reloc->target.offset) >> reloc->shift;
>  		u32 *target;
> @@ -455,7 +455,7 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
>  
>  	fw.job = job;
>  	fw.dev = dev;
> -	fw.reloc = job->relocarray;
> +	fw.reloc = job->relocs;
>  	fw.num_relocs = job->num_relocs;
>  	fw.class = job->class;
>  
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 0632010f47fb..dcb6140d39d7 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -210,7 +210,7 @@ struct host1x_job {
>  	unsigned int num_gathers;
>  
>  	/* Array of handles to be pinned & unpinned */
> -	struct host1x_reloc *relocarray;
> +	struct host1x_reloc *relocs;
>  	unsigned int num_relocs;
>  	struct host1x_job_unpin_data *unpins;
>  	unsigned int num_unpins;
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] gpu: host1x: Use not explicitly sized types

[Dmitry Osipenko]

On 17.05.2018 18:34, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The number of words and the offset in a gather don't need to be
> explicitly sized, so make them unsigned int instead.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/host1x/job.c | 13 ++++++++-----
>  drivers/gpu/host1x/job.h |  4 ++--
>  include/linux/host1x.h   |  4 ++--
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 9d6d3e151291..1a8344dbdb2b 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -96,13 +96,16 @@ void host1x_job_put(struct host1x_job *job)
>  EXPORT_SYMBOL(host1x_job_put);
>  
>  void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *bo,
> -			   u32 words, u32 offset)
> +			   unsigned int words, unsigned int offset)
>  {
> -	struct host1x_job_gather *cur_gather = &job->gathers[job->num_gathers];
> +	struct host1x_job_gather *gather = &job->gathers[job->num_gathers];
> +
> +	WARN_ON(job->num_gathers >= job->num_cmdbufs);
> +
> +	gather->words = words;
> +	gather->bo = bo;
> +	gather->offset = offset;
>  
> -	cur_gather->words = words;
> -	cur_gather->bo = bo;
> -	cur_gather->offset = offset;
>  	job->num_gathers++;
>  }
>  EXPORT_SYMBOL(host1x_job_add_gather);
> diff --git a/drivers/gpu/host1x/job.h b/drivers/gpu/host1x/job.h
> index 4bda51d503ec..188400e00192 100644
> --- a/drivers/gpu/host1x/job.h
> +++ b/drivers/gpu/host1x/job.h
> @@ -20,10 +20,10 @@
>  #define __HOST1X_JOB_H
>  
>  struct host1x_job_gather {
> -	u32 words;
> +	unsigned int words;
>  	dma_addr_t base;
>  	struct host1x_bo *bo;
> -	u32 offset;
> +	unsigned int offset;
>  	bool handled;
>  };
>  
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index dcb6140d39d7..89110d896d72 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -251,8 +251,8 @@ struct host1x_job {
>  
>  struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
>  				    u32 num_cmdbufs, u32 num_relocs);
> -void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *mem_id,
> -			   u32 words, u32 offset);
> +void host1x_job_add_gather(struct host1x_job *job, struct host1x_bo *bo,
> +			   unsigned int words, unsigned int offset);
>  struct host1x_job *host1x_job_get(struct host1x_job *job);
>  void host1x_job_put(struct host1x_job *job);
>  int host1x_job_pin(struct host1x_job *job, struct device *dev);
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] gpu: host1x: Track client version

[Dmitry Osipenko]

On 18.05.2018 15:39, Thierry Reding wrote:
> On Fri, May 18, 2018 at 03:21:11PM +0300, Mikko Perttunen wrote:
>> On 05/17/2018 06:34 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Userspace needs to know the version of the interface implemented by a
>>> client so it can create the proper command streams. Allow individual
>>> drivers to store this version along with the client so that it can be
>>> returned to userspace upon opening a channel.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>   include/linux/host1x.h | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
>>> index 89110d896d72..57d26406bdfd 100644
>>> --- a/include/linux/host1x.h
>>> +++ b/include/linux/host1x.h
>>> @@ -49,6 +49,7 @@ struct host1x_client_ops {
>>>    * @dev: pointer to struct device backing this host1x client
>>>    * @ops: host1x client operations
>>>    * @class: host1x class represented by this client
>>> + * @version: interface version implemented by this client
>>>    * @channel: host1x channel associated with this client
>>>    * @syncpts: array of syncpoints requested for this client
>>>    * @num_syncpts: number of syncpoints requested for this client
>>> @@ -61,6 +62,8 @@ struct host1x_client {
>>>   	const struct host1x_client_ops *ops;
>>>   	enum host1x_class class;
>>> +	unsigned int version;
>>> +
>>
>> It doesn't seem to me that this fits here - Host1x doesn't provide any
>> userspace interface, TegraDRM does. We will (hopefully) have clients in the
>> future that use a different userspace interface, or don't have one at all.
>> So this property should be on TegraDRM side instead.
> 
> I think the line is somewhat blurry. Technically it is the host1x
> clients that define the version of the interface that they provide. By
> interface I mean the "class" of the command stream supported. So gr2d
> will define what methods are valid to use on a specific version of the
> module. The same goes for gr3d and VIC. The hardware module is where
> that specification lives.
> 
> So if we ever had a client that didn't provide a userspace interface via
> Tegra DRM we'd still want to represent what version of the (command
> stream) interface is expected. By moving this to drm/tegra, we move the
> hardware specific bits into the userspace related parts, so I'm not sure
> it is a really good fit.
> 
> However, I do understand where you're coming from. Ultimately where
> userspace gets involved is at the ABI level that drm/tegra exposes. And
> for drivers that don't expose a userspace ABI, they'd likely need to
> deal with the version number internally, so storing this in host1x
> clients wouldn't fit very well either.
> 
> I don't really care all that much how exactly we store the version
> information so that it can be reported back to userspace, and moving it
> to drm/tegra makes the code somewhat more elegant. I came up with the
> below, which is completely untested but should work.
> 
> Thierry
> 
> --- >8 ---
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 4330d5d48801..5dc02340007e 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1389,7 +1389,7 @@ static int tegra_open_channel(struct drm_device *drm, void *data,
>  				break;
>  
>  			args->syncpts = client->base.num_syncpts;
> -			args->version = client->base.version;
> +			args->version = client->version;
>  			args->context = context->id;
>  			break;
>  		}
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 13660f6225a2..f493e0bf2172 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -105,6 +105,7 @@ struct tegra_drm_client {
>  	struct host1x_client base;
>  	struct list_head list;
>  
> +	unsigned int version;
>  	const struct tegra_drm_client_ops *ops;
>  };
>  
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index 7cae8252f0fa..165a1cece039 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -196,11 +196,11 @@ static int gr2d_probe(struct platform_device *pdev)
>  	gr2d->client.base.ops = &gr2d_client_ops;
>  	gr2d->client.base.dev = dev;
>  	gr2d->client.base.class = HOST1X_CLASS_GR2D;
> -	gr2d->client.base.version = gr2d->soc->version;
>  	gr2d->client.base.syncpts = syncpts;
>  	gr2d->client.base.num_syncpts = 1;
>  
>  	INIT_LIST_HEAD(&gr2d->client.list);
> +	gr2d->client.version = gr2d->soc->version;
>  	gr2d->client.ops = &gr2d_ops;
>  
>  	err = host1x_client_register(&gr2d->client.base);
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 9bd89916d2ef..0d72e78bd6e8 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -325,11 +325,11 @@ static int gr3d_probe(struct platform_device *pdev)
>  	gr3d->client.base.ops = &gr3d_client_ops;
>  	gr3d->client.base.dev = &pdev->dev;
>  	gr3d->client.base.class = HOST1X_CLASS_GR3D;
> -	gr3d->client.base.version = gr3d->soc->version;
>  	gr3d->client.base.syncpts = syncpts;
>  	gr3d->client.base.num_syncpts = 1;
>  
>  	INIT_LIST_HEAD(&gr3d->client.list);
> +	gr3d->client.version = gr3d->soc->version;
>  	gr3d->client.ops = &gr3d_ops;
>  
>  	err = host1x_client_register(&gr3d->client.base);
> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> index b9df467ca71e..9fa77405db01 100644
> --- a/drivers/gpu/drm/tegra/vic.c
> +++ b/drivers/gpu/drm/tegra/vic.c
> @@ -342,12 +342,12 @@ static int vic_probe(struct platform_device *pdev)
>  	vic->client.base.ops = &vic_client_ops;
>  	vic->client.base.dev = dev;
>  	vic->client.base.class = HOST1X_CLASS_VIC;
> -	vic->client.base.version = vic->config->version;
>  	vic->client.base.syncpts = syncpts;
>  	vic->client.base.num_syncpts = 1;
>  	vic->dev = dev;
>  
>  	INIT_LIST_HEAD(&vic->client.list);
> +	vic->client.version = vic->config->version;
>  	vic->client.ops = &vic_ops;
>  
>  	err = host1x_client_register(&vic->client.base);
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 6dc819b1ab64..518864cfbd16 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -62,7 +62,6 @@ struct host1x_client {
>  	const struct host1x_client_ops *ops;
>  
>  	enum host1x_class class;
> -	unsigned int version;
>  
>  	struct host1x_channel *channel;
>  
> 

Acked-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] gpu: host1x: Store pointer to client in jobs

[Dmitry Osipenko]

On 17.05.2018 18:34, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Rather than storing some identifier derived from the application
> context that can't be used concretely anywhere, store a pointer to the
> client directly so that accesses can be made directly through that
> client object.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 5 +++--
>  drivers/gpu/host1x/cdma.c   | 2 +-
>  drivers/gpu/host1x/cdma.h   | 2 +-
>  include/linux/host1x.h      | 3 ++-
>  4 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 33e479182773..e92e44e69871 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -317,6 +317,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_client *client = &context->client->base;
>  	unsigned int num_cmdbufs = args->num_cmdbufs;
>  	unsigned int num_relocs = args->num_relocs;
>  	struct drm_tegra_cmdbuf __user *user_cmdbufs;
> @@ -348,8 +349,8 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  		return -ENOMEM;
>  
>  	job->num_relocs = args->num_relocs;
> -	job->client = (u32)args->context;
> -	job->class = context->client->base.class;
> +	job->client = client;
> +	job->class = client->class;
>  	job->serialize = true;
>  
>  	/*
> diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
> index 28541b280739..cb590ba886ec 100644
> --- a/drivers/gpu/host1x/cdma.c
> +++ b/drivers/gpu/host1x/cdma.c
> @@ -247,7 +247,7 @@ static void cdma_start_timer_locked(struct host1x_cdma *cdma,
>  static void stop_cdma_timer_locked(struct host1x_cdma *cdma)
>  {
>  	cancel_delayed_work(&cdma->timeout.wq);
> -	cdma->timeout.client = 0;
> +	cdma->timeout.client = NULL;
>  }
>  
>  /*
> diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h
> index 286d49386be9..1825091bc0ed 100644
> --- a/drivers/gpu/host1x/cdma.h
> +++ b/drivers/gpu/host1x/cdma.h
> @@ -58,7 +58,7 @@ struct buffer_timeout {
>  	u32 syncpt_val;			/* syncpt value when completed */
>  	ktime_t start_ktime;		/* starting time */
>  	/* context timeout information */
> -	int client;
> +	struct host1x_client *client;
>  };
>  
>  enum cdma_event {
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index f66bece1e1b7..0632010f47fb 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -202,7 +202,8 @@ struct host1x_job {
>  	/* Channel where job is submitted to */
>  	struct host1x_channel *channel;
>  
> -	u32 client;
> +	/* client where the job originated */
> +	struct host1x_client *client;
>  
>  	/* Gathers and their memory */
>  	struct host1x_job_gather *gathers;
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel