[PATCH v2] Update function level documentation for GPUVM v2
Christian König
christian.koenig at amd.com
Tue Jun 12 14:41:22 UTC 2018
Please update the subject line and add a "drm/amdgpu: " prefix.
Am 12.06.2018 um 16:05 schrieb Andrey Grodzovsky:
> Add/update function level documentation and add reference to amdgpu_vm.c
> in amdgpu.rst
>
> v2:
> Fix reference in rst file.
> Fix compilation warnings.
> Add space between function names and params list where
> it's missing.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
> ---
> Documentation/gpu/amdgpu.rst | 9 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 199 ++++++++++++++++++++++++++++-----
> 2 files changed, 179 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
> index a4852f9..3ffb2a2 100644
> --- a/Documentation/gpu/amdgpu.rst
> +++ b/Documentation/gpu/amdgpu.rst
> @@ -44,3 +44,12 @@ MMU Notifier
>
> .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> :internal:
> +
> +AMDGPU Virtual Memory
> +---------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> + :doc: GPUVM
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> + :internal:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index d88687b..345fb3c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -34,8 +34,9 @@
> #include "amdgpu_trace.h"
> #include "amdgpu_amdkfd.h"
>
> -/*
> - * GPUVM
> +/**
> + * DOC: GPUVM
> + *
> * GPUVM is similar to the legacy gart on older asics, however
> * rather than there being a single global gart table
> * for the entire GPU, there are multiple VM page tables active
> @@ -63,7 +64,8 @@ INTERVAL_TREE_DEFINE(struct amdgpu_bo_va_mapping, rb, uint64_t, __subtree_last,
> #undef START
> #undef LAST
>
> -/* Local structure. Encapsulate some VM table update parameters to reduce
> +/*
> + * Local structure. Encapsulate some VM table update parameters to reduce
> * the number of function parameters
> */
While at it you could change the field comments into proper kernel
documentation as well.
> struct amdgpu_pte_update_params {
> @@ -94,6 +96,16 @@ struct amdgpu_prt_cb {
> struct dma_fence_cb cb;
> };
>
> +/**
> + * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
Better use something like "Initialize a bo_va_base structure and add it
to the appropriate lists.".
> + *
> + * @base: base structure for tracking BO usage in a VM
> + * @vm: vm to which bo is to be added
> + * @bo: amdgpu buffer object
> + *
> + * Adds bo to the list of bos associated with the vm
> + *
> + */
> static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
> struct amdgpu_vm *vm,
> struct amdgpu_bo *bo)
> @@ -126,8 +138,10 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
> * amdgpu_vm_level_shift - return the addr shift for each level
> *
> * @adev: amdgpu_device pointer
> + * @level: VMPT level
> *
> - * Returns the number of bits the pfn needs to be right shifted for a level.
> + * Returns:
> + * The number of bits the pfn needs to be right shifted for a level.
> */
> static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
> unsigned level)
> @@ -155,8 +169,10 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
> * amdgpu_vm_num_entries - return the number of entries in a PD/PT
> *
> * @adev: amdgpu_device pointer
> + * @level: VMPT level
> *
> - * Calculate the number of entries in a page directory or page table.
> + * Returns:
> + * The number of entries in a page directory or page table.
> */
> static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
> unsigned level)
> @@ -179,8 +195,10 @@ static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
> * amdgpu_vm_bo_size - returns the size of the BOs in bytes
> *
> * @adev: amdgpu_device pointer
> + * @level: VMPT level
> *
> - * Calculate the size of the BO for a page directory or page table in bytes.
> + * Returns:
> + * The size of the BO for a page directory or page table in bytes.
> */
> static unsigned amdgpu_vm_bo_size(struct amdgpu_device *adev, unsigned level)
> {
> @@ -218,6 +236,9 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
> * @param: parameter for the validation callback
> *
> * Validate the page table BOs on command submission if neccessary.
> + *
> + * Returns:
> + * Validation result.
> */
> int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> int (*validate)(void *p, struct amdgpu_bo *bo),
> @@ -273,6 +294,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> * @vm: VM to check
> *
> * Check if all VM PDs/PTs are ready for updates
> + *
> + * Returns:
> + * True if eviction list is empty.
> */
> bool amdgpu_vm_ready(struct amdgpu_vm *vm)
> {
> @@ -283,10 +307,14 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
> * amdgpu_vm_clear_bo - initially clear the PDs/PTs
> *
> * @adev: amdgpu_device pointer
> + * @vm: VM to clear BO from
> * @bo: BO to clear
> * @level: level this BO is at
> *
> * Root PD needs to be reserved when calling this.
> + *
> + * Returns:
> + * 0 on success, errno otherwise.
> */
> static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> struct amdgpu_vm *vm, struct amdgpu_bo *bo,
> @@ -382,10 +410,16 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> *
> * @adev: amdgpu_device pointer
> * @vm: requested vm
> + * @parent: parent PT
> * @saddr: start of the address range
> * @eaddr: end of the address range
> + * @level: VMPT level
> + * @ats: indicate ATS support from PTE
> *
> * Make sure the page directories and page tables are allocated
> + *
> + * Returns:
> + * 0 on success, errno otherwise.
> */
> static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> @@ -494,6 +528,9 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
> * @size: Size from start address we need.
> *
> * Make sure the page tables are allocated.
> + *
> + * Returns:
> + * 0 on success, errno otherwise.
> */
> int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> @@ -559,6 +596,15 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev)
> }
> }
>
> +/**
> + * amdgpu_vm_need_pipeline_sync - Check if pipe sync is needed for job.
> + *
> + * @ring: ring on which the job will be submitted
> + * @job: job to submit
> + *
> + * Returns:
> + * True if sync is needed.
> + */
> bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> struct amdgpu_job *job)
> {
> @@ -586,6 +632,14 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> return vm_flush_needed || gds_switch_needed;
> }
>
> +/**
> + * amdgpu_vm_is_large_bar - Check if BAR is large enough
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * Returns:
> + * True if BAR is large enough.
> + */
Unrelated to this patch, but we should probably move that function into
amdgpu_gmc.h.
There are a couple of more occasions of that check waiting for cleanup
in amdgpu_cs.c.
Can you take care of cleaning that up as well? Thanks in advance.
> static bool amdgpu_vm_is_large_bar(struct amdgpu_device *adev)
> {
> return (adev->gmc.real_vram_size == adev->gmc.visible_vram_size);
> @@ -595,10 +649,12 @@ static bool amdgpu_vm_is_large_bar(struct amdgpu_device *adev)
> * amdgpu_vm_flush - hardware flush the vm
> *
> * @ring: ring to use for flush
> - * @vmid: vmid number to use
> - * @pd_addr: address of the page directory
> + * @need_pipe_sync: is pipe sync needed
> *
> * Emit a VM flush when it is necessary.
> + *
> + * Returns:
> + * 0 on success, errno otherwise.
> */
> int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync)
> {
> @@ -706,6 +762,9 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_
> * Returns the found bo_va or NULL if none is found
> *
> * Object has to be reserved!
> + *
> + * Returns:
> + * Found bo_va or NULL.
> */
> struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
> struct amdgpu_bo *bo)
> @@ -787,7 +846,10 @@ static void amdgpu_vm_do_copy_ptes(struct amdgpu_pte_update_params *params,
> * @addr: the unmapped addr
> *
> * Look up the physical address of the page that the pte resolves
> - * to and return the pointer for the page table entry.
> + * to.
> + *
> + * Returns:
> + * The pointer for the page table entry.
> */
> static uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr)
> {
> @@ -840,6 +902,17 @@ static void amdgpu_vm_cpu_set_ptes(struct amdgpu_pte_update_params *params,
> }
> }
>
> +
> +/**
> + * amdgpu_vm_wait_pd - Wait for PT BOs to be free.
> + *
> + * @adev: amdgpu_device pointer
> + * @vm: related vm
> + * @owner: fence owner
> + *
> + * Returns:
> + * 0 on success, errno otherwise.
> + */
> static int amdgpu_vm_wait_pd(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> void *owner)
> {
> @@ -893,7 +966,10 @@ static void amdgpu_vm_update_pde(struct amdgpu_pte_update_params *params,
> /*
> * amdgpu_vm_invalidate_level - mark all PD levels as invalid
> *
> + * @adev: amdgpu_device pointer
> + * @vm: related vm
> * @parent: parent PD
> + * @level: VMPT level
> *
> * Mark all PD level as invalid after an error.
> */
> @@ -928,7 +1004,9 @@ static void amdgpu_vm_invalidate_level(struct amdgpu_device *adev,
> * @vm: requested vm
> *
> * Makes sure all directories are up to date.
> - * Returns 0 for success, error for failure.
> + *
> + * Returns:
> + * 0 for success, error for failure.
> */
> int amdgpu_vm_update_directories(struct amdgpu_device *adev,
> struct amdgpu_vm *vm)
> @@ -1115,14 +1193,15 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
> * amdgpu_vm_update_ptes - make sure that page tables are valid
> *
> * @params: see amdgpu_pte_update_params definition
> - * @vm: requested vm
> * @start: start of GPU address range
> * @end: end of GPU address range
> * @dst: destination address to map to, the next dst inside the function
> * @flags: mapping flags
> *
> * Update the page tables in the range @start - @end.
> - * Returns 0 for success, -EINVAL for failure.
> + *
> + * Returns:
> + * 0 for success, -EINVAL for failure.
> */
> static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
> uint64_t start, uint64_t end,
> @@ -1176,7 +1255,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
> * @end: last PTE to handle
> * @dst: addr those PTEs should point to
> * @flags: hw mapping flags
> - * Returns 0 for success, -EINVAL for failure.
> + *
> + * Returns:
> + * 0 for success, -EINVAL for failure.
> */
> static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params *params,
> uint64_t start, uint64_t end,
> @@ -1248,7 +1329,9 @@ static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params *params,
> * @fence: optional resulting fence
> *
> * Fill in the page table entries between @start and @last.
> - * Returns 0 for success, -EINVAL for failure.
> + *
> + * Returns:
> + * 0 for success, -EINVAL for failure.
> */
> static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> struct dma_fence *exclusive,
> @@ -1403,7 +1486,9 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> *
> * Split the mapping into smaller chunks so that each update fits
> * into a SDMA IB.
> - * Returns 0 for success, -EINVAL for failure.
> + *
> + * Returns:
> + * 0 for success, -EINVAL for failure.
> */
> static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
> struct dma_fence *exclusive,
> @@ -1514,7 +1599,9 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
> * @clear: if true clear the entries
> *
> * Fill in the page table entries for @bo_va.
> - * Returns 0 for success, -EINVAL for failure.
> + *
> + * Returns:
> + * 0 for success, -EINVAL for failure.
> */
> int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> struct amdgpu_bo_va *bo_va,
> @@ -1609,6 +1696,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>
> /**
> * amdgpu_vm_update_prt_state - update the global PRT state
> + *
> + * @adev: amdgpu_device pointer
> */
> static void amdgpu_vm_update_prt_state(struct amdgpu_device *adev)
> {
> @@ -1623,6 +1712,8 @@ static void amdgpu_vm_update_prt_state(struct amdgpu_device *adev)
>
> /**
> * amdgpu_vm_prt_get - add a PRT user
> + *
> + * @adev: amdgpu_device pointer
> */
> static void amdgpu_vm_prt_get(struct amdgpu_device *adev)
> {
> @@ -1635,6 +1726,8 @@ static void amdgpu_vm_prt_get(struct amdgpu_device *adev)
>
> /**
> * amdgpu_vm_prt_put - drop a PRT user
> + *
> + * @adev: amdgpu_device pointer
> */
> static void amdgpu_vm_prt_put(struct amdgpu_device *adev)
> {
> @@ -1644,6 +1737,8 @@ static void amdgpu_vm_prt_put(struct amdgpu_device *adev)
>
> /**
> * amdgpu_vm_prt_cb - callback for updating the PRT status
> + *
> + * @fence: fence for the callback
> */
> static void amdgpu_vm_prt_cb(struct dma_fence *fence, struct dma_fence_cb *_cb)
> {
> @@ -1655,6 +1750,9 @@ static void amdgpu_vm_prt_cb(struct dma_fence *fence, struct dma_fence_cb *_cb)
>
> /**
> * amdgpu_vm_add_prt_cb - add callback for updating the PRT status
> + *
> + * @adev: amdgpu_device pointer
> + * @fence: fence for the callback
> */
> static void amdgpu_vm_add_prt_cb(struct amdgpu_device *adev,
> struct dma_fence *fence)
> @@ -1746,9 +1844,11 @@ static void amdgpu_vm_prt_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> * or if an error occurred)
> *
> * Make sure all freed BOs are cleared in the PT.
> - * Returns 0 for success.
> - *
> * PTs have to be reserved and mutex must be locked!
> + *
> + * Returns:
> + * 0 for success.
> + *
> */
> int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> @@ -1793,10 +1893,11 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> *
> * @adev: amdgpu_device pointer
> * @vm: requested vm
> - * @sync: sync object to add fences to
> *
> * Make sure all BOs which are moved are updated in the PTs.
> - * Returns 0 for success.
> + *
> + * Returns:
> + * 0 for success.
> *
> * PTs have to be reserved!
> */
> @@ -1851,7 +1952,9 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> *
> * Add @bo into the requested vm.
> * Add @bo to the list of bos associated with the vm
> - * Returns newly added bo_va or NULL for failure
> + *
> + * Returns:
> + * Newly added bo_va or NULL for failure
> *
> * Object has to be reserved!
> */
> @@ -1917,7 +2020,9 @@ static void amdgpu_vm_bo_insert_map(struct amdgpu_device *adev,
> * @flags: attributes of pages (read/write/valid/etc.)
> *
> * Add a mapping of the BO at the specefied addr into the VM.
> - * Returns 0 for success, error for failure.
> + *
> + * Returns:
> + * 0 for success, error for failure.
> *
> * Object has to be reserved and unreserved outside!
> */
> @@ -1979,7 +2084,9 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
> *
> * Add a mapping of the BO at the specefied addr into the VM. Replace existing
> * mappings as we do so.
> - * Returns 0 for success, error for failure.
> + *
> + * Returns:
> + * 0 for success, error for failure.
> *
> * Object has to be reserved and unreserved outside!
> */
> @@ -2036,7 +2143,9 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
> * @saddr: where to the BO is mapped
> *
> * Remove a mapping of the BO at the specefied addr from the VM.
> - * Returns 0 for success, error for failure.
> + *
> + * Returns:
> + * 0 for success, error for failure.
> *
> * Object has to be reserved and unreserved outside!
> */
> @@ -2090,7 +2199,9 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device *adev,
> * @size: size of the range
> *
> * Remove all mappings in a range, split them as appropriate.
> - * Returns 0 for success, error for failure.
> + *
> + * Returns:
> + * 0 for success, error for failure.
> */
> int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> @@ -2189,6 +2300,10 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
> * @vm: the requested VM
> *
> * Find a mapping by it's address.
> + *
> + * Returns:
> + * The amdgpu_bo_va_mapping matching for addr or NULL
> + *
> */
> struct amdgpu_bo_va_mapping *amdgpu_vm_bo_lookup_mapping(struct amdgpu_vm *vm,
> uint64_t addr)
> @@ -2240,7 +2355,6 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
> * amdgpu_vm_bo_invalidate - mark the bo as invalid
> *
> * @adev: amdgpu_device pointer
> - * @vm: requested vm
> * @bo: amdgpu buffer object
> *
> * Mark @bo as invalid.
> @@ -2281,6 +2395,14 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
> }
> }
>
> +/**
> + * amdgpu_vm_get_block_size - calculate VM page table size in bits
Better "calculate VM page table size as power of two".
Nice cleanup, with the minor issues fixed Reviewed-by: Christian König
<christian.koenig at amd.com>.
Thanks,
Christian.
> + *
> + * @vm_size: VM size
> + *
> + * Returns:
> + * VM page table size in bits
> + */
> static uint32_t amdgpu_vm_get_block_size(uint64_t vm_size)
> {
> /* Total bits covered by PD + PTs */
> @@ -2368,6 +2490,9 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
> * @vm_context: Indicates if it GFX or Compute context
> *
> * Init @vm fields.
> + *
> + * Returns:
> + * 0 for success, error for failure.
> */
> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> int vm_context, unsigned int pasid)
> @@ -2488,6 +2613,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> /**
> * amdgpu_vm_make_compute - Turn a GFX VM into a compute VM
> *
> + * @adev: amdgpu_device pointer
> + * @vm: requested vm
> + *
> * This only works on GFX VMs that don't have any BOs added and no
> * page tables allocated yet.
> *
> @@ -2500,7 +2628,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> * setting. May leave behind an unused shadow BO for the page
> * directory when switching from SDMA updates to CPU updates.
> *
> - * Returns 0 for success, -errno for errors.
> + * Returns:
> + * 0 for success, -errno for errors.
> */
> int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> {
> @@ -2655,8 +2784,10 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> * @adev: amdgpu_device pointer
> * @pasid: PASID do identify the VM
> *
> - * This function is expected to be called in interrupt context. Returns
> - * true if there was fault credit, false otherwise
> + * This function is expected to be called in interrupt context.
> + *
> + * Returns:
> + * True if there was fault credit, false otherwise
> */
> bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
> unsigned int pasid)
> @@ -2740,6 +2871,16 @@ void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
> amdgpu_vmid_mgr_fini(adev);
> }
>
> +/**
> + * amdgpu_vm_ioctl - Manages VMID reservation for vm hubs.
> + *
> + * @dev: drm device pointer
> + * @data: drm_amdgpu_vm
> + * @filp: drm file pointer
> + *
> + * Returns:
> + * 0 for success, -errno for errors.
> + */
> int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> {
> union drm_amdgpu_vm *args = data;
More information about the amd-gfx
mailing list