[PATCH 0/1] drm: Add a gpu page-table walker

[PATCH 0/1] drm: Add a gpu page-table walker

[Thomas Hellström]

A slightly unusual cover letter for a single patch.

The page table walker is currently used by the xe driver only,
but the code is generic so we can be good citizens and add it to drm
as a helper, for possible use by other drivers,
If so we can merge the commit when we merge the xe driver.

The question raised here is
*) Should it be a generic drm helper or xe-specific with changed
   prefixes?
*) If a drm helper, should we use a config option?

For usage examples, see xe_pt.c
https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c

Thanks,
Thomas

Thomas Hellström (1):
  drm: Add a gpu page-table walker helper

 drivers/gpu/drm/Makefile      |   1 +
 drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
 include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_pt_walk.c
 create mode 100644 include/drm/drm_pt_walk.h

-- 
2.34.1

[PATCH 1/1] drm: Add a gpu page-table walker helper

[Thomas Hellström]

Add a gpu page table walker similar in functionality to the cpu page-table
walker in mm/pagewalk.c. This is made a drm helper in the hope that it
might prove useful to other drivers, but we could of course make it
single-driver only and rename the functions initially.

Also if remaining a DRM helper, we should consider making it a helper
kernel module of its own.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/Makefile      |   1 +
 drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
 include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_pt_walk.c
 create mode 100644 include/drm/drm_pt_walk.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index ab4460fcd63f..53aae8a4ae99 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -39,6 +39,7 @@ drm-y := \
 	drm_prime.o \
 	drm_print.o \
 	drm_property.o \
+	drm_pt_walk.o \
 	drm_syncobj.o \
 	drm_sysfs.o \
 	drm_trace_points.o \
diff --git a/drivers/gpu/drm/drm_pt_walk.c b/drivers/gpu/drm/drm_pt_walk.c
new file mode 100644
index 000000000000..1a0b147a3acc
--- /dev/null
+++ b/drivers/gpu/drm/drm_pt_walk.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+#include <drm/drm_pt_walk.h>
+
+/**
+ * DOC: GPU page-table tree walking.
+ * The utilities in this file are similar to the CPU page-table walk
+ * utilities in mm/pagewalk.c. The main difference is that we distinguish
+ * the various levels of a page-table tree with an unsigned integer rather
+ * than by name. 0 is the lowest level, and page-tables with level 0 can
+ * not be directories pointing to lower levels, whereas all other levels
+ * can. The user of the utilities determines the highest level.
+ *
+ * Nomenclature:
+ * Each struct drm_pt, regardless of level is referred to as a page table, and
+ * multiple page tables typically form a page table tree with page tables at
+ * intermediate levels being page directories pointing at page tables at lower
+ * levels. A shared page table for a given address range is a page-table which
+ * is neither fully within nor fully outside the address range and that can
+ * thus be shared by two or more address ranges.
+ */
+static u64 drm_pt_addr_end(u64 addr, u64 end, unsigned int level,
+			   const struct drm_pt_walk *walk)
+{
+	u64 size = 1ull << walk->shifts[level];
+	u64 tmp = round_up(addr + 1, size);
+
+	return min_t(u64, tmp, end);
+}
+
+static bool drm_pt_next(pgoff_t *offset, u64 *addr, u64 next, u64 end,
+			unsigned int level, const struct drm_pt_walk *walk)
+{
+	pgoff_t step = 1;
+
+	/* Shared pt walk skips to the last pagetable */
+	if (unlikely(walk->shared_pt_mode)) {
+		unsigned int shift = walk->shifts[level];
+		u64 skip_to = round_down(end, 1ull << shift);
+
+		if (skip_to > next) {
+			step += (skip_to - next) >> shift;
+			next = skip_to;
+		}
+	}
+
+	*addr = next;
+	*offset += step;
+
+	return next != end;
+}
+
+/**
+ * drm_pt_walk_range() - Walk a range of a gpu page table tree with callbacks
+ * for each page-table entry in all levels.
+ * @parent: The root page table for walk start.
+ * @level: The root page table level.
+ * @addr: Virtual address start.
+ * @end: Virtual address end + 1.
+ * @walk: Walk info.
+ *
+ * Similar to the CPU page-table walker, this is a helper to walk
+ * a gpu page table and call a provided callback function for each entry.
+ *
+ * Return: 0 on success, negative error code on error. The error is
+ * propagated from the callback and on error the walk is terminated.
+ */
+int drm_pt_walk_range(struct drm_pt *parent, unsigned int level,
+		      u64 addr, u64 end, struct drm_pt_walk *walk)
+{
+	pgoff_t offset = drm_pt_offset(addr, level, walk);
+	struct drm_pt **entries = parent->dir ? parent->dir->entries : NULL;
+	const struct drm_pt_walk_ops *ops = walk->ops;
+	enum page_walk_action action;
+	struct drm_pt *child;
+	int err = 0;
+	u64 next;
+
+	do {
+		next = drm_pt_addr_end(addr, end, level, walk);
+		if (walk->shared_pt_mode && drm_pt_covers(addr, next, level,
+							  walk))
+			continue;
+again:
+		action = ACTION_SUBTREE;
+		child = entries ? entries[offset] : NULL;
+		err = ops->pt_entry(parent, offset, level, addr, next,
+				    &child, &action, walk);
+		if (err)
+			break;
+
+		/* Probably not needed yet for gpu pagetable walk. */
+		if (unlikely(action == ACTION_AGAIN))
+			goto again;
+
+		if (likely(!level || !child || action == ACTION_CONTINUE))
+			continue;
+
+		err = drm_pt_walk_range(child, level - 1, addr, next, walk);
+
+		if (!err && ops->pt_post_descend)
+			err = ops->pt_post_descend(parent, offset, level, addr,
+						   next, &child, &action, walk);
+		if (err)
+			break;
+
+	} while (drm_pt_next(&offset, &addr, next, end, level, walk));
+
+	return err;
+}
+EXPORT_SYMBOL(drm_pt_walk_range);
+
+/**
+ * drm_pt_walk_shared() - Walk shared page tables of a page-table tree.
+ * @parent: Root page table directory.
+ * @level: Level of the root.
+ * @addr: Start address.
+ * @end: Last address + 1.
+ * @walk: Walk info.
+ *
+ * This function is similar to drm_pt_walk_range() but it skips page tables
+ * that are private to the range. Since the root (or @parent) page table is
+ * typically also a shared page table this function is different in that it
+ * calls the pt_entry callback and the post_descend callback also for the
+ * root. The root can be detected in the callbacks by checking whether
+ * parent == *child.
+ * Walking only the shared page tables is common for unbind-type operations
+ * where the page-table entries for an address range are cleared or detached
+ * from the main page-table tree.
+ *
+ * Return: 0 on success, negative error code on error: If a callback
+ * returns an error, the walk will be terminated and the error returned by
+ * this function.
+ */
+int drm_pt_walk_shared(struct drm_pt *parent, unsigned int level,
+		       u64 addr, u64 end, struct drm_pt_walk *walk)
+{
+	const struct drm_pt_walk_ops *ops = walk->ops;
+	enum page_walk_action action = ACTION_SUBTREE;
+	struct drm_pt *child = parent;
+	int err;
+
+	walk->shared_pt_mode = true;
+	err = walk->ops->pt_entry(parent, 0, level + 1, addr, end,
+				  &child, &action, walk);
+
+	if (err || action != ACTION_SUBTREE)
+		return err;
+
+	err = drm_pt_walk_range(parent, level, addr, end, walk);
+	if (!err && ops->pt_post_descend) {
+		err = ops->pt_post_descend(parent, 0, level + 1, addr, end,
+					   &child, &action, walk);
+	}
+	return err;
+}
+EXPORT_SYMBOL(drm_pt_walk_shared);
diff --git a/include/drm/drm_pt_walk.h b/include/drm/drm_pt_walk.h
new file mode 100644
index 000000000000..64e7a418217c
--- /dev/null
+++ b/include/drm/drm_pt_walk.h
@@ -0,0 +1,161 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+#ifndef __DRM_PT_WALK__
+#define __DRM_PT_WALK__
+
+#include <linux/pagewalk.h>
+#include <linux/types.h>
+
+struct drm_pt_dir;
+
+/**
+ * struct drm_pt - base class for driver pagetable subclassing.
+ * @dir: Pointer to an array of children if any.
+ *
+ * Drivers could subclass this, and if it's a page-directory, typically
+ * embed the drm_pt_dir::entries array in the same allocation.
+ */
+struct drm_pt {
+	struct drm_pt_dir *dir;
+};
+
+/**
+ * struct drm_pt_dir - page directory structure
+ * @entries: Array holding page directory children.
+ *
+ * It is the responsibility of the user to ensure @entries is
+ * correctly sized.
+ */
+struct drm_pt_dir {
+	struct drm_pt *entries[0];
+};
+
+/**
+ * struct drm_pt_walk - Embeddable struct for walk parameters
+ */
+struct drm_pt_walk {
+	/** @ops: The walk ops used for the pagewalk */
+	const struct drm_pt_walk_ops *ops;
+	/**
+	 * @shifts: Array of page-table entry shifts used for the
+	 * different levels, starting out with the leaf level 0
+	 * page-shift as the first entry. It's legal for this pointer to be
+	 * changed during the walk.
+	 */
+	const u64 *shifts;
+	/** @max_level: Highest populated level in @sizes */
+	unsigned int max_level;
+	/**
+	 * @shared_pt_mode: Whether to skip all entries that are private
+	 * to the address range and called only for entries that are
+	 * shared with other address ranges. Such entries are referred to
+	 * as shared pagetables.
+	 */
+	bool shared_pt_mode;
+};
+
+/**
+ * typedef drm_pt_entry_fn - gpu page-table-walk callback-function
+ * @parent: The parent page.table.
+ * @offset: The offset (number of entries) into the page table.
+ * @level: The level of @parent.
+ * @addr: The virtual address.
+ * @next: The virtual address for the next call, or end address.
+ * @child: Pointer to pointer to child page-table at this @offset. The
+ * function may modify the value pointed to if, for example, allocating a
+ * child page table.
+ * @action: The walk action to take upon return. See <linux/pagewalk.h>.
+ * @walk: The walk parameters.
+ */
+typedef int (*drm_pt_entry_fn)(struct drm_pt *parent, pgoff_t offset,
+			       unsigned int level, u64 addr, u64 next,
+			       struct drm_pt **child,
+			       enum page_walk_action *action,
+			       struct drm_pt_walk *walk);
+
+/**
+ * struct drm_pt_walk_ops - Walk callbacks.
+ */
+struct drm_pt_walk_ops {
+	/**
+	 * @pt_entry: Callback to be called for each page table entry prior
+	 * to descending to the next level. The returned value of the action
+	 * function parameter is honored.
+	 */
+	drm_pt_entry_fn pt_entry;
+	/**
+	 * @pt_post_descend: Callback to be called for each page table entry
+	 * after return from descending to the next level. The returned value
+	 * of the action function parameter is ignored.
+	 */
+	drm_pt_entry_fn pt_post_descend;
+};
+
+int drm_pt_walk_range(struct drm_pt *parent, unsigned int level,
+		      u64 addr, u64 end, struct drm_pt_walk *walk);
+
+int drm_pt_walk_shared(struct drm_pt *parent, unsigned int level,
+		       u64 addr, u64 end, struct drm_pt_walk *walk);
+
+/**
+ * drm_pt_covers - Whether the address range covers an entire entry in @level
+ * @addr: Start of the range.
+ * @end: End of range + 1.
+ * @level: Page table level.
+ * @walk: Page table walk info.
+ *
+ * This function is a helper to aid in determining whether a leaf page table
+ * entry can be inserted at this @level.
+ *
+ * Return: Whether the range provided covers exactly an entry at this level.
+ */
+static inline bool drm_pt_covers(u64 addr, u64 end, unsigned int level,
+				 const struct drm_pt_walk *walk)
+{
+	u64 pt_size = 1ull << walk->shifts[level];
+
+	return end - addr == pt_size && IS_ALIGNED(addr, pt_size);
+}
+
+/**
+ * drm_pt_num_entries: Number of page-table entries of a given range at this
+ * level
+ * @addr: Start address.
+ * @end: End address.
+ * @level: Page table level.
+ * @walk: Walk info.
+ *
+ * Return: The number of page table entries at this level between @start and
+ * @end.
+ */
+static inline pgoff_t
+drm_pt_num_entries(u64 addr, u64 end, unsigned int level,
+		   const struct drm_pt_walk *walk)
+{
+	u64 pt_size = 1ull << walk->shifts[level];
+
+	return (round_up(end, pt_size) - round_down(addr, pt_size)) >>
+		walk->shifts[level];
+}
+
+/**
+ * drm_pt_offset: Offset of the page-table entry for a given address.
+ * @addr: The address.
+ * @level: Page table level.
+ * @walk: Walk info.
+ *
+ * Return: The page table entry offset for the given address in a
+ * page table with size indicated by @level.
+ */
+static inline pgoff_t
+drm_pt_offset(u64 addr, unsigned int level, const struct drm_pt_walk *walk)
+{
+	if (level < walk->max_level)
+		addr &= ((1ull << walk->shifts[level + 1]) - 1);
+
+	return addr >> walk->shifts[level];
+}
+
+#endif
-- 
2.34.1

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Daniel Vetter]

On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
> A slightly unusual cover letter for a single patch.
> 
> The page table walker is currently used by the xe driver only,
> but the code is generic so we can be good citizens and add it to drm
> as a helper, for possible use by other drivers,
> If so we can merge the commit when we merge the xe driver.
> 
> The question raised here is
> *) Should it be a generic drm helper or xe-specific with changed
>    prefixes?

I think if there's some other drivers interested in using this, then this
sounds like a good idea. Maybe more useful if it's also integrated into
the vm/vma helpers that are being discussed as an optional part?

Maybe some good old sales pitching here to convince people would be good.

Maybe one of the new accel drivers is interested in this too?

> *) If a drm helper, should we use a config option?

I am no fan of Kconfig things tbh. Maybe just include it in the vma
helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
drm/sched, this one here, vm/vma helpers or whatever they will be and so
on? Kinda like we have modeset helpers.

I'd definitely not go for a Kconfig per individual file, that's just
excessive.
-Daniel

> 
> For usage examples, see xe_pt.c
> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
> 
> Thanks,
> Thomas
> 
> Thomas Hellström (1):
>   drm: Add a gpu page-table walker helper
> 
>  drivers/gpu/drm/Makefile      |   1 +
>  drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
>  include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
>  3 files changed, 321 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_pt_walk.c
>  create mode 100644 include/drm/drm_pt_walk.h
> 
> -- 
> 2.34.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Thomas Hellström]

Hi, Daniel,

On 2/16/23 21:18, Daniel Vetter wrote:
> On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
>> A slightly unusual cover letter for a single patch.
>>
>> The page table walker is currently used by the xe driver only,
>> but the code is generic so we can be good citizens and add it to drm
>> as a helper, for possible use by other drivers,
>> If so we can merge the commit when we merge the xe driver.
>>
>> The question raised here is
>> *) Should it be a generic drm helper or xe-specific with changed
>>     prefixes?
> I think if there's some other drivers interested in using this, then this
> sounds like a good idea. Maybe more useful if it's also integrated into
> the vm/vma helpers that are being discussed as an optional part?
>
> Maybe some good old sales pitching here to convince people would be good.
>
> Maybe one of the new accel drivers is interested in this too?

Thanks for your thoughts on this. Yeah, I think it's a bit awkward to 
push for having code generic when there is only one user, and the 
prospect of having other drivers rewrite their page-table building code 
based on this helper in the near future is probably small. Perhaps more 
of interest to new drivers. I think what will happen otherwise is that 
during some future cleanup this will be pushed down to xe claiming it's 
the only user.

I wonder whether it might be an idea to maintain a small document where 
driver writers can list suggestions for code that could be lifted to 
core drm and be reused by others. That way both reviewers and writers of 
other drivers can keep an eye on that document and use it to avoid 
duplicating code. The procedure would then be to lift it to core drm and 
fix up prefixes as soon as we have two or more users.

Thoughts?

Thomas


>
>> *) If a drm helper, should we use a config option?
> I am no fan of Kconfig things tbh. Maybe just include it in the vma
> helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
> drm/sched, this one here, vm/vma helpers or whatever they will be and so
> on? Kinda like we have modeset helpers.
>
> I'd definitely not go for a Kconfig per individual file, that's just
> excessive.
> -Daniel
>
>> For usage examples, see xe_pt.c
>> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
>>
>> Thanks,
>> Thomas
>>
>> Thomas Hellström (1):
>>    drm: Add a gpu page-table walker helper
>>
>>   drivers/gpu/drm/Makefile      |   1 +
>>   drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
>>   include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 321 insertions(+)
>>   create mode 100644 drivers/gpu/drm/drm_pt_walk.c
>>   create mode 100644 include/drm/drm_pt_walk.h
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Alex Deucher]

On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> Hi, Daniel,
>
> On 2/16/23 21:18, Daniel Vetter wrote:
> > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
> >> A slightly unusual cover letter for a single patch.
> >>
> >> The page table walker is currently used by the xe driver only,
> >> but the code is generic so we can be good citizens and add it to drm
> >> as a helper, for possible use by other drivers,
> >> If so we can merge the commit when we merge the xe driver.
> >>
> >> The question raised here is
> >> *) Should it be a generic drm helper or xe-specific with changed
> >>     prefixes?
> > I think if there's some other drivers interested in using this, then this
> > sounds like a good idea. Maybe more useful if it's also integrated into
> > the vm/vma helpers that are being discussed as an optional part?
> >
> > Maybe some good old sales pitching here to convince people would be good.
> >
> > Maybe one of the new accel drivers is interested in this too?
>
> Thanks for your thoughts on this. Yeah, I think it's a bit awkward to
> push for having code generic when there is only one user, and the
> prospect of having other drivers rewrite their page-table building code
> based on this helper in the near future is probably small. Perhaps more
> of interest to new drivers. I think what will happen otherwise is that
> during some future cleanup this will be pushed down to xe claiming it's
> the only user.
>
> I wonder whether it might be an idea to maintain a small document where
> driver writers can list suggestions for code that could be lifted to
> core drm and be reused by others. That way both reviewers and writers of
> other drivers can keep an eye on that document and use it to avoid
> duplicating code. The procedure would then be to lift it to core drm and
> fix up prefixes as soon as we have two or more users.
>
> Thoughts?

FWIW, when we originally wrote the GPU scheduler it was part of
amdgpu, but we consciously kept any AMD-isms out of it so it could be
lifted up to a core component when another user came along.  Maybe
some comments in the top of those files to that effect to maintain the
separation.

Alex


>
> Thomas
>
>
> >
> >> *) If a drm helper, should we use a config option?
> > I am no fan of Kconfig things tbh. Maybe just include it in the vma
> > helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
> > drm/sched, this one here, vm/vma helpers or whatever they will be and so
> > on? Kinda like we have modeset helpers.
> >
> > I'd definitely not go for a Kconfig per individual file, that's just
> > excessive.
> > -Daniel
> >
> >> For usage examples, see xe_pt.c
> >> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
> >>
> >> Thanks,
> >> Thomas
> >>
> >> Thomas Hellström (1):
> >>    drm: Add a gpu page-table walker helper
> >>
> >>   drivers/gpu/drm/Makefile      |   1 +
> >>   drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
> >>   include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
> >>   3 files changed, 321 insertions(+)
> >>   create mode 100644 drivers/gpu/drm/drm_pt_walk.c
> >>   create mode 100644 include/drm/drm_pt_walk.h
> >>
> >> --
> >> 2.34.1
> >>

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Oded Gabbay]

On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
> >
> > Hi, Daniel,
> >
> > On 2/16/23 21:18, Daniel Vetter wrote:
> > > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
> > >> A slightly unusual cover letter for a single patch.
> > >>
> > >> The page table walker is currently used by the xe driver only,
> > >> but the code is generic so we can be good citizens and add it to drm
> > >> as a helper, for possible use by other drivers,
> > >> If so we can merge the commit when we merge the xe driver.
> > >>
> > >> The question raised here is
> > >> *) Should it be a generic drm helper or xe-specific with changed
> > >>     prefixes?
> > > I think if there's some other drivers interested in using this, then this
> > > sounds like a good idea. Maybe more useful if it's also integrated into
> > > the vm/vma helpers that are being discussed as an optional part?
> > >
> > > Maybe some good old sales pitching here to convince people would be good.
> > >
> > > Maybe one of the new accel drivers is interested in this too?
Hi,
As the habanalabs driver is not really a new driver, I currently don't
see the benefit of moving
to this code. Our pgt code is quite mature and was tested extensively
in deployment in the
past couple of years.

Nevertheless, I'll try to offer this code for any new/future driver
that will want to join accel.

Stanislaw, I'm adding you here in case you missed this. Might be of an
interest to you.

Thanks,
Oded

- Oded



> >
> > Thanks for your thoughts on this. Yeah, I think it's a bit awkward to
> > push for having code generic when there is only one user, and the
> > prospect of having other drivers rewrite their page-table building code
> > based on this helper in the near future is probably small. Perhaps more
> > of interest to new drivers. I think what will happen otherwise is that
> > during some future cleanup this will be pushed down to xe claiming it's
> > the only user.
> >
> > I wonder whether it might be an idea to maintain a small document where
> > driver writers can list suggestions for code that could be lifted to
> > core drm and be reused by others. That way both reviewers and writers of
> > other drivers can keep an eye on that document and use it to avoid
> > duplicating code. The procedure would then be to lift it to core drm and
> > fix up prefixes as soon as we have two or more users.
> >
> > Thoughts?
>
> FWIW, when we originally wrote the GPU scheduler it was part of
> amdgpu, but we consciously kept any AMD-isms out of it so it could be
> lifted up to a core component when another user came along.  Maybe
> some comments in the top of those files to that effect to maintain the
> separation.
>
> Alex
>
>
> >
> > Thomas
> >
> >
> > >
> > >> *) If a drm helper, should we use a config option?
> > > I am no fan of Kconfig things tbh. Maybe just include it in the vma
> > > helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
> > > drm/sched, this one here, vm/vma helpers or whatever they will be and so
> > > on? Kinda like we have modeset helpers.
> > >
> > > I'd definitely not go for a Kconfig per individual file, that's just
> > > excessive.
> > > -Daniel
> > >
> > >> For usage examples, see xe_pt.c
> > >> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
> > >>
> > >> Thanks,
> > >> Thomas
> > >>
> > >> Thomas Hellström (1):
> > >>    drm: Add a gpu page-table walker helper
> > >>
> > >>   drivers/gpu/drm/Makefile      |   1 +
> > >>   drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
> > >>   include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
> > >>   3 files changed, 321 insertions(+)
> > >>   create mode 100644 drivers/gpu/drm/drm_pt_walk.c
> > >>   create mode 100644 include/drm/drm_pt_walk.h
> > >>
> > >> --
> > >> 2.34.1
> > >>

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Thomas Hellström]

Hi,

On 2/23/23 19:50, Alex Deucher wrote:
> On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> Hi, Daniel,
>>
>> On 2/16/23 21:18, Daniel Vetter wrote:
>>> On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
>>>> A slightly unusual cover letter for a single patch.
>>>>
>>>> The page table walker is currently used by the xe driver only,
>>>> but the code is generic so we can be good citizens and add it to drm
>>>> as a helper, for possible use by other drivers,
>>>> If so we can merge the commit when we merge the xe driver.
>>>>
>>>> The question raised here is
>>>> *) Should it be a generic drm helper or xe-specific with changed
>>>>      prefixes?
>>> I think if there's some other drivers interested in using this, then this
>>> sounds like a good idea. Maybe more useful if it's also integrated into
>>> the vm/vma helpers that are being discussed as an optional part?
>>>
>>> Maybe some good old sales pitching here to convince people would be good.
>>>
>>> Maybe one of the new accel drivers is interested in this too?
>> Thanks for your thoughts on this. Yeah, I think it's a bit awkward to
>> push for having code generic when there is only one user, and the
>> prospect of having other drivers rewrite their page-table building code
>> based on this helper in the near future is probably small. Perhaps more
>> of interest to new drivers. I think what will happen otherwise is that
>> during some future cleanup this will be pushed down to xe claiming it's
>> the only user.
>>
>> I wonder whether it might be an idea to maintain a small document where
>> driver writers can list suggestions for code that could be lifted to
>> core drm and be reused by others. That way both reviewers and writers of
>> other drivers can keep an eye on that document and use it to avoid
>> duplicating code. The procedure would then be to lift it to core drm and
>> fix up prefixes as soon as we have two or more users.
>>
>> Thoughts?
> FWIW, when we originally wrote the GPU scheduler it was part of
> amdgpu, but we consciously kept any AMD-isms out of it so it could be
> lifted up to a core component when another user came along.  Maybe
> some comments in the top of those files to that effect to maintain the
> separation.

Sure. I'll do that. It sounds like we'll keep this in xe for now.

Thanks,

/Thomas


> Alex
>
>
>> Thomas
>>
>>
>>>> *) If a drm helper, should we use a config option?
>>> I am no fan of Kconfig things tbh. Maybe just include it in the vma
>>> helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
>>> drm/sched, this one here, vm/vma helpers or whatever they will be and so
>>> on? Kinda like we have modeset helpers.
>>>
>>> I'd definitely not go for a Kconfig per individual file, that's just
>>> excessive.
>>> -Daniel
>>>
>>>> For usage examples, see xe_pt.c
>>>> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
>>>>
>>>> Thanks,
>>>> Thomas
>>>>
>>>> Thomas Hellström (1):
>>>>     drm: Add a gpu page-table walker helper
>>>>
>>>>    drivers/gpu/drm/Makefile      |   1 +
>>>>    drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 321 insertions(+)
>>>>    create mode 100644 drivers/gpu/drm/drm_pt_walk.c
>>>>    create mode 100644 include/drm/drm_pt_walk.h
>>>>
>>>> --
>>>> 2.34.1
>>>>

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Thomas Hellström]

Hi, Oded.

On 2/26/23 19:56, Oded Gabbay wrote:
> On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström
>> <thomas.hellstrom@linux.intel.com> wrote:
>>> Hi, Daniel,
>>>
>>> On 2/16/23 21:18, Daniel Vetter wrote:
>>>> On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
>>>>> A slightly unusual cover letter for a single patch.
>>>>>
>>>>> The page table walker is currently used by the xe driver only,
>>>>> but the code is generic so we can be good citizens and add it to drm
>>>>> as a helper, for possible use by other drivers,
>>>>> If so we can merge the commit when we merge the xe driver.
>>>>>
>>>>> The question raised here is
>>>>> *) Should it be a generic drm helper or xe-specific with changed
>>>>>      prefixes?
>>>> I think if there's some other drivers interested in using this, then this
>>>> sounds like a good idea. Maybe more useful if it's also integrated into
>>>> the vm/vma helpers that are being discussed as an optional part?
>>>>
>>>> Maybe some good old sales pitching here to convince people would be good.
>>>>
>>>> Maybe one of the new accel drivers is interested in this too?
> Hi,
> As the habanalabs driver is not really a new driver, I currently don't
> see the benefit of moving
> to this code. Our pgt code is quite mature and was tested extensively
> in deployment in the
> past couple of years.
>
> Nevertheless, I'll try to offer this code for any new/future driver
> that will want to join accel.
>
> Stanislaw, I'm adding you here in case you missed this. Might be of an
> interest to you.


Thanks for taking a look. Yes, as also mentioned to Alex, I think we'll 
keep this in xe for now.

/Thomas

>
> Thanks,
> Oded
>
> - Oded
>
>
>
>>> Thanks for your thoughts on this. Yeah, I think it's a bit awkward to
>>> push for having code generic when there is only one user, and the
>>> prospect of having other drivers rewrite their page-table building code
>>> based on this helper in the near future is probably small. Perhaps more
>>> of interest to new drivers. I think what will happen otherwise is that
>>> during some future cleanup this will be pushed down to xe claiming it's
>>> the only user.
>>>
>>> I wonder whether it might be an idea to maintain a small document where
>>> driver writers can list suggestions for code that could be lifted to
>>> core drm and be reused by others. That way both reviewers and writers of
>>> other drivers can keep an eye on that document and use it to avoid
>>> duplicating code. The procedure would then be to lift it to core drm and
>>> fix up prefixes as soon as we have two or more users.
>>>
>>> Thoughts?
>> FWIW, when we originally wrote the GPU scheduler it was part of
>> amdgpu, but we consciously kept any AMD-isms out of it so it could be
>> lifted up to a core component when another user came along.  Maybe
>> some comments in the top of those files to that effect to maintain the
>> separation.
>>
>> Alex
>>
>>
>>> Thomas
>>>
>>>
>>>>> *) If a drm helper, should we use a config option?
>>>> I am no fan of Kconfig things tbh. Maybe just include it in the vma
>>>> helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
>>>> drm/sched, this one here, vm/vma helpers or whatever they will be and so
>>>> on? Kinda like we have modeset helpers.
>>>>
>>>> I'd definitely not go for a Kconfig per individual file, that's just
>>>> excessive.
>>>> -Daniel
>>>>
>>>>> For usage examples, see xe_pt.c
>>>>> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
>>>>>
>>>>> Thanks,
>>>>> Thomas
>>>>>
>>>>> Thomas Hellström (1):
>>>>>     drm: Add a gpu page-table walker helper
>>>>>
>>>>>    drivers/gpu/drm/Makefile      |   1 +
>>>>>    drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++
>>>>>    include/drm/drm_pt_walk.h     | 161 ++++++++++++++++++++++++++++++++++
>>>>>    3 files changed, 321 insertions(+)
>>>>>    create mode 100644 drivers/gpu/drm/drm_pt_walk.c
>>>>>    create mode 100644 include/drm/drm_pt_walk.h
>>>>>
>>>>> --
>>>>> 2.34.1
>>>>>

Re: [PATCH 0/1] drm: Add a gpu page-table walker

[Stanislaw Gruszka]

On Mon, Feb 27, 2023 at 09:09:14AM +0100, Thomas Hellström wrote:
> Hi, Oded.
> 
> On 2/26/23 19:56, Oded Gabbay wrote:
> > On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com> wrote:
> > > > Hi, Daniel,
> > > > 
> > > > On 2/16/23 21:18, Daniel Vetter wrote:
> > > > > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
> > > > > > A slightly unusual cover letter for a single patch.
> > > > > > 
> > > > > > The page table walker is currently used by the xe driver only,
> > > > > > but the code is generic so we can be good citizens and add it to drm
> > > > > > as a helper, for possible use by other drivers,
> > > > > > If so we can merge the commit when we merge the xe driver.
> > > > > > 
> > > > > > The question raised here is
> > > > > > *) Should it be a generic drm helper or xe-specific with changed
> > > > > >      prefixes?
> > > > > I think if there's some other drivers interested in using this, then this
> > > > > sounds like a good idea. Maybe more useful if it's also integrated into
> > > > > the vm/vma helpers that are being discussed as an optional part?
> > > > > 
> > > > > Maybe some good old sales pitching here to convince people would be good.
> > > > > 
> > > > > Maybe one of the new accel drivers is interested in this too?
> > Hi,
> > As the habanalabs driver is not really a new driver, I currently don't
> > see the benefit of moving
> > to this code. Our pgt code is quite mature and was tested extensively
> > in deployment in the
> > past couple of years.
> > 
> > Nevertheless, I'll try to offer this code for any new/future driver
> > that will want to join accel.
> > 
> > Stanislaw, I'm adding you here in case you missed this. Might be of an
> > interest to you.

Rewrite table walk will not give the ivpu driver much, perhaps one function
would be smaller. Nothing that would justify the effort IMO.

> Thanks for taking a look. Yes, as also mentioned to Alex, I think we'll keep
> this in xe for now.

Sounds good :-)

Regards
Stanislaw