Re: [PATCH 01/13] drm/amdgpu: start to move VM internals into amdgpu_vm_internal.h

["Timur Kristóf" <timur.kristof@gmail.com>]

On Thursday, September 17, 2026 1:01:46 PM Central European Summer Time Huang 
Yu wrote:
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> 
> Avoid exposing some defines outside of the VM code.
> 
> Co-developed by Claude Sonnet 4.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Hi,

I believe I've already commented on this patch two times already, but it seems 
both comments were lost, so I'm going to repeat:

Please use the "Assisted-by" tag instead, see this page for details and an 
example: https://docs.kernel.org/process/coding-assistants.html

with that fixed, the patch is:
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>


> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h     |   2 +
>  .../gpu/drm/amd/amdgpu/amdgpu_trace_points.c  |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        | 116 -------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c    |   1 +
>  .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   | 155 ++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c     |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c   |   1 +
>  8 files changed, 162 insertions(+), 116 deletions(-)
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h index
> 5324030a13f5..784adf975b04 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -37,6 +37,8 @@
>  #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
>  	
> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->fini
> shed)
> 
> +struct amdgpu_vm_update_params;
> +
>  TRACE_EVENT(amdgpu_device_rreg,
>  	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>  	    TP_ARGS(did, reg, value),
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace_points.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace_points.c index
> b96d885f6e33..fe61ed5b487e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace_points.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace_points.c
> @@ -25,6 +25,7 @@
>  #include <drm/amdgpu_drm.h>
>  #include "amdgpu_cs.h"
>  #include "amdgpu.h"
> +#include "amdgpu_vm_internal.h"
> 
>  #define CREATE_TRACE_POINTS
>  #include "amdgpu_trace.h"
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 88249aa89ee3..517490946a9c
> 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_exec.h>
>  #include "amdgpu.h"
>  #include "amdgpu_vm.h"
> +#include "amdgpu_vm_internal.h"
>  #include "amdgpu_trace.h"
>  #include "amdgpu_amdkfd.h"
>  #include "amdgpu_gmc.h"
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index d32183cd9e0f..ac3b8e8525c1
> 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -262,80 +262,6 @@ struct amdgpu_task_info {
>  	struct kref	refcount;
>  };
> 
> -/**
> - * struct amdgpu_vm_update_params
> - *
> - * Encapsulate some VM table update parameters to reduce
> - * the number of function parameters
> - *
> - */
> -struct amdgpu_vm_update_params {
> -
> -	/**
> -	 * @adev: amdgpu device we do this update for
> -	 */
> -	struct amdgpu_device *adev;
> -
> -	/**
> -	 * @vm: optional amdgpu_vm we do this update for
> -	 */
> -	struct amdgpu_vm *vm;
> -
> -	/**
> -	 * @immediate: if changes should be made immediately
> -	 */
> -	bool immediate;
> -
> -	/**
> -	 * @unlocked: true if the root BO is not locked
> -	 */
> -	bool unlocked;
> -
> -	/**
> -	 * @pages_addr:
> -	 *
> -	 * DMA addresses to use for mapping
> -	 */
> -	dma_addr_t *pages_addr;
> -
> -	/**
> -	 * @job: job to used for hw submission
> -	 */
> -	struct amdgpu_job *job;
> -
> -	/**
> -	 * @num_dw_left: number of dw left for the IB
> -	 */
> -	unsigned int num_dw_left;
> -
> -	/**
> -	 * @needs_flush: true whenever we need to invalidate the TLB
> -	 */
> -	bool needs_flush;
> -
> -	/**
> -	 * @override_pte: true for memory that is not uncached and gmc 
override
> function is -	 * implemented to allow MTYPE to be overridden for 
NUMA local
> memory. -	 */
> -	bool override_pte;
> -
> -	/**
> -	 * @tlb_flush_waitlist: temporary storage for BOs until tlb_flush
> -	 */
> -	struct list_head tlb_flush_waitlist;
> -};
> -
> -struct amdgpu_vm_update_funcs {
> -	int (*map_table)(struct amdgpu_bo_vm *bo);
> -	int (*prepare)(struct amdgpu_vm_update_params *p,
> -		       struct amdgpu_sync *sync, u64 k_job_id);
> -	int (*update)(struct amdgpu_vm_update_params *p,
> -		      struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t 
addr,
> -		      unsigned count, uint32_t incr, uint64_t flags);
> -	int (*commit)(struct amdgpu_vm_update_params *p,
> -		      struct dma_fence **fence);
> -};
> -
>  struct amdgpu_vm_fault_info {
>  	/* fault address */
>  	uint64_t	addr;
> @@ -600,22 +526,6 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device
> *adev, void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
>  			  struct amdgpu_mem_stats 
stats[__AMDGPU_PL_NUM]);
> 
> -int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -		       struct amdgpu_bo_vm *vmbo, bool immediate);
> -int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			int level, bool immediate, struct 
amdgpu_bo_vm **vmbo,
> -			int32_t xcp_id);
> -void amdgpu_vm_pt_free_root(struct amdgpu_device *adev, struct amdgpu_vm
> *vm); -
> -int amdgpu_vm_pde_update(struct amdgpu_vm_update_params *params,
> -			 struct amdgpu_vm_bo_base *entry);
> -int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
> -			  uint64_t start, uint64_t end,
> -			  uint64_t dst, uint64_t flags);
> -void amdgpu_vm_pt_free_work(struct work_struct *work);
> -void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
> -			    struct amdgpu_vm_update_params *params);
> -
>  #if defined(CONFIG_DEBUG_FS)
>  void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm *vm, struct seq_file *m);
>  #endif
> @@ -651,32 +561,6 @@ static inline uint64_t amdgpu_vm_tlb_seq(struct
> amdgpu_vm *vm) return atomic64_read(&vm->tlb_seq);
>  }
> 
> -/*
> - * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
> - * happens while holding this lock anywhere to prevent deadlocks when
> - * an MMU notifier runs in reclaim-FS context.
> - */
> -static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
> -{
> -	mutex_lock(&vm->eviction_lock);
> -	vm->saved_flags = memalloc_noreclaim_save();
> -}
> -
> -static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
> -{
> -	if (mutex_trylock(&vm->eviction_lock)) {
> -		vm->saved_flags = memalloc_noreclaim_save();
> -		return true;
> -	}
> -	return false;
> -}
> -
> -static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
> -{
> -	memalloc_noreclaim_restore(vm->saved_flags);
> -	mutex_unlock(&vm->eviction_lock);
> -}
> -
>  void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>  				  unsigned int pasid,
>  				  uint64_t addr,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c index
> b31ff6f56f0d..d03b9bbe20c7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> @@ -21,6 +21,7 @@
>   */
> 
>  #include "amdgpu_vm.h"
> +#include "amdgpu_vm_internal.h"
>  #include "amdgpu.h"
>  #include "amdgpu_reset.h"
>  #include "amdgpu_object.h"
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h new file mode 100644
> index 000000000000..447195cc4593
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright 2016 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"), + * to deal in the Software without restriction, including
> without limitation + * the rights to use, copy, modify, merge, publish,
> distribute, sublicense, + * and/or sell copies of the Software, and to
> permit persons to whom the + * Software is furnished to do so, subject to
> the following conditions: + *
> + * The above copyright notice and this permission notice shall be included
> in + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. 
> IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
> ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF
> CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH
> THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König
> + */
> +#ifndef __AMDGPU_VM_INTERNAL_H__
> +#define __AMDGPU_VM_INTERNAL_H__
> +
> +#include <linux/types.h>
> +#include <linux/list.h>
> +#include "amdgpu_vm.h"
> +
> +struct amdgpu_device;
> +struct amdgpu_vm;
> +struct amdgpu_job;
> +struct amdgpu_sync;
> +struct amdgpu_bo_vm;
> +struct amdgpu_vm_bo_base;
> +struct dma_fence;
> +
> +/**
> + * struct amdgpu_vm_update_params
> + *
> + * Encapsulate some VM table update parameters to reduce
> + * the number of function parameters
> + *
> + */
> +struct amdgpu_vm_update_params {
> +
> +	/**
> +	 * @adev: amdgpu device we do this update for
> +	 */
> +	struct amdgpu_device *adev;
> +
> +	/**
> +	 * @vm: optional amdgpu_vm we do this update for
> +	 */
> +	struct amdgpu_vm *vm;
> +
> +	/**
> +	 * @immediate: if changes should be made immediately
> +	 */
> +	bool immediate;
> +
> +	/**
> +	 * @unlocked: true if the root BO is not locked
> +	 */
> +	bool unlocked;
> +
> +	/**
> +	 * @pages_addr:
> +	 *
> +	 * DMA addresses to use for mapping
> +	 */
> +	dma_addr_t *pages_addr;
> +
> +	/**
> +	 * @job: job to used for hw submission
> +	 */
> +	struct amdgpu_job *job;
> +
> +	/**
> +	 * @num_dw_left: number of dw left for the IB
> +	 */
> +	unsigned int num_dw_left;
> +
> +	/**
> +	 * @needs_flush: true whenever we need to invalidate the TLB
> +	 */
> +	bool needs_flush;
> +
> +	/**
> +	 * @override_pte: true for memory that is not uncached and gmc 
override
> function is +	 * implemented to allow MTYPE to be overridden for 
NUMA local
> memory. +	 */
> +	bool override_pte;
> +
> +	/**
> +	 * @tlb_flush_waitlist: temporary storage for BOs until tlb_flush
> +	 */
> +	struct list_head tlb_flush_waitlist;
> +};
> +
> +struct amdgpu_vm_update_funcs {
> +	int (*map_table)(struct amdgpu_bo_vm *bo);
> +	int (*prepare)(struct amdgpu_vm_update_params *p,
> +		       struct amdgpu_sync *sync, u64 k_job_id);
> +	int (*update)(struct amdgpu_vm_update_params *p,
> +		      struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t 
addr,
> +		      unsigned count, uint32_t incr, uint64_t flags);
> +	int (*commit)(struct amdgpu_vm_update_params *p,
> +		      struct dma_fence **fence);
> +};
> +
> +int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +		       struct amdgpu_bo_vm *vmbo, bool immediate);
> +int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +			int level, bool immediate, struct 
amdgpu_bo_vm **vmbo,
> +			int32_t xcp_id);
> +void amdgpu_vm_pt_free_root(struct amdgpu_device *adev, struct amdgpu_vm
> *vm); +int amdgpu_vm_pde_update(struct amdgpu_vm_update_params *params,
> +			 struct amdgpu_vm_bo_base *entry);
> +int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
> +			  uint64_t start, uint64_t end,
> +			  uint64_t dst, uint64_t flags);
> +void amdgpu_vm_pt_free_work(struct work_struct *work);
> +void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
> +			    struct amdgpu_vm_update_params *params);
> +int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct amdgpu_vm
> *vm); +
> +/*
> + * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
> + * happens while holding this lock anywhere to prevent deadlocks when
> + * an MMU notifier runs in reclaim-FS context.
> + */
> +static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
> +{
> +	mutex_lock(&vm->eviction_lock);
> +	vm->saved_flags = memalloc_noreclaim_save();
> +}
> +
> +static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
> +{
> +	if (mutex_trylock(&vm->eviction_lock)) {
> +		vm->saved_flags = memalloc_noreclaim_save();
> +		return true;
> +	}
> +	return false;
> +}
> +
> +static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
> +{
> +	memalloc_noreclaim_restore(vm->saved_flags);
> +	mutex_unlock(&vm->eviction_lock);
> +}
> +
> +#endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c index
> e43a60d09808..0d7fe32d0c7d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -26,6 +26,7 @@
>  #include "amdgpu.h"
>  #include "amdgpu_trace.h"
>  #include "amdgpu_vm.h"
> +#include "amdgpu_vm_internal.h"
>  #include "amdgpu_job.h"
> 
>  /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index
> 430274237689..a6ae2f62f80d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -21,6 +21,7 @@
>   */
> 
>  #include "amdgpu_vm.h"
> +#include "amdgpu_vm_internal.h"
>  #include "amdgpu_job.h"
>  #include "amdgpu_object.h"
>  #include "amdgpu_trace.h"