Re: [PATCH v3 1/6] drm/amdgpu: Implement a new 64bit sequence memory driver

["Christian König" <christian.koenig@amd.com>]

Am 20.04.23 um 16:47 schrieb Arunpravin Paneer Selvam:
> Developed a new driver which allocates a 64bit memory on
> each request in sequence order. At the moment, user queue
> fence memory is the main consumer of this seq64 driver.
>
> v2: Worked on review comments from Christian for the following
>      modifications
>
>      - Move driver name from "semaphore" to "seq64"
>      - Remove unnecessary PT/PD mapping
>      - Move enable_mes check into init/fini functions.

Please just drop the enable_mes check. We need this functionality for 
the TLB counter as well and that is completely independent of MES.

One more technical comment below.

Apart from that looks good to me, but could use a little bit kerneldoc.

>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/Makefile        |   2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   5 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |   7 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  13 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c  | 158 +++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.h  |  48 +++++++
>   6 files changed, 232 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 2cc7897de7e6..d39504e65db1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -59,7 +59,7 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o amdgpu_kms.o \
>   	amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
>   	amdgpu_fw_attestation.o amdgpu_securedisplay.o \
>   	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
> -	amdgpu_ring_mux.o
> +	amdgpu_ring_mux.o amdgpu_seq64.o
>   
>   amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index c5f9af0e74ee..3bc8a2d35bb3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -109,6 +109,7 @@
>   #include "amdgpu_fdinfo.h"
>   #include "amdgpu_mca.h"
>   #include "amdgpu_ras.h"
> +#include "amdgpu_seq64.h"
>   
>   #define MAX_GPU_INSTANCE		16
>   
> @@ -486,6 +487,7 @@ struct amdgpu_fpriv {
>   	struct amdgpu_vm	vm;
>   	struct amdgpu_bo_va	*prt_va;
>   	struct amdgpu_bo_va	*csa_va;
> +	struct amdgpu_bo_va	*seq64_va;
>   	struct mutex		bo_list_lock;
>   	struct idr		bo_list_handles;
>   	struct amdgpu_ctx_mgr	ctx_mgr;
> @@ -950,6 +952,9 @@ struct amdgpu_device {
>   	/* GDS */
>   	struct amdgpu_gds		gds;
>   
> +	/* for userq and VM fences */
> +	struct amdgpu_seq64		seq64;
> +
>   	/* KFD */
>   	struct amdgpu_kfd_dev		kfd;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 7f8fcac4f18b..828d0dd1455b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2253,6 +2253,12 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
>   					goto init_failed;
>   				}
>   			}
> +
> +			r = amdgpu_seq64_init(adev);
> +			if (r) {
> +				DRM_ERROR("allocate seq64 failed %d\n", r);
> +				goto init_failed;
> +			}
>   		}
>   	}
>   
> @@ -2709,6 +2715,7 @@ static int amdgpu_device_ip_fini(struct amdgpu_device *adev)
>   			amdgpu_device_wb_fini(adev);
>   			amdgpu_device_vram_scratch_fini(adev);
>   			amdgpu_ib_pool_fini(adev);
> +			amdgpu_seq64_fini(adev);
>   		}
>   
>   		r = adev->ip_blocks[i].version->funcs->sw_fini((void *)adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index b16b8155a157..d89f321304df 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1183,6 +1183,12 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   			goto error_vm;
>   	}
>   
> +	r = amdgpu_seq64_map(adev, &fpriv->vm, &fpriv->seq64_va,
> +			     AMDGPU_SEQ64_VADDR_START,
> +			     AMDGPU_SEQ64_SIZE);
> +	if (r)
> +		goto error_vm;
> +
>   	mutex_init(&fpriv->bo_list_lock);
>   	idr_init_base(&fpriv->bo_list_handles, 1);
>   
> @@ -1250,6 +1256,13 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>   		amdgpu_bo_unreserve(adev->virt.csa_obj);
>   	}
>   
> +	if (fpriv->seq64_va) {
> +		WARN_ON(amdgpu_bo_reserve(adev->seq64.sbo, true));
> +		amdgpu_vm_bo_del(adev, fpriv->seq64_va);
> +		fpriv->seq64_va = NULL;
> +		amdgpu_bo_unreserve(adev->seq64.sbo);
> +	}
> +
>   	pasid = fpriv->vm.pasid;
>   	pd = amdgpu_bo_ref(fpriv->vm.root.bo);
>   	if (!WARN_ON(amdgpu_bo_reserve(pd, true))) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> new file mode 100644
> index 000000000000..bf43856cebbc
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2023 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.
> + *
> + */
> +
> +#include "amdgpu.h"
> +#include "amdgpu_seq64.h"
> +
> +void amdgpu_seq64_fini(struct amdgpu_device *adev)
> +{
> +	if (!adev->enable_mes)
> +		return;
> +
> +	amdgpu_bo_free_kernel(&adev->seq64.sbo,
> +			      NULL,
> +			      (void **)&adev->seq64.cpu_base_addr);
> +}
> +
> +int amdgpu_seq64_init(struct amdgpu_device *adev)
> +{
> +	int r;
> +
> +	if (!adev->enable_mes)
> +		return -EINVAL;
> +
> +	if (adev->seq64.sbo)
> +		return 0;
> +	
> +	/*
> +	 * AMDGPU_MAX_SEQ64_SLOTS * sizeof(u64) * 8 = AMDGPU_MAX_SEQ64_SLOTS
> +	 * 64bit slots
> +	 */
> +	r = amdgpu_bo_create_kernel(adev, AMDGPU_SEQ64_SIZE,
> +				    PAGE_SIZE, AMDGPU_GEM_DOMAIN_GTT,
> +				    &adev->seq64.sbo, NULL,
> +				    (void **)&adev->seq64.cpu_base_addr);
> +	if (r) {
> +		dev_warn(adev->dev, "(%d) create seq64 failed\n", r);
> +		return r;
> +	}
> +	
> +	memset(adev->seq64.cpu_base_addr, 0, AMDGPU_SEQ64_SIZE);
> +	
> +	adev->seq64.num_sem = AMDGPU_MAX_SEQ64_SLOTS;
> +	memset(&adev->seq64.used, 0, sizeof(adev->seq64.used));
> +	
> +	return 0;
> +}
> +
> +int amdgpu_seq64_map(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +		     struct amdgpu_bo_va **bo_va, u64 seq64_addr,
> +		     uint32_t size)
> +{
> +	struct ttm_validate_buffer seq64_tv;
> +	struct amdgpu_bo_list_entry pd;
> +	struct ww_acquire_ctx ticket;
> +	struct list_head list;
> +	struct amdgpu_bo *bo;
> +	int r;
> +
> +	bo = adev->seq64.sbo;
> +	if (!bo)
> +		return -EINVAL;
> +
> +	INIT_LIST_HEAD(&list);
> +	INIT_LIST_HEAD(&seq64_tv.head);
> +
> +	seq64_tv.bo = &bo->tbo;
> +	seq64_tv.num_shared = 1;
> +
> +	list_add(&seq64_tv.head, &list);
> +	amdgpu_vm_get_pd_bo(vm, &list, &pd);
> +
> +	r = ttm_eu_reserve_buffers(&ticket, &list, true, NULL);
> +	if (r)
> +		return r;
> +
> +	*bo_va = amdgpu_vm_bo_add(adev, vm, bo);
> +	if (!*bo_va) {
> +		r = -ENOMEM;
> +		goto error_vm;
> +	}
> +
> +	r = amdgpu_vm_bo_map(adev, *bo_va, seq64_addr, 0, size,
> +			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
> +			     AMDGPU_PTE_EXECUTABLE);
> +	if (r) {
> +		DRM_ERROR("failed to do bo_map on userq sem, err=%d\n", r);
> +		goto error_map;
> +	}
> +
> +	r = amdgpu_vm_bo_update(adev, *bo_va, false);
> +	if (r) {
> +		DRM_ERROR("failed to do vm_bo_update on userq sem\n");
> +		goto error_map;
> +	}
> +
> +	ttm_eu_backoff_reservation(&ticket, &list);
> +
> +	return 0;
> +
> +error_map:
> +	amdgpu_vm_bo_del(adev, *bo_va);
> +error_vm:
> +	ttm_eu_backoff_reservation(&ticket, &list);
> +	return r;
> +}
> +
> +int amdgpu_seq64_get(struct amdgpu_device *adev, u64 *gpu_addr,
> +		     u64 **cpu_addr)
> +{
> +	unsigned long bit_pos;
> +	u32 offset;
> +
> +	bit_pos = find_first_zero_bit(adev->seq64.used, adev->seq64.num_sem);
> +
> +	if (bit_pos < adev->seq64.num_sem) {
> +		__set_bit(bit_pos, adev->seq64.used);
> +		offset = bit_pos << 6; /* convert to qw offset */
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	*gpu_addr = offset + AMDGPU_SEQ64_VADDR_START;
> +	*cpu_addr = offset + adev->seq64.cpu_base_addr;
> +
> +	return 0;
> +}
> +
> +void amdgpu_seq64_free(struct amdgpu_device *adev, u64 gpu_addr)
> +{
> +	u32 offset;
> +
> +	offset = gpu_addr - AMDGPU_SEQ64_VADDR_START;
> +
> +	offset >>= 6;
> +	if (offset < adev->seq64.num_sem)
> +		__clear_bit(offset, adev->seq64.used);
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.h
> new file mode 100644
> index 000000000000..e9b0afa9c5aa
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2023 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.
> + *
> + */
> +
> +#ifndef __AMDGPU_SEQ64_H__
> +#define __AMDGPU_SEQ64_H__
> +
> +#define AMDGPU_SEQ64_SIZE		(2ULL << 20)
> +#define AMDGPU_MAX_SEQ64_SLOTS		(AMDGPU_SEQ64_SIZE / (sizeof(u64) * 8))
> +#define AMDGPU_SEQ64_VADDR_OFFSET	0x50000
> +#define AMDGPU_SEQ64_VADDR_START	(AMDGPU_VA_RESERVED_SIZE + AMDGPU_SEQ64_VADDR_OFFSET)
> +
> +struct amdgpu_seq64 {
> +	struct amdgpu_bo *sbo;
> +	u32 num_sem;
> +	u64 *cpu_base_addr;
> +	unsigned long used[DIV_ROUND_UP(AMDGPU_MAX_SEQ64_SLOTS, BITS_PER_LONG)];

Please use DECLARE_BITMAP() instead of open coding this here.

Christian.

> +};
> +
> +void amdgpu_seq64_fini(struct amdgpu_device *adev);
> +int amdgpu_seq64_init(struct amdgpu_device *adev);
> +int amdgpu_seq64_get(struct amdgpu_device *adev, u64 *gpu_addr, u64 **cpu_addr);
> +void amdgpu_seq64_free(struct amdgpu_device *adev, u64 gpu_addr);
> +int amdgpu_seq64_map(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +		     struct amdgpu_bo_va **bo_va, u64 seq64_addr, uint32_t size);
> +
> +#endif
> +