[PATCH v2 0/4] drm/exec: drm_exec polishing

[PATCH v2 0/4] drm/exec: drm_exec polishing

[Thomas Hellström]

During the work towards enabling exhaustive eviction using full
ww locking in TTM, Christian indicated that the path for the
drm_exec moving forward was to be a full drm_exec helper with
things like userptr validation rather than a WW transaction
abstraction. The idea was then briefly discussed to craft a
WW transaction helper and then subclass that with drm_exec
with the idea that the WW transaction helper could be used in
TTM for eviction and for other uses that didn't mandate a full
exec sequence.

Regardless whether that actually happens or not, this series
aims to clean up abuses of drm_exec internals in drivers
so that future development of drm_exec isn't blocked by
such driver usage.

Except for patch 2 which is a small cleanup only.

v2:
- An drm/msm patch was already merged.
- Use a unique id for the loop variable (patch 1) (Christian)
- Move assignment to declaration (patch 2) (Christian)
- Declare the retry pointer as void *const. (patch 2)
- Only allow if the drm_exec context is newly initialized. (patch 3)
  (Christian)
- Fix amdgpu compile error (patch 4) (Intel CI)
- Update the commit message (patch 4)

Thomas Hellström (4):
  drm/exec: Remove the index parameter from
    drm_exec_for_each_locked_obj[_reverse]
  drm/exec: Make the drm_exec_until_all_locked() macro more readable
  drm/exec, drm/xe: Avoid abusing the drm_exec retry pointer
  drm/exec, drm/xe, drm/amdgpu: Add an accessor for struct
    drm_exec::ticket

 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        | 15 ++--
 .../drm/amd/amdgpu/amdgpu_eviction_fence.c    |  3 +-
 drivers/gpu/drm/drm_exec.c                    |  9 +--
 drivers/gpu/drm/drm_gpuvm.c                   |  3 +-
 drivers/gpu/drm/xe/xe_validation.c            |  4 +-
 drivers/gpu/drm/xe/xe_validation.h            |  2 +-
 drivers/gpu/drm/xe/xe_vm.c                    |  3 +-
 include/drm/drm_exec.h                        | 71 ++++++++++++++-----
 9 files changed, 70 insertions(+), 44 deletions(-)

-- 
2.54.0

[PATCH v2 1/4] drm/exec: Remove the index parameter from drm_exec_for_each_locked_obj[_reverse]

[Thomas Hellström]

Nobody makes any use of it. Possible internal future users can
instead use the _index variable. External users shouldn't use
it since the array it's pointing into is internal drm_exec state.

v2:
- Use a unique id for the loop variable (Christian)

Assisted-by: GitHub Copilot:claude-sonnet-4.6
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  9 +++------
 .../drm/amd/amdgpu/amdgpu_eviction_fence.c    |  3 +--
 drivers/gpu/drm/drm_exec.c                    |  6 ++----
 drivers/gpu/drm/drm_gpuvm.c                   |  3 +--
 drivers/gpu/drm/xe/xe_vm.c                    |  3 +--
 include/drm/drm_exec.h                        | 20 +++++++++++--------
 6 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 32af8cce3df8..97a851ae7bd8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -850,7 +850,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	struct amdgpu_vm *vm = &fpriv->vm;
 	struct amdgpu_bo_list_entry *e;
 	struct drm_gem_object *obj;
-	unsigned long index;
 	unsigned int i;
 	int r;
 
@@ -961,7 +960,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		goto out_free_user_pages;
 	}
 
-	drm_exec_for_each_locked_object(&p->exec, index, obj) {
+	drm_exec_for_each_locked_object(&p->exec, obj) {
 		r = amdgpu_cs_bo_validate(p, gem_to_amdgpu_bo(obj));
 		if (unlikely(r))
 			goto out_free_user_pages;
@@ -1201,7 +1200,6 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 	struct drm_gpu_scheduler *sched;
 	struct drm_gem_object *obj;
 	struct dma_fence *fence;
-	unsigned long index;
 	unsigned int i;
 	int r;
 
@@ -1212,7 +1210,7 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 		return r;
 	}
 
-	drm_exec_for_each_locked_object(&p->exec, index, obj) {
+	drm_exec_for_each_locked_object(&p->exec, obj) {
 		struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
 
 		struct dma_resv *resv = bo->tbo.base.resv;
@@ -1281,7 +1279,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	struct amdgpu_job *leader = p->gang_leader;
 	struct amdgpu_bo_list_entry *e;
 	struct drm_gem_object *gobj;
-	unsigned long index;
 	unsigned int i;
 	uint64_t seq;
 	int r;
@@ -1331,7 +1328,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	}
 
 	p->fence = dma_fence_get(&leader->base.s_fence->finished);
-	drm_exec_for_each_locked_object(&p->exec, index, gobj) {
+	drm_exec_for_each_locked_object(&p->exec, gobj) {
 
 		ttm_bo_move_to_lru_tail_unlocked(&gem_to_amdgpu_bo(gobj)->tbo);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
index 4c5e38dea4c2..f6b7522c3c82 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
@@ -121,7 +121,6 @@ int amdgpu_evf_mgr_rearm(struct amdgpu_eviction_fence_mgr *evf_mgr,
 {
 	struct amdgpu_eviction_fence *ev_fence;
 	struct drm_gem_object *obj;
-	unsigned long index;
 
 	/* Create and initialize a new eviction fence */
 	ev_fence = kzalloc_obj(*ev_fence);
@@ -140,7 +139,7 @@ int amdgpu_evf_mgr_rearm(struct amdgpu_eviction_fence_mgr *evf_mgr,
 	evf_mgr->ev_fence = &ev_fence->base;
 
 	/* And add it to all existing BOs */
-	drm_exec_for_each_locked_object(exec, index, obj) {
+	drm_exec_for_each_locked_object(exec, obj) {
 		struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
 
 		amdgpu_evf_mgr_attach_fence(evf_mgr, bo);
diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
index 8d0601400182..746210f3f6c2 100644
--- a/drivers/gpu/drm/drm_exec.c
+++ b/drivers/gpu/drm/drm_exec.c
@@ -24,7 +24,6 @@
  *
  *	struct drm_gem_object *obj;
  *	struct drm_exec exec;
- *	unsigned long index;
  *	int ret;
  *
  *	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
@@ -40,7 +39,7 @@
  *			goto error;
  *	}
  *
- *	drm_exec_for_each_locked_object(&exec, index, obj) {
+ *	drm_exec_for_each_locked_object(&exec, obj) {
  *		dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ);
  *		...
  *	}
@@ -56,9 +55,8 @@
 static void drm_exec_unlock_all(struct drm_exec *exec)
 {
 	struct drm_gem_object *obj;
-	unsigned long index;
 
-	drm_exec_for_each_locked_object_reverse(exec, index, obj) {
+	drm_exec_for_each_locked_object_reverse(exec, obj) {
 		dma_resv_unlock(obj->resv);
 		drm_gem_object_put(obj);
 	}
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index 83020b6cf9ba..c422c5af1f4b 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -1557,9 +1557,8 @@ drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm,
 			 enum dma_resv_usage extobj_usage)
 {
 	struct drm_gem_object *obj;
-	unsigned long index;
 
-	drm_exec_for_each_locked_object(exec, index, obj) {
+	drm_exec_for_each_locked_object(exec, obj) {
 		dma_resv_assert_held(obj->resv);
 		dma_resv_add_fence(obj->resv, fence,
 				   drm_gpuvm_is_extobj(gpuvm, obj) ?
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index b01f31ed4417..080c2fff0e95 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -373,7 +373,6 @@ int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
 			  unsigned int num_fences)
 {
 	struct drm_gem_object *obj;
-	unsigned long index;
 	int ret;
 
 	do {
@@ -386,7 +385,7 @@ int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
 			return ret;
 	} while (!list_empty(&vm->gpuvm.evict.list));
 
-	drm_exec_for_each_locked_object(exec, index, obj) {
+	drm_exec_for_each_locked_object(exec, obj) {
 		ret = dma_resv_reserve_fences(obj->resv, num_fences);
 		if (ret)
 			return ret;
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index aa786b828a0a..dee6ebdbe416 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -65,31 +65,35 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
 	return index < exec->num_objects ? exec->objects[index] : NULL;
 }
 
+/* Helper for drm_exec_for_each_locked_object(). Internal use only. */
+#define __drm_exec_for_each_locked_object(exec, obj, __index)		\
+	for (unsigned long __index = 0; ((obj) = drm_exec_obj(exec, __index)); ++__index)
 /**
  * drm_exec_for_each_locked_object - iterate over all the locked objects
  * @exec: drm_exec object
- * @index: unsigned long index for the iteration
  * @obj: the current GEM object
  *
  * Iterate over all the locked GEM objects inside the drm_exec object.
  */
-#define drm_exec_for_each_locked_object(exec, index, obj)		\
-	for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
+#define drm_exec_for_each_locked_object(exec, obj)			\
+	__drm_exec_for_each_locked_object(exec, obj, __UNIQUE_ID(drm_exec))
 
+/* Helper for drm_exec_for_each_locked_object_reverse(). Internal use only. */
+#define __drm_exec_for_each_locked_object_reverse(exec, obj, __index)	\
+	for (unsigned long __index = (exec)->num_objects - 1;		\
+	     ((obj) = drm_exec_obj(exec, __index)); --__index)
 /**
  * drm_exec_for_each_locked_object_reverse - iterate over all the locked
  * objects in reverse locking order
  * @exec: drm_exec object
- * @index: unsigned long index for the iteration
  * @obj: the current GEM object
  *
  * Iterate over all the locked GEM objects inside the drm_exec object in
- * reverse locking order. Note that @index may go below zero and wrap,
+ * reverse locking order. Note that the internal index may wrap around,
  * but that will be caught by drm_exec_obj(), returning a NULL object.
  */
-#define drm_exec_for_each_locked_object_reverse(exec, index, obj)	\
-	for ((index) = (exec)->num_objects - 1;				\
-	     ((obj) = drm_exec_obj(exec, index)); --(index))
+#define drm_exec_for_each_locked_object_reverse(exec, obj)		\
+	__drm_exec_for_each_locked_object_reverse(exec, obj, __UNIQUE_ID(drm_exec))
 
 /**
  * drm_exec_until_all_locked - loop until all GEM objects are locked
-- 
2.54.0

Re: [PATCH v2 1/4] drm/exec: Remove the index parameter from drm_exec_for_each_locked_obj[_reverse]

[Christian König]

On 5/20/26 12:16, Thomas Hellström wrote:
> Nobody makes any use of it. Possible internal future users can
> instead use the _index variable. External users shouldn't use
> it since the array it's pointing into is internal drm_exec state.
> 
> v2:
> - Use a unique id for the loop variable (Christian)
> 
> Assisted-by: GitHub Copilot:claude-sonnet-4.6
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  9 +++------
>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    |  3 +--
>  drivers/gpu/drm/drm_exec.c                    |  6 ++----
>  drivers/gpu/drm/drm_gpuvm.c                   |  3 +--
>  drivers/gpu/drm/xe/xe_vm.c                    |  3 +--
>  include/drm/drm_exec.h                        | 20 +++++++++++--------
>  6 files changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 32af8cce3df8..97a851ae7bd8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -850,7 +850,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  	struct amdgpu_vm *vm = &fpriv->vm;
>  	struct amdgpu_bo_list_entry *e;
>  	struct drm_gem_object *obj;
> -	unsigned long index;
>  	unsigned int i;
>  	int r;
>  
> @@ -961,7 +960,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  		goto out_free_user_pages;
>  	}
>  
> -	drm_exec_for_each_locked_object(&p->exec, index, obj) {
> +	drm_exec_for_each_locked_object(&p->exec, obj) {
>  		r = amdgpu_cs_bo_validate(p, gem_to_amdgpu_bo(obj));
>  		if (unlikely(r))
>  			goto out_free_user_pages;
> @@ -1201,7 +1200,6 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>  	struct drm_gpu_scheduler *sched;
>  	struct drm_gem_object *obj;
>  	struct dma_fence *fence;
> -	unsigned long index;
>  	unsigned int i;
>  	int r;
>  
> @@ -1212,7 +1210,7 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>  		return r;
>  	}
>  
> -	drm_exec_for_each_locked_object(&p->exec, index, obj) {
> +	drm_exec_for_each_locked_object(&p->exec, obj) {
>  		struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>  
>  		struct dma_resv *resv = bo->tbo.base.resv;
> @@ -1281,7 +1279,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  	struct amdgpu_job *leader = p->gang_leader;
>  	struct amdgpu_bo_list_entry *e;
>  	struct drm_gem_object *gobj;
> -	unsigned long index;
>  	unsigned int i;
>  	uint64_t seq;
>  	int r;
> @@ -1331,7 +1328,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  	}
>  
>  	p->fence = dma_fence_get(&leader->base.s_fence->finished);
> -	drm_exec_for_each_locked_object(&p->exec, index, gobj) {
> +	drm_exec_for_each_locked_object(&p->exec, gobj) {
>  
>  		ttm_bo_move_to_lru_tail_unlocked(&gem_to_amdgpu_bo(gobj)->tbo);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> index 4c5e38dea4c2..f6b7522c3c82 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> @@ -121,7 +121,6 @@ int amdgpu_evf_mgr_rearm(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  {
>  	struct amdgpu_eviction_fence *ev_fence;
>  	struct drm_gem_object *obj;
> -	unsigned long index;
>  
>  	/* Create and initialize a new eviction fence */
>  	ev_fence = kzalloc_obj(*ev_fence);
> @@ -140,7 +139,7 @@ int amdgpu_evf_mgr_rearm(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  	evf_mgr->ev_fence = &ev_fence->base;
>  
>  	/* And add it to all existing BOs */
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> +	drm_exec_for_each_locked_object(exec, obj) {
>  		struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>  
>  		amdgpu_evf_mgr_attach_fence(evf_mgr, bo);
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index 8d0601400182..746210f3f6c2 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -24,7 +24,6 @@
>   *
>   *	struct drm_gem_object *obj;
>   *	struct drm_exec exec;
> - *	unsigned long index;
>   *	int ret;
>   *
>   *	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> @@ -40,7 +39,7 @@
>   *			goto error;
>   *	}
>   *
> - *	drm_exec_for_each_locked_object(&exec, index, obj) {
> + *	drm_exec_for_each_locked_object(&exec, obj) {
>   *		dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ);
>   *		...
>   *	}
> @@ -56,9 +55,8 @@
>  static void drm_exec_unlock_all(struct drm_exec *exec)
>  {
>  	struct drm_gem_object *obj;
> -	unsigned long index;
>  
> -	drm_exec_for_each_locked_object_reverse(exec, index, obj) {
> +	drm_exec_for_each_locked_object_reverse(exec, obj) {
>  		dma_resv_unlock(obj->resv);
>  		drm_gem_object_put(obj);
>  	}
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 83020b6cf9ba..c422c5af1f4b 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -1557,9 +1557,8 @@ drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm,
>  			 enum dma_resv_usage extobj_usage)
>  {
>  	struct drm_gem_object *obj;
> -	unsigned long index;
>  
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> +	drm_exec_for_each_locked_object(exec, obj) {
>  		dma_resv_assert_held(obj->resv);
>  		dma_resv_add_fence(obj->resv, fence,
>  				   drm_gpuvm_is_extobj(gpuvm, obj) ?
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index b01f31ed4417..080c2fff0e95 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -373,7 +373,6 @@ int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
>  			  unsigned int num_fences)
>  {
>  	struct drm_gem_object *obj;
> -	unsigned long index;
>  	int ret;
>  
>  	do {
> @@ -386,7 +385,7 @@ int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
>  			return ret;
>  	} while (!list_empty(&vm->gpuvm.evict.list));
>  
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> +	drm_exec_for_each_locked_object(exec, obj) {
>  		ret = dma_resv_reserve_fences(obj->resv, num_fences);
>  		if (ret)
>  			return ret;
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index aa786b828a0a..dee6ebdbe416 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -65,31 +65,35 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
>  	return index < exec->num_objects ? exec->objects[index] : NULL;
>  }
>  
> +/* Helper for drm_exec_for_each_locked_object(). Internal use only. */
> +#define __drm_exec_for_each_locked_object(exec, obj, __index)		\
> +	for (unsigned long __index = 0; ((obj) = drm_exec_obj(exec, __index)); ++__index)
>  /**
>   * drm_exec_for_each_locked_object - iterate over all the locked objects
>   * @exec: drm_exec object
> - * @index: unsigned long index for the iteration
>   * @obj: the current GEM object
>   *
>   * Iterate over all the locked GEM objects inside the drm_exec object.
>   */
> -#define drm_exec_for_each_locked_object(exec, index, obj)		\
> -	for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
> +#define drm_exec_for_each_locked_object(exec, obj)			\
> +	__drm_exec_for_each_locked_object(exec, obj, __UNIQUE_ID(drm_exec))
>  
> +/* Helper for drm_exec_for_each_locked_object_reverse(). Internal use only. */
> +#define __drm_exec_for_each_locked_object_reverse(exec, obj, __index)	\
> +	for (unsigned long __index = (exec)->num_objects - 1;		\
> +	     ((obj) = drm_exec_obj(exec, __index)); --__index)
>  /**
>   * drm_exec_for_each_locked_object_reverse - iterate over all the locked
>   * objects in reverse locking order
>   * @exec: drm_exec object
> - * @index: unsigned long index for the iteration
>   * @obj: the current GEM object
>   *
>   * Iterate over all the locked GEM objects inside the drm_exec object in
> - * reverse locking order. Note that @index may go below zero and wrap,
> + * reverse locking order. Note that the internal index may wrap around,
>   * but that will be caught by drm_exec_obj(), returning a NULL object.
>   */
> -#define drm_exec_for_each_locked_object_reverse(exec, index, obj)	\
> -	for ((index) = (exec)->num_objects - 1;				\
> -	     ((obj) = drm_exec_obj(exec, index)); --(index))
> +#define drm_exec_for_each_locked_object_reverse(exec, obj)		\
> +	__drm_exec_for_each_locked_object_reverse(exec, obj, __UNIQUE_ID(drm_exec))
>  
>  /**
>   * drm_exec_until_all_locked - loop until all GEM objects are locked

Claude review: drm/exec: Remove the index parameter from drm_exec_for_each_locked_obj[_reverse]

[Claude Code Review Bot]

Patch Review

**Assessment: Good.**

This hides the loop index variable from callers by introducing helper macros with `__UNIQUE_ID`-generated variable names. The approach is standard kernel practice for preventing name collisions.

The reverse iteration case is correct for `num_objects == 0`:

```c
#define __drm_exec_for_each_locked_object_reverse(exec, obj, __index)	\
	for (unsigned long __index = (exec)->num_objects - 1;		\
	     ((obj) = drm_exec_obj(exec, __index)); --__index)
```

When `num_objects` is 0, `__index` wraps to `ULONG_MAX`, and `drm_exec_obj()` returns NULL immediately. This matches the existing behavior.

All call sites are correctly updated, and the removed `unsigned long index` declarations are the only uses. No issues.

Minor nit: The `unsigned long` type for `__index` in the `for` init is a C99 declaration-in-for, which is fine since the kernel requires C11+.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 2/4] drm/exec: Make the drm_exec_until_all_locked() macro more readable

[Thomas Hellström]

Use __UNIQUE_ID as done elsewhere in the kernel rather than a
hand-rolled __PASTE to craft a unique id.

Also use __maybe_unused rather than (void) to signify that a
variable, althrough written to, may not actually be used.

v2:
- Move assignment to declaration (Christian)
- Declare the retry pointer as void *const.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 include/drm/drm_exec.h | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index dee6ebdbe416..18f84faabbb9 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -95,6 +95,17 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
 #define drm_exec_for_each_locked_object_reverse(exec, obj)		\
 	__drm_exec_for_each_locked_object_reverse(exec, obj, __UNIQUE_ID(drm_exec))
 
+/*
+ * Helper to drm_exec_until_all_locked(). Don't use directly.
+ *
+ * Since labels can't be defined local to the loop's body we use a jump pointer
+ * to make sure that the retry is only used from within the loop's body.
+ */
+#define __drm_exec_until_all_locked(exec, _label)			 \
+_label:									 \
+	for (void *const __maybe_unused __drm_exec_retry_ptr = &&_label; \
+	     drm_exec_cleanup(exec);)
+
 /**
  * drm_exec_until_all_locked - loop until all GEM objects are locked
  * @exec: drm_exec object
@@ -102,17 +113,9 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
  * Core functionality of the drm_exec object. Loops until all GEM objects are
  * locked and no more contention exists. At the beginning of the loop it is
  * guaranteed that no GEM object is locked.
- *
- * Since labels can't be defined local to the loops body we use a jump pointer
- * to make sure that the retry is only used from within the loops body.
  */
 #define drm_exec_until_all_locked(exec)					\
-__PASTE(__drm_exec_, __LINE__):						\
-	for (void *__drm_exec_retry_ptr; ({				\
-		__drm_exec_retry_ptr = &&__PASTE(__drm_exec_, __LINE__);\
-		(void)__drm_exec_retry_ptr;				\
-		drm_exec_cleanup(exec);					\
-	});)
+	__drm_exec_until_all_locked(exec, __UNIQUE_ID(drm_exec))
 
 /**
  * drm_exec_retry_on_contention - restart the loop to grap all locks
-- 
2.54.0

Re: [PATCH v2 2/4] drm/exec: Make the drm_exec_until_all_locked() macro more readable

[Christian König]

On 5/20/26 12:16, Thomas Hellström wrote:
> Use __UNIQUE_ID as done elsewhere in the kernel rather than a
> hand-rolled __PASTE to craft a unique id.
> 
> Also use __maybe_unused rather than (void) to signify that a
> variable, althrough written to, may not actually be used.
> 
> v2:
> - Move assignment to declaration (Christian)
> - Declare the retry pointer as void *const.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  include/drm/drm_exec.h | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index dee6ebdbe416..18f84faabbb9 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -95,6 +95,17 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
>  #define drm_exec_for_each_locked_object_reverse(exec, obj)		\
>  	__drm_exec_for_each_locked_object_reverse(exec, obj, __UNIQUE_ID(drm_exec))
>  
> +/*
> + * Helper to drm_exec_until_all_locked(). Don't use directly.
> + *
> + * Since labels can't be defined local to the loop's body we use a jump pointer
> + * to make sure that the retry is only used from within the loop's body.
> + */
> +#define __drm_exec_until_all_locked(exec, _label)			 \
> +_label:									 \
> +	for (void *const __maybe_unused __drm_exec_retry_ptr = &&_label; \
> +	     drm_exec_cleanup(exec);)
> +
>  /**
>   * drm_exec_until_all_locked - loop until all GEM objects are locked
>   * @exec: drm_exec object
> @@ -102,17 +113,9 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
>   * Core functionality of the drm_exec object. Loops until all GEM objects are
>   * locked and no more contention exists. At the beginning of the loop it is
>   * guaranteed that no GEM object is locked.
> - *
> - * Since labels can't be defined local to the loops body we use a jump pointer
> - * to make sure that the retry is only used from within the loops body.
>   */
>  #define drm_exec_until_all_locked(exec)					\
> -__PASTE(__drm_exec_, __LINE__):						\
> -	for (void *__drm_exec_retry_ptr; ({				\
> -		__drm_exec_retry_ptr = &&__PASTE(__drm_exec_, __LINE__);\
> -		(void)__drm_exec_retry_ptr;				\
> -		drm_exec_cleanup(exec);					\
> -	});)
> +	__drm_exec_until_all_locked(exec, __UNIQUE_ID(drm_exec))
>  
>  /**
>   * drm_exec_retry_on_contention - restart the loop to grap all locks

Claude review: drm/exec: Make the drm_exec_until_all_locked() macro more readable

[Claude Code Review Bot]

Patch Review

**Assessment: Good.**

```c
#define __drm_exec_until_all_locked(exec, _label)			 \
_label:									 \
	for (void *const __maybe_unused __drm_exec_retry_ptr = &&_label; \
	     drm_exec_cleanup(exec);)
```

Three improvements here:
1. `__UNIQUE_ID` for the label name instead of `__PASTE(__drm_exec_, __LINE__)` — consistent with patch 1 and standard kernel idiom.
2. `__maybe_unused` instead of `(void)__drm_exec_retry_ptr` cast — cleaner.
3. `void *const` — prevents accidental reassignment of the jump target.

Note that `__drm_exec_retry_ptr` itself is deliberately *not* uniquified, since `drm_exec_retry_on_contention()` (and the new `drm_exec_retry()` from patch 3) reference it by name. This is the correct design — the variable name forms the contract between the loop macro and the retry macros.

One minor observation: the comment block is moved from the `drm_exec_until_all_locked` kdoc to the helper, which is appropriate since the implementation detail about label scoping belongs with the helper.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 3/4] drm/exec, drm/xe: Avoid abusing the drm_exec retry pointer

[Thomas Hellström]

The xe driver was using the drm_exec retry pointer directly to
restart the locking loop after out-of-memory errors. This is
relying on undocumented behaviour.

Instead add a drm_exec_retry() macro that can be used in this
situation, and that also warns if the struct drm_exec is
not newly (re-)initialized.

Use that macro in xe.

v2:
- Only allow if the drm_exec context is newly initialized.
  (Christian)

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/drm_exec.c         |  3 ---
 drivers/gpu/drm/xe/xe_validation.h |  2 +-
 include/drm/drm_exec.h             | 19 +++++++++++++++++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
index 746210f3f6c2..7988f5e7d56a 100644
--- a/drivers/gpu/drm/drm_exec.c
+++ b/drivers/gpu/drm/drm_exec.c
@@ -48,9 +48,6 @@
  * See struct dma_exec for more details.
  */
 
-/* Dummy value used to initially enter the retry loop */
-#define DRM_EXEC_DUMMY ((void *)~0)
-
 /* Unlock all objects and drop references */
 static void drm_exec_unlock_all(struct drm_exec *exec)
 {
diff --git a/drivers/gpu/drm/xe/xe_validation.h b/drivers/gpu/drm/xe/xe_validation.h
index a30e732c4d51..4cd955ce6cd2 100644
--- a/drivers/gpu/drm/xe/xe_validation.h
+++ b/drivers/gpu/drm/xe/xe_validation.h
@@ -146,7 +146,7 @@ bool xe_validation_should_retry(struct xe_validation_ctx *ctx, int *ret);
 #define xe_validation_retry_on_oom(_ctx, _ret)				\
 	do {								\
 		if (xe_validation_should_retry(_ctx, _ret))		\
-			goto *__drm_exec_retry_ptr;			\
+			drm_exec_retry((_ctx)->exec);			\
 	} while (0)
 
 /**
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index 18f84faabbb9..99c7e1bb3c5b 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -9,6 +9,12 @@
 #define DRM_EXEC_INTERRUPTIBLE_WAIT	BIT(0)
 #define DRM_EXEC_IGNORE_DUPLICATES	BIT(1)
 
+/*
+ * Dummy value used to initially enter the retry loop.
+ * internal use only.
+ */
+#define DRM_EXEC_DUMMY ((void *)~0)
+
 struct drm_gem_object;
 
 /**
@@ -142,6 +148,19 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)
 	return !!exec->contended;
 }
 
+/**
+ * drm_exec_retry() - Unconditionally restart the loop to grab all locks.
+ * @exec: drm_exec object
+ *
+ * Unconditionally retry the loop to lock all objects. For consistency,
+ * the exec object needs to be newly initialized.
+ */
+#define drm_exec_retry(_exec)					\
+	do {							\
+		WARN_ON((_exec)->contended != DRM_EXEC_DUMMY);	\
+		goto *__drm_exec_retry_ptr;			\
+	} while (0)
+
 void drm_exec_init(struct drm_exec *exec, u32 flags, unsigned nr);
 void drm_exec_fini(struct drm_exec *exec);
 bool drm_exec_cleanup(struct drm_exec *exec);
-- 
2.54.0

Re: [PATCH v2 3/4] drm/exec, drm/xe: Avoid abusing the drm_exec retry pointer

[Christian König]

On 5/20/26 12:16, Thomas Hellström wrote:
> The xe driver was using the drm_exec retry pointer directly to
> restart the locking loop after out-of-memory errors. This is
> relying on undocumented behaviour.
> 
> Instead add a drm_exec_retry() macro that can be used in this
> situation, and that also warns if the struct drm_exec is
> not newly (re-)initialized.
> 
> Use that macro in xe.
> 
> v2:
> - Only allow if the drm_exec context is newly initialized.
>   (Christian)
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

BTW I've ran into just another complex use case were we need the retry handling with userqueues.

So I'm probably going to use that macro in amdgpu as well but with quite a bit of extended functionality *sigh*.

Anyway patch is good like it is for now, so Reviewed-by: Christian König <christian.koenig@amd.com>.

Thanks,
Christian.

> ---
>  drivers/gpu/drm/drm_exec.c         |  3 ---
>  drivers/gpu/drm/xe/xe_validation.h |  2 +-
>  include/drm/drm_exec.h             | 19 +++++++++++++++++++
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index 746210f3f6c2..7988f5e7d56a 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -48,9 +48,6 @@
>   * See struct dma_exec for more details.
>   */
>  
> -/* Dummy value used to initially enter the retry loop */
> -#define DRM_EXEC_DUMMY ((void *)~0)
> -
>  /* Unlock all objects and drop references */
>  static void drm_exec_unlock_all(struct drm_exec *exec)
>  {
> diff --git a/drivers/gpu/drm/xe/xe_validation.h b/drivers/gpu/drm/xe/xe_validation.h
> index a30e732c4d51..4cd955ce6cd2 100644
> --- a/drivers/gpu/drm/xe/xe_validation.h
> +++ b/drivers/gpu/drm/xe/xe_validation.h
> @@ -146,7 +146,7 @@ bool xe_validation_should_retry(struct xe_validation_ctx *ctx, int *ret);
>  #define xe_validation_retry_on_oom(_ctx, _ret)				\
>  	do {								\
>  		if (xe_validation_should_retry(_ctx, _ret))		\
> -			goto *__drm_exec_retry_ptr;			\
> +			drm_exec_retry((_ctx)->exec);			\
>  	} while (0)
>  
>  /**
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 18f84faabbb9..99c7e1bb3c5b 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -9,6 +9,12 @@
>  #define DRM_EXEC_INTERRUPTIBLE_WAIT	BIT(0)
>  #define DRM_EXEC_IGNORE_DUPLICATES	BIT(1)
>  
> +/*
> + * Dummy value used to initially enter the retry loop.
> + * internal use only.
> + */
> +#define DRM_EXEC_DUMMY ((void *)~0)
> +
>  struct drm_gem_object;
>  
>  /**
> @@ -142,6 +148,19 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)
>  	return !!exec->contended;
>  }
>  
> +/**
> + * drm_exec_retry() - Unconditionally restart the loop to grab all locks.
> + * @exec: drm_exec object
> + *
> + * Unconditionally retry the loop to lock all objects. For consistency,
> + * the exec object needs to be newly initialized.
> + */
> +#define drm_exec_retry(_exec)					\
> +	do {							\
> +		WARN_ON((_exec)->contended != DRM_EXEC_DUMMY);	\
> +		goto *__drm_exec_retry_ptr;			\
> +	} while (0)
> +
>  void drm_exec_init(struct drm_exec *exec, u32 flags, unsigned nr);
>  void drm_exec_fini(struct drm_exec *exec);
>  bool drm_exec_cleanup(struct drm_exec *exec);

Claude review: drm/exec, drm/xe: Avoid abusing the drm_exec retry pointer

[Claude Code Review Bot]

Patch Review

**Assessment: Good, one minor concern.**

This formalizes the xe driver's pattern of directly using `goto *__drm_exec_retry_ptr` for OOM retry by introducing `drm_exec_retry()`:

```c
#define drm_exec_retry(_exec)					\
	do {							\
		WARN_ON((_exec)->contended != DRM_EXEC_DUMMY);	\
	goto *__drm_exec_retry_ptr;			\
	} while (0)
```

The `WARN_ON` check is correct: `xe_validation_should_retry()` calls `drm_exec_fini()` + `drm_exec_init()` before returning true, so `contended == DRM_EXEC_DUMMY` at that point. Then the `goto` jumps back to the `drm_exec_until_all_locked` label, which calls `drm_exec_cleanup()`, which sees `DRM_EXEC_DUMMY` and calls `ww_acquire_init()` to properly initialize the ticket. The flow is safe.

**Minor concern:** `DRM_EXEC_DUMMY` is moved from `drm_exec.c` to `drm_exec.h`, exposing an implementation detail in the public header. The "internal use only" comment mitigates this, but drivers could start relying on it. An alternative would be to add a `drm_exec_is_newly_initialized()` inline helper that hides the constant, but since the `goto` must remain in a macro anyway, this is acceptable as-is. Worth mentioning explicitly in the commit message that this is an intentional trade-off.

The xe_validation.h change is clean:
```c
-			goto *__drm_exec_retry_ptr;
+			drm_exec_retry((_ctx)->exec);
```

And the comment update in xe_validation.c correctly narrows the scope of the "abuses" comment:
```c
- * This abuses both drm_exec and ww_mutex internals and should be
+ * This abuses ww_mutex internals and should be
```

---
Generated by Claude Code Patch Reviewer

[PATCH v2 4/4] drm/exec, drm/xe, drm/amdgpu: Add an accessor for struct drm_exec::ticket

[Thomas Hellström]

Drivers were accessing this drm_exec member directly.
While that may seem harmless, it will require action if
the drm_exec utility is made a subclass of a dma-resv transaction
utility as outlined in the cover-letter.

Provide an accessor, drm_exec_ticket() to avoid that.

v2:
- Fix amdgpu compile error (Intel CI)
- Update the commit message.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c           |  6 +++---
 drivers/gpu/drm/xe/xe_validation.c               |  4 ++--
 include/drm/drm_exec.h                           | 11 +++++++++++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 72a5a29e63f6..d54794e5b18b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2999,7 +2999,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence __rcu *
 	/* Validate PDs, PTs and evicted DMABuf imports last. Otherwise BO
 	 * validations above would invalidate DMABuf imports again.
 	 */
-	ret = process_validate_vms(process_info, &exec.ticket);
+	ret = process_validate_vms(process_info, drm_exec_ticket(&exec));
 	if (ret) {
 		pr_debug("Validating VMs failed, ret: %d\n", ret);
 		goto validate_map_fail;
@@ -3040,7 +3040,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence __rcu *
 			goto validate_map_fail;
 		}
 
-		ret = amdgpu_vm_handle_moved(adev, peer_vm, &exec.ticket);
+		ret = amdgpu_vm_handle_moved(adev, peer_vm, drm_exec_ticket(&exec));
 		if (ret) {
 			dev_dbg(adev->dev,
 				"Memory eviction: handle moved failed, pid %8d. Try again.\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 97a851ae7bd8..6ada57abce9d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1157,7 +1157,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 			return r;
 	}
 
-	r = amdgpu_vm_handle_moved(adev, vm, &p->exec.ticket);
+	r = amdgpu_vm_handle_moved(adev, vm, drm_exec_ticket(&p->exec));
 	if (r)
 		return r;
 
@@ -1359,7 +1359,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	cs->out.handle = seq;
 	leader->uf_sequence = seq;
 
-	amdgpu_vm_bo_trace_cs(&fpriv->vm, &p->exec.ticket);
+	amdgpu_vm_bo_trace_cs(&fpriv->vm, drm_exec_ticket(&p->exec));
 	for (i = 0; i < p->gang_size; ++i) {
 		amdgpu_job_free_resources(p->jobs[i]);
 		trace_amdgpu_cs_ioctl(p->jobs[i]);
@@ -1794,7 +1794,7 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
 	*map = mapping;
 
 	/* Double check that the BO is reserved by this CS */
-	if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != &parser->exec.ticket)
+	if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != drm_exec_ticket(&parser->exec))
 		return -EINVAL;
 
 	/* Make sure VRAM is allocated contigiously */
diff --git a/drivers/gpu/drm/xe/xe_validation.c b/drivers/gpu/drm/xe/xe_validation.c
index a611438eaafe..8dff4d0ec895 100644
--- a/drivers/gpu/drm/xe/xe_validation.c
+++ b/drivers/gpu/drm/xe/xe_validation.c
@@ -156,7 +156,7 @@ int xe_validation_ctx_init(struct xe_validation_ctx *ctx, struct xe_validation_d
 
 #ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
 /*
- * This abuses both drm_exec and ww_mutex internals and should be
+ * This abuses ww_mutex internals and should be
  * replaced by checking for -EDEADLK when we can make TTM
  * stop converting -EDEADLK to -ENOMEM.
  * An alternative is to not have exhaustive eviction with
@@ -164,7 +164,7 @@ int xe_validation_ctx_init(struct xe_validation_ctx *ctx, struct xe_validation_d
  */
 static bool xe_validation_contention_injected(struct drm_exec *exec)
 {
-	return !!exec->ticket.contending_lock;
+	return !!drm_exec_ticket(exec)->contending_lock;
 }
 
 #else
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index 99c7e1bb3c5b..8725ba92ff91 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -161,6 +161,17 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)
 		goto *__drm_exec_retry_ptr;			\
 	} while (0)
 
+/**
+ * drm_exec_ticket - return the ww_acquire_ctx for this exec context
+ * @exec: drm_exec object
+ *
+ * Return: Pointer to the ww_acquire_ctx embedded in @exec.
+ */
+static inline struct ww_acquire_ctx *drm_exec_ticket(struct drm_exec *exec)
+{
+	return &exec->ticket;
+}
+
 void drm_exec_init(struct drm_exec *exec, u32 flags, unsigned nr);
 void drm_exec_fini(struct drm_exec *exec);
 bool drm_exec_cleanup(struct drm_exec *exec);
-- 
2.54.0

Re: [PATCH v2 4/4] drm/exec, drm/xe, drm/amdgpu: Add an accessor for struct drm_exec::ticket

[Christian König]

On 5/20/26 12:16, Thomas Hellström wrote:
> Drivers were accessing this drm_exec member directly.
> While that may seem harmless, it will require action if
> the drm_exec utility is made a subclass of a dma-resv transaction
> utility as outlined in the cover-letter.
> 
> Provide an accessor, drm_exec_ticket() to avoid that.
> 
> v2:
> - Fix amdgpu compile error (Intel CI)
> - Update the commit message.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c |  4 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c           |  6 +++---
>  drivers/gpu/drm/xe/xe_validation.c               |  4 ++--
>  include/drm/drm_exec.h                           | 11 +++++++++++
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 72a5a29e63f6..d54794e5b18b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2999,7 +2999,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence __rcu *
>  	/* Validate PDs, PTs and evicted DMABuf imports last. Otherwise BO
>  	 * validations above would invalidate DMABuf imports again.
>  	 */
> -	ret = process_validate_vms(process_info, &exec.ticket);
> +	ret = process_validate_vms(process_info, drm_exec_ticket(&exec));
>  	if (ret) {
>  		pr_debug("Validating VMs failed, ret: %d\n", ret);
>  		goto validate_map_fail;
> @@ -3040,7 +3040,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence __rcu *
>  			goto validate_map_fail;
>  		}
>  
> -		ret = amdgpu_vm_handle_moved(adev, peer_vm, &exec.ticket);
> +		ret = amdgpu_vm_handle_moved(adev, peer_vm, drm_exec_ticket(&exec));
>  		if (ret) {
>  			dev_dbg(adev->dev,
>  				"Memory eviction: handle moved failed, pid %8d. Try again.\n",
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 97a851ae7bd8..6ada57abce9d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1157,7 +1157,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>  			return r;
>  	}
>  
> -	r = amdgpu_vm_handle_moved(adev, vm, &p->exec.ticket);
> +	r = amdgpu_vm_handle_moved(adev, vm, drm_exec_ticket(&p->exec));
>  	if (r)
>  		return r;
>  
> @@ -1359,7 +1359,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  	cs->out.handle = seq;
>  	leader->uf_sequence = seq;
>  
> -	amdgpu_vm_bo_trace_cs(&fpriv->vm, &p->exec.ticket);
> +	amdgpu_vm_bo_trace_cs(&fpriv->vm, drm_exec_ticket(&p->exec));
>  	for (i = 0; i < p->gang_size; ++i) {
>  		amdgpu_job_free_resources(p->jobs[i]);
>  		trace_amdgpu_cs_ioctl(p->jobs[i]);
> @@ -1794,7 +1794,7 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>  	*map = mapping;
>  
>  	/* Double check that the BO is reserved by this CS */
> -	if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != &parser->exec.ticket)
> +	if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != drm_exec_ticket(&parser->exec))
>  		return -EINVAL;
>  
>  	/* Make sure VRAM is allocated contigiously */
> diff --git a/drivers/gpu/drm/xe/xe_validation.c b/drivers/gpu/drm/xe/xe_validation.c
> index a611438eaafe..8dff4d0ec895 100644
> --- a/drivers/gpu/drm/xe/xe_validation.c
> +++ b/drivers/gpu/drm/xe/xe_validation.c
> @@ -156,7 +156,7 @@ int xe_validation_ctx_init(struct xe_validation_ctx *ctx, struct xe_validation_d
>  
>  #ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
>  /*
> - * This abuses both drm_exec and ww_mutex internals and should be
> + * This abuses ww_mutex internals and should be
>   * replaced by checking for -EDEADLK when we can make TTM
>   * stop converting -EDEADLK to -ENOMEM.
>   * An alternative is to not have exhaustive eviction with
> @@ -164,7 +164,7 @@ int xe_validation_ctx_init(struct xe_validation_ctx *ctx, struct xe_validation_d
>   */
>  static bool xe_validation_contention_injected(struct drm_exec *exec)
>  {
> -	return !!exec->ticket.contending_lock;
> +	return !!drm_exec_ticket(exec)->contending_lock;
>  }
>  
>  #else
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 99c7e1bb3c5b..8725ba92ff91 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -161,6 +161,17 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)
>  		goto *__drm_exec_retry_ptr;			\
>  	} while (0)
>  
> +/**
> + * drm_exec_ticket - return the ww_acquire_ctx for this exec context
> + * @exec: drm_exec object
> + *
> + * Return: Pointer to the ww_acquire_ctx embedded in @exec.
> + */
> +static inline struct ww_acquire_ctx *drm_exec_ticket(struct drm_exec *exec)
> +{
> +	return &exec->ticket;
> +}
> +
>  void drm_exec_init(struct drm_exec *exec, u32 flags, unsigned nr);
>  void drm_exec_fini(struct drm_exec *exec);
>  bool drm_exec_cleanup(struct drm_exec *exec);

Claude review: drm/exec, drm/xe, drm/amdgpu: Add an accessor for struct drm_exec::ticket

[Claude Code Review Bot]

Patch Review

**Assessment: Good.**

The accessor is trivially correct:

```c
static inline struct ww_acquire_ctx *drm_exec_ticket(struct drm_exec *exec)
{
	return &exec->ticket;
}
```

All driver-side conversions are mechanical and correct. The internal uses in `drm_exec.c` (`drm_exec_cleanup`, `drm_exec_fini`, `drm_exec_lock_contended`) are deliberately not converted, which is appropriate since those are the implementation of drm_exec itself and would not need to change when subclassing.

One observation about the amdgpu `dma_resv_locking_ctx` comparison:

```c
-	if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != &parser->exec.ticket)
+	if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != drm_exec_ticket(&parser->exec))
```

This pointer comparison remains correct since `drm_exec_ticket` returns `&exec->ticket`, which is the same address.

Minor style note: the kdoc says `drm_exec_ticket` without the `()` convention used elsewhere for function names, but this is consistent with many kernel kdoc examples and not an issue.

---
Generated by Claude Code Patch Reviewer

Re: [PATCH v2 0/4] drm/exec: drm_exec polishing

[Christian König]

On 5/20/26 12:16, Thomas Hellström wrote:
> During the work towards enabling exhaustive eviction using full
> ww locking in TTM, Christian indicated that the path for the
> drm_exec moving forward was to be a full drm_exec helper with
> things like userptr validation rather than a WW transaction
> abstraction. The idea was then briefly discussed to craft a
> WW transaction helper and then subclass that with drm_exec
> with the idea that the WW transaction helper could be used in
> TTM for eviction and for other uses that didn't mandate a full
> exec sequence.

BTW Natalie is now trying to polish my TTM patches for doing that because some test case for the SteamDeck is running into issues with pinning buffers for display.

> Regardless whether that actually happens or not, this series
> aims to clean up abuses of drm_exec internals in drivers
> so that future development of drm_exec isn't blocked by
> such driver usage.

Yeah completely agree, going to review those patches now.

Regards,
Christian.

> 
> Except for patch 2 which is a small cleanup only.
> 
> v2:
> - An drm/msm patch was already merged.
> - Use a unique id for the loop variable (patch 1) (Christian)
> - Move assignment to declaration (patch 2) (Christian)
> - Declare the retry pointer as void *const. (patch 2)
> - Only allow if the drm_exec context is newly initialized. (patch 3)
>   (Christian)
> - Fix amdgpu compile error (patch 4) (Intel CI)
> - Update the commit message (patch 4)
> 
> Thomas Hellström (4):
>   drm/exec: Remove the index parameter from
>     drm_exec_for_each_locked_obj[_reverse]
>   drm/exec: Make the drm_exec_until_all_locked() macro more readable
>   drm/exec, drm/xe: Avoid abusing the drm_exec retry pointer
>   drm/exec, drm/xe, drm/amdgpu: Add an accessor for struct
>     drm_exec::ticket
> 
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  4 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        | 15 ++--
>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    |  3 +-
>  drivers/gpu/drm/drm_exec.c                    |  9 +--
>  drivers/gpu/drm/drm_gpuvm.c                   |  3 +-
>  drivers/gpu/drm/xe/xe_validation.c            |  4 +-
>  drivers/gpu/drm/xe/xe_validation.h            |  2 +-
>  drivers/gpu/drm/xe/xe_vm.c                    |  3 +-
>  include/drm/drm_exec.h                        | 71 ++++++++++++++-----
>  9 files changed, 70 insertions(+), 44 deletions(-)
> 

Re: [PATCH v2 0/4] drm/exec: drm_exec polishing

[Thomas Hellström]

Hi,

On Wed, 2026-05-20 at 13:34 +0200, Christian König wrote:
> On 5/20/26 12:16, Thomas Hellström wrote:
> > During the work towards enabling exhaustive eviction using full
> > ww locking in TTM, Christian indicated that the path for the
> > drm_exec moving forward was to be a full drm_exec helper with
> > things like userptr validation rather than a WW transaction
> > abstraction. The idea was then briefly discussed to craft a
> > WW transaction helper and then subclass that with drm_exec
> > with the idea that the WW transaction helper could be used in
> > TTM for eviction and for other uses that didn't mandate a full
> > exec sequence.
> 
> BTW Natalie is now trying to polish my TTM patches for doing that
> because some test case for the SteamDeck is running
>  into issues with pinning buffers for display.

Thanks for the review, Christian.

FWIW I did a POC implementation moving drm_exec as a transaction helper
to the dma-buf layer, and then re-implemented drm_exec on top as a
wrapper here:

https://gitlab.freedesktop.org/thomash/xe-vibe/-/commits/ww_transaction?ref_type=heads

Since this would work on any object with a refererence count and a
dma_resv, and even mixing them, it would solve a couple of issues:

1) Decouple the ttm object refcount issue from the TTM exhaustive
eviction (Not saying we shouldn't get rid of the ttm object refcount
anyway).

2) Implement any eviction locking modes (like snapshotting, object
release after slow-lock and similar) separate from drm_exec, and keep
exec as a pure exec-buf helper.

3) Facilitate ww locking in dma_buf_map().

4) Directly lock VMs instead of vm objects.

I haven't posted the patches yet since I wanted to get some feedback
first of whether this is a reasonable direction.

Thanks,
Thomas

> 
> > Regardless whether that actually happens or not, this series
> > aims to clean up abuses of drm_exec internals in drivers
> > so that future development of drm_exec isn't blocked by
> > such driver usage.
> 
> Yeah completely agree, going to review those patches now.
> 
> Regards,
> Christian.
> 
> > 
> > Except for patch 2 which is a small cleanup only.
> > 
> > v2:
> > - An drm/msm patch was already merged.
> > - Use a unique id for the loop variable (patch 1) (Christian)
> > - Move assignment to declaration (patch 2) (Christian)
> > - Declare the retry pointer as void *const. (patch 2)
> > - Only allow if the drm_exec context is newly initialized. (patch
> > 3)
> >   (Christian)
> > - Fix amdgpu compile error (patch 4) (Intel CI)
> > - Update the commit message (patch 4)
> > 
> > Thomas Hellström (4):
> >   drm/exec: Remove the index parameter from
> >     drm_exec_for_each_locked_obj[_reverse]
> >   drm/exec: Make the drm_exec_until_all_locked() macro more
> > readable
> >   drm/exec, drm/xe: Avoid abusing the drm_exec retry pointer
> >   drm/exec, drm/xe, drm/amdgpu: Add an accessor for struct
> >     drm_exec::ticket
> > 
> >  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  4 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        | 15 ++--
> >  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    |  3 +-
> >  drivers/gpu/drm/drm_exec.c                    |  9 +--
> >  drivers/gpu/drm/drm_gpuvm.c                   |  3 +-
> >  drivers/gpu/drm/xe/xe_validation.c            |  4 +-
> >  drivers/gpu/drm/xe/xe_validation.h            |  2 +-
> >  drivers/gpu/drm/xe/xe_vm.c                    |  3 +-
> >  include/drm/drm_exec.h                        | 71 ++++++++++++++-
> > ----
> >  9 files changed, 70 insertions(+), 44 deletions(-)
> > 

Claude review: drm/exec: drm_exec polishing

[Claude Code Review Bot]

Overall Series Review

Subject: drm/exec: drm_exec polishing
Author: =?UTF-8?q?Thomas=20Hellstr=C3=B6m?= <thomas.hellstrom@linux.intel.com>
Patches: 11
Reviewed: 2026-05-25T21:59:34.662912

---

This is a well-structured cleanup series that improves the `drm_exec` API by hiding internal implementation details from drivers, preparing the way for a future refactoring where `drm_exec` could be subclassed from a WW transaction utility. The four patches progressively tighten the API surface: removing the exposed loop index, cleaning up the retry macro, formalizing the retry-on-OOM pattern, and adding a ticket accessor.

The series is clean, appropriately scoped, and the v2 changes properly address reviewer feedback. No functional bugs found. A few minor observations below.

**Verdict: Series looks good to merge.**

---

---
Generated by Claude Code Patch Reviewer