Re: [PATCH 04/11] drm/amdgpu: add some VM PD/PT iterators
On 09/10/2018 02:03 AM, Christian König wrote:
Both a leaf as well as dfs iterator to walk over all the PDs/PTs.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 221 +
1 file changed, 221 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 416eccd9ea29..4007202585d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -355,6 +355,227 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct
amdgpu_vm_pt *pt)
return list_first_entry(>va, struct amdgpu_vm_pt, base.bo_list);
}
+/**
+ * amdgpu_vm_pt_cursor - state for for_each_amdgpu_vm_pt
+ */
+struct amdgpu_vm_pt_cursor {
+ uint64_t pfn;
+ struct amdgpu_vm_pt *parent;
+ struct amdgpu_vm_pt *entry;
+ unsigned level;
+};
+
+/**
+ * amdgpu_vm_pt_start - start PD/PT walk
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm structure
+ * @start: start address of the walk
+ * @cursor: state to initialize
+ *
+ * Initialize a amdgpu_vm_pt_cursor to start a walk.
+ */
+static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
+ struct amdgpu_vm *vm, uint64_t start,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+ cursor->pfn = start;
+ cursor->parent = NULL;
+ cursor->entry = >root;
+ cursor->level = adev->vm_manager.root_level;
+}
+
+/**
+ * amdgpu_vm_pt_descendant - got to child node
seems typo for "go to"
Jerry
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the child node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+ unsigned num_entries, shift, idx;
+
+ if (!cursor->entry->entries)
+ return false;
+
+ BUG_ON(!cursor->entry->base.bo);
+ num_entries = amdgpu_vm_num_entries(adev, cursor->level);
+ shift = amdgpu_vm_level_shift(adev, cursor->level);
+
+ ++cursor->level;
+ idx = (cursor->pfn >> shift) % num_entries;
+ cursor->parent = cursor->entry;
+ cursor->entry = >entry->entries[idx];
+ return true;
+}
+
+/**
+ * amdgpu_vm_pt_sibling - go to sibling node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the sibling node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_sibling(struct amdgpu_device *adev,
+struct amdgpu_vm_pt_cursor *cursor)
+{
+ unsigned shift, num_entries;
+
+ /* Root doesn't have a sibling */
+ if (!cursor->parent)
+ return false;
+
+ /* Go to our parents and see if we got a sibling */
+ shift = amdgpu_vm_level_shift(adev, cursor->level - 1);
+ num_entries = amdgpu_vm_num_entries(adev, cursor->level - 1);
+
+ if (cursor->entry == >parent->entries[num_entries - 1])
+ return false;
+
+ cursor->pfn += 1ULL << shift;
+ cursor->pfn &= ~((1ULL << shift) - 1);
+ ++cursor->entry;
+ return true;
+}
+
+/**
+ * amdgpu_vm_pt_ancestor - go to parent node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the parent node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor)
+{
+ if (!cursor->parent)
+ return false;
+
+ --cursor->level;
+ cursor->entry = cursor->parent;
+ cursor->parent = amdgpu_vm_pt_parent(cursor->parent);
+ return true;
+}
+
+/**
+ * amdgpu_vm_pt_next - get next PD/PT in hieratchy
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk the PD/PT tree to the next node.
+ */
+static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+ /* First try a newborn child */
+ if (amdgpu_vm_pt_descendant(adev, cursor))
+ return;
+
+ /* If that didn't worked try to find a sibling */
+ while (!amdgpu_vm_pt_sibling(adev, cursor)) {
+ /* No sibling, go to our parents and grandparents */
+ if (!amdgpu_vm_pt_ancestor(cursor)) {
+ cursor->pfn = ~0ll;
+ return;
+ }
+ }
+}
+
+/**
+ * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm structure
+ * @start: start addr of the walk
+ * @cursor: state to initialize
+ *
+ * Start a walk and go directly to the leaf node.
+ */
+static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
+ struct amdgpu_vm *vm, uint64_t start,
+
Re: [PATCH 04/11] drm/amdgpu: add some VM PD/PT iterators
On 2018-09-09 02:03 PM, Christian König wrote:
> Both a leaf as well as dfs iterator to walk over all the PDs/PTs.
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 221
> +
> 1 file changed, 221 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 416eccd9ea29..4007202585d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -355,6 +355,227 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct
> amdgpu_vm_pt *pt)
> return list_first_entry(>va, struct amdgpu_vm_pt, base.bo_list);
> }
>
> +/**
> + * amdgpu_vm_pt_cursor - state for for_each_amdgpu_vm_pt
> + */
> +struct amdgpu_vm_pt_cursor {
A lot of functions below have an adev parameter. I wonder if it makes
sense to add that to the cursor structure. Maybe it doesn't matter. Many
of those small functions will be inlined anyway.
> + uint64_t pfn;
> + struct amdgpu_vm_pt *parent;
> + struct amdgpu_vm_pt *entry;
> + unsigned level;
> +};
> +
> +/**
> + * amdgpu_vm_pt_start - start PD/PT walk
> + *
> + * @adev: amdgpu_device pointer
> + * @vm: amdgpu_vm structure
> + * @start: start address of the walk
> + * @cursor: state to initialize
> + *
> + * Initialize a amdgpu_vm_pt_cursor to start a walk.
> + */
> +static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
> +struct amdgpu_vm *vm, uint64_t start,
> +struct amdgpu_vm_pt_cursor *cursor)
> +{
> + cursor->pfn = start;
> + cursor->parent = NULL;
> + cursor->entry = >root;
> + cursor->level = adev->vm_manager.root_level;
> +}
> +
> +/**
> + * amdgpu_vm_pt_descendant - got to child node
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk to the child node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
> + struct amdgpu_vm_pt_cursor *cursor)
> +{
> + unsigned num_entries, shift, idx;
> +
> + if (!cursor->entry->entries)
> + return false;
> +
> + BUG_ON(!cursor->entry->base.bo);
Is that just a sanity check? Doesn't seem like anything in this function
depends on this. In KFD we made a choice to replace all BUG_ONs with
other means of reporting problems. Mostly WARN_ON, which is just as
noisy, but easier to debug. In a function that already has a failure
mode, it just adds one more condition for the function to fail, so the
code calling it shouldn't need any modifications.
checkpatch.pl will issue a warning for all BUG_ONs to consider alternatives.
> + num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> + shift = amdgpu_vm_level_shift(adev, cursor->level);
> +
> + ++cursor->level;
> + idx = (cursor->pfn >> shift) % num_entries;
& (num_entries - 1)?
> + cursor->parent = cursor->entry;
> + cursor->entry = >entry->entries[idx];
> + return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_sibling - go to sibling node
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk to the sibling node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_sibling(struct amdgpu_device *adev,
> + struct amdgpu_vm_pt_cursor *cursor)
> +{
> + unsigned shift, num_entries;
> +
> + /* Root doesn't have a sibling */
> + if (!cursor->parent)
> + return false;
> +
> + /* Go to our parents and see if we got a sibling */
> + shift = amdgpu_vm_level_shift(adev, cursor->level - 1);
> + num_entries = amdgpu_vm_num_entries(adev, cursor->level - 1);
> +
> + if (cursor->entry == >parent->entries[num_entries - 1])
> + return false;
> +
> + cursor->pfn += 1ULL << shift;
> + cursor->pfn &= ~((1ULL << shift) - 1);
> + ++cursor->entry;
> + return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_ancestor - go to parent node
> + *
> + * @adev: amdgpu_device pointer
Doesn't really have an adev parameter.
> + * @cursor: current state
> + *
> + * Walk to the parent node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor)
> +{
> + if (!cursor->parent)
> + return false;
> +
> + --cursor->level;
> + cursor->entry = cursor->parent;
> + cursor->parent = amdgpu_vm_pt_parent(cursor->parent);
> + return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_next - get next PD/PT in hieratchy
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk the PD/PT tree to the next node.
> + */
> +static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
> + struct
[PATCH 04/11] drm/amdgpu: add some VM PD/PT iterators
Both a leaf as well as dfs iterator to walk over all the PDs/PTs.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 221 +
1 file changed, 221 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 416eccd9ea29..4007202585d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -355,6 +355,227 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct
amdgpu_vm_pt *pt)
return list_first_entry(>va, struct amdgpu_vm_pt, base.bo_list);
}
+/**
+ * amdgpu_vm_pt_cursor - state for for_each_amdgpu_vm_pt
+ */
+struct amdgpu_vm_pt_cursor {
+ uint64_t pfn;
+ struct amdgpu_vm_pt *parent;
+ struct amdgpu_vm_pt *entry;
+ unsigned level;
+};
+
+/**
+ * amdgpu_vm_pt_start - start PD/PT walk
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm structure
+ * @start: start address of the walk
+ * @cursor: state to initialize
+ *
+ * Initialize a amdgpu_vm_pt_cursor to start a walk.
+ */
+static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
+ struct amdgpu_vm *vm, uint64_t start,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+ cursor->pfn = start;
+ cursor->parent = NULL;
+ cursor->entry = >root;
+ cursor->level = adev->vm_manager.root_level;
+}
+
+/**
+ * amdgpu_vm_pt_descendant - got to child node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the child node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+ unsigned num_entries, shift, idx;
+
+ if (!cursor->entry->entries)
+ return false;
+
+ BUG_ON(!cursor->entry->base.bo);
+ num_entries = amdgpu_vm_num_entries(adev, cursor->level);
+ shift = amdgpu_vm_level_shift(adev, cursor->level);
+
+ ++cursor->level;
+ idx = (cursor->pfn >> shift) % num_entries;
+ cursor->parent = cursor->entry;
+ cursor->entry = >entry->entries[idx];
+ return true;
+}
+
+/**
+ * amdgpu_vm_pt_sibling - go to sibling node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the sibling node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_sibling(struct amdgpu_device *adev,
+struct amdgpu_vm_pt_cursor *cursor)
+{
+ unsigned shift, num_entries;
+
+ /* Root doesn't have a sibling */
+ if (!cursor->parent)
+ return false;
+
+ /* Go to our parents and see if we got a sibling */
+ shift = amdgpu_vm_level_shift(adev, cursor->level - 1);
+ num_entries = amdgpu_vm_num_entries(adev, cursor->level - 1);
+
+ if (cursor->entry == >parent->entries[num_entries - 1])
+ return false;
+
+ cursor->pfn += 1ULL << shift;
+ cursor->pfn &= ~((1ULL << shift) - 1);
+ ++cursor->entry;
+ return true;
+}
+
+/**
+ * amdgpu_vm_pt_ancestor - go to parent node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the parent node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor)
+{
+ if (!cursor->parent)
+ return false;
+
+ --cursor->level;
+ cursor->entry = cursor->parent;
+ cursor->parent = amdgpu_vm_pt_parent(cursor->parent);
+ return true;
+}
+
+/**
+ * amdgpu_vm_pt_next - get next PD/PT in hieratchy
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk the PD/PT tree to the next node.
+ */
+static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+ /* First try a newborn child */
+ if (amdgpu_vm_pt_descendant(adev, cursor))
+ return;
+
+ /* If that didn't worked try to find a sibling */
+ while (!amdgpu_vm_pt_sibling(adev, cursor)) {
+ /* No sibling, go to our parents and grandparents */
+ if (!amdgpu_vm_pt_ancestor(cursor)) {
+ cursor->pfn = ~0ll;
+ return;
+ }
+ }
+}
+
+/**
+ * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm structure
+ * @start: start addr of the walk
+ * @cursor: state to initialize
+ *
+ * Start a walk and go directly to the leaf node.
+ */
+static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
+ struct amdgpu_vm *vm, uint64_t start,
+ struct amdgpu_vm_pt_cursor *cursor)
+{
+
