[PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Christian König]

Similar to the CPU address space the VA on Vega10 has a hole in it.

v2: use dev_dbg instead of dev_err

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 10 +++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  5 +++++
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index ee7736419411..fea4429d3f72 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -869,8 +869,8 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
 			struct amdgpu_bo_va_mapping *m;
 			struct amdgpu_bo *aobj = NULL;
 			struct amdgpu_cs_chunk *chunk;
+			uint64_t offset, va_start;
 			struct amdgpu_ib *ib;
-			uint64_t offset;
 			uint8_t *kptr;
 
 			chunk = &p->chunks[i];
@@ -880,14 +880,14 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
 			if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
 				continue;
 
-			r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
-						   &aobj, &m);
+			va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
+			r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
 			if (r) {
 				DRM_ERROR("IB va_start is invalid\n");
 				return r;
 			}
 
-			if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
+			if ((va_start + chunk_ib->ib_bytes) >
 			    (m->last + 1) * AMDGPU_GPU_PAGE_SIZE) {
 				DRM_ERROR("IB va_start+ib_bytes is invalid\n");
 				return -EINVAL;
@@ -900,7 +900,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
 			}
 
 			offset = m->start * AMDGPU_GPU_PAGE_SIZE;
-			kptr += chunk_ib->va_start - offset;
+			kptr += va_start - offset;
 
 			memcpy(ib->ptr, kptr, chunk_ib->ib_bytes);
 			amdgpu_bo_kunmap(aobj);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 04ddd782bf6d..432ec2924dc5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -566,6 +566,17 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (args->va_address >= AMDGPU_VA_HOLE_START &&
+	    args->va_address < AMDGPU_VA_HOLE_END) {
+		dev_dbg(&dev->pdev->dev,
+			"va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
+			args->va_address, AMDGPU_VA_HOLE_START,
+			AMDGPU_VA_HOLE_END);
+		return -EINVAL;
+	}
+
+	args->va_address &= AMDGPU_VA_HOLE_MASK;
+
 	if ((args->flags & ~valid_flags) && (args->flags & ~prt_flags)) {
 		dev_err(&dev->pdev->dev, "invalid flags combination 0x%08X\n",
 			args->flags);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 65360cde5342..e84a7f7f642e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -578,7 +578,9 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 		if (amdgpu_sriov_vf(adev))
 			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
 		dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
-		dev_info.virtual_address_max = (uint64_t)adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
+		dev_info.virtual_address_max =
+			min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
+			    AMDGPU_VA_HOLE_START);
 		dev_info.virtual_address_alignment = max((int)PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE);
 		dev_info.pte_fragment_size = (1 << adev->vm_manager.fragment_size) * AMDGPU_GPU_PAGE_SIZE;
 		dev_info.gart_page_size = AMDGPU_GPU_PAGE_SIZE;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index e8f8896d18db..31cd57592546 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -96,6 +96,11 @@ struct amdgpu_bo_list_entry;
 /* hardcode that limit for now */
 #define AMDGPU_VA_RESERVED_SIZE			(8ULL << 20)
 
+/* VA hole for 48bit addresses on Vega10 */
+#define AMDGPU_VA_HOLE_START			0x0000800000000000ULL
+#define AMDGPU_VA_HOLE_END			0xffff800000000000ULL
+#define AMDGPU_VA_HOLE_MASK			0x0000ffffffffffffULL
+
 /* max vmids dedicated for process */
 #define AMDGPU_VM_MAX_RESERVED_VMID	1
 
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amdgpu: use dev_dbg instead of dev_err in the VA IOCTL

[Christian König]

Userspace buggy userspace can spam the logs.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 432ec2924dc5..a8dc7b95f29a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -560,7 +560,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	int r = 0;
 
 	if (args->va_address < AMDGPU_VA_RESERVED_SIZE) {
-		dev_err(&dev->pdev->dev,
+		dev_dbg(&dev->pdev->dev,
 			"va_address 0x%LX is in reserved area 0x%LX\n",
 			args->va_address, AMDGPU_VA_RESERVED_SIZE);
 		return -EINVAL;
@@ -578,7 +578,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	args->va_address &= AMDGPU_VA_HOLE_MASK;
 
 	if ((args->flags & ~valid_flags) && (args->flags & ~prt_flags)) {
-		dev_err(&dev->pdev->dev, "invalid flags combination 0x%08X\n",
+		dev_dbg(&dev->pdev->dev, "invalid flags combination 0x%08X\n",
 			args->flags);
 		return -EINVAL;
 	}
@@ -590,7 +590,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	case AMDGPU_VA_OP_REPLACE:
 		break;
 	default:
-		dev_err(&dev->pdev->dev, "unsupported operation %d\n",
+		dev_dbg(&dev->pdev->dev, "unsupported operation %d\n",
 			args->operation);
 		return -EINVAL;
 	}
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm/amdgpu: expose the VA above the hole to userspace

[Christian König]

Let userspace know how much area we have above the 48bit VA hole on
Vega10.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 ++++++++++--
 include/uapi/drm/amdgpu_drm.h           |  4 ++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index e84a7f7f642e..9875d64ae7d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -550,6 +550,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 	}
 	case AMDGPU_INFO_DEV_INFO: {
 		struct drm_amdgpu_info_device dev_info = {};
+		uint64_t vm_size;
 
 		dev_info.device_id = dev->pdev->device;
 		dev_info.chip_rev = adev->rev_id;
@@ -577,10 +578,17 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
 		if (amdgpu_sriov_vf(adev))
 			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
+
+		vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
 		dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
 		dev_info.virtual_address_max =
-			min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
-			    AMDGPU_VA_HOLE_START);
+			min(vm_size, AMDGPU_VA_HOLE_START);
+
+		vm_size -= AMDGPU_VA_RESERVED_SIZE;
+		if (vm_size > AMDGPU_VA_HOLE_START) {
+			dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
+			dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
+		}
 		dev_info.virtual_address_alignment = max((int)PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE);
 		dev_info.pte_fragment_size = (1 << adev->vm_manager.fragment_size) * AMDGPU_GPU_PAGE_SIZE;
 		dev_info.gart_page_size = AMDGPU_GPU_PAGE_SIZE;
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index f7a4cf1b6ef6..1dc8089b480a 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -880,6 +880,10 @@ struct drm_amdgpu_info_device {
 	__u32 _pad1;
 	/* always on cu bitmap */
 	__u32 cu_ao_bitmap[4][4];
+	/** Starting high virtual address for UMDs. */
+	__u64 high_va_offset;
+	/** The maximum high virtual address */
+	__u64 high_va_max;
 };
 
 struct drm_amdgpu_info_hw_ip {
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Christian König]

Ping? Michel, Alex can somebody take a look?

Thanks,
Christian.

Am 16.11.2017 um 11:22 schrieb Christian König:
> Similar to the CPU address space the VA on Vega10 has a hole in it.
>
> v2: use dev_dbg instead of dev_err
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 10 +++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  4 +++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  5 +++++
>   4 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index ee7736419411..fea4429d3f72 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -869,8 +869,8 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   			struct amdgpu_bo_va_mapping *m;
>   			struct amdgpu_bo *aobj = NULL;
>   			struct amdgpu_cs_chunk *chunk;
> +			uint64_t offset, va_start;
>   			struct amdgpu_ib *ib;
> -			uint64_t offset;
>   			uint8_t *kptr;
>   
>   			chunk = &p->chunks[i];
> @@ -880,14 +880,14 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   			if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
>   				continue;
>   
> -			r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
> -						   &aobj, &m);
> +			va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
> +			r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
>   			if (r) {
>   				DRM_ERROR("IB va_start is invalid\n");
>   				return r;
>   			}
>   
> -			if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
> +			if ((va_start + chunk_ib->ib_bytes) >
>   			    (m->last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>   				DRM_ERROR("IB va_start+ib_bytes is invalid\n");
>   				return -EINVAL;
> @@ -900,7 +900,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   			}
>   
>   			offset = m->start * AMDGPU_GPU_PAGE_SIZE;
> -			kptr += chunk_ib->va_start - offset;
> +			kptr += va_start - offset;
>   
>   			memcpy(ib->ptr, kptr, chunk_ib->ib_bytes);
>   			amdgpu_bo_kunmap(aobj);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 04ddd782bf6d..432ec2924dc5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -566,6 +566,17 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>   		return -EINVAL;
>   	}
>   
> +	if (args->va_address >= AMDGPU_VA_HOLE_START &&
> +	    args->va_address < AMDGPU_VA_HOLE_END) {
> +		dev_dbg(&dev->pdev->dev,
> +			"va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
> +			args->va_address, AMDGPU_VA_HOLE_START,
> +			AMDGPU_VA_HOLE_END);
> +		return -EINVAL;
> +	}
> +
> +	args->va_address &= AMDGPU_VA_HOLE_MASK;
> +
>   	if ((args->flags & ~valid_flags) && (args->flags & ~prt_flags)) {
>   		dev_err(&dev->pdev->dev, "invalid flags combination 0x%08X\n",
>   			args->flags);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 65360cde5342..e84a7f7f642e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -578,7 +578,9 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>   		if (amdgpu_sriov_vf(adev))
>   			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
>   		dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
> -		dev_info.virtual_address_max = (uint64_t)adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
> +		dev_info.virtual_address_max =
> +			min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
> +			    AMDGPU_VA_HOLE_START);
>   		dev_info.virtual_address_alignment = max((int)PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE);
>   		dev_info.pte_fragment_size = (1 << adev->vm_manager.fragment_size) * AMDGPU_GPU_PAGE_SIZE;
>   		dev_info.gart_page_size = AMDGPU_GPU_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index e8f8896d18db..31cd57592546 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -96,6 +96,11 @@ struct amdgpu_bo_list_entry;
>   /* hardcode that limit for now */
>   #define AMDGPU_VA_RESERVED_SIZE			(8ULL << 20)
>   
> +/* VA hole for 48bit addresses on Vega10 */
> +#define AMDGPU_VA_HOLE_START			0x0000800000000000ULL
> +#define AMDGPU_VA_HOLE_END			0xffff800000000000ULL
> +#define AMDGPU_VA_HOLE_MASK			0x0000ffffffffffffULL
> +
>   /* max vmids dedicated for process */
>   #define AMDGPU_VM_MAX_RESERVED_VMID	1
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Michel Dänzer]

On 17/11/17 11:28 AM, Christian König wrote:
> Ping? Michel, Alex can somebody take a look?

Patch 2 is

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


With patches 1 & 3, it's not 100% clear to me what the idea is behind
the handling of the hole on the kernel and userspace side. Maybe you can
add some explanation in code comments or the commit logs?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amdgpu: expose the VA above the hole to userspace

[Alex Deucher]

On Thu, Nov 16, 2017 at 5:22 AM, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> Let userspace know how much area we have above the 48bit VA hole on
> Vega10.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Please add a patch to bump the driver version as well.  With that,
this series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 ++++++++++--
>  include/uapi/drm/amdgpu_drm.h           |  4 ++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index e84a7f7f642e..9875d64ae7d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -550,6 +550,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>         }
>         case AMDGPU_INFO_DEV_INFO: {
>                 struct drm_amdgpu_info_device dev_info = {};
> +               uint64_t vm_size;
>
>                 dev_info.device_id = dev->pdev->device;
>                 dev_info.chip_rev = adev->rev_id;
> @@ -577,10 +578,17 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>                         dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>                 if (amdgpu_sriov_vf(adev))
>                         dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
> +
> +               vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>                 dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>                 dev_info.virtual_address_max =
> -                       min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
> -                           AMDGPU_VA_HOLE_START);
> +                       min(vm_size, AMDGPU_VA_HOLE_START);
> +
> +               vm_size -= AMDGPU_VA_RESERVED_SIZE;
> +               if (vm_size > AMDGPU_VA_HOLE_START) {
> +                       dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
> +                       dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
> +               }
>                 dev_info.virtual_address_alignment = max((int)PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE);
>                 dev_info.pte_fragment_size = (1 << adev->vm_manager.fragment_size) * AMDGPU_GPU_PAGE_SIZE;
>                 dev_info.gart_page_size = AMDGPU_GPU_PAGE_SIZE;
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index f7a4cf1b6ef6..1dc8089b480a 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -880,6 +880,10 @@ struct drm_amdgpu_info_device {
>         __u32 _pad1;
>         /* always on cu bitmap */
>         __u32 cu_ao_bitmap[4][4];
> +       /** Starting high virtual address for UMDs. */
> +       __u64 high_va_offset;
> +       /** The maximum high virtual address */
> +       __u64 high_va_max;
>  };
>
>  struct drm_amdgpu_info_hw_ip {
> --
> 2.11.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amdgpu: expose the VA above the hole to userspace

[Christian König]

Am 17.11.2017 um 17:11 schrieb Alex Deucher:
> On Thu, Nov 16, 2017 at 5:22 AM, Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Let userspace know how much area we have above the 48bit VA hole on
>> Vega10.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> Please add a patch to bump the driver version as well.

I could add one, but actually we don't need one cause the new fields are 
simply zero initialized by userspace when requestion the device info.

I'm actually considering add a CC stable tag to at least  the first 
patch, cause when a client actually would try to use this it could 
confuse the hardware badly.

Christian.

>    With that,
> this series is:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 ++++++++++--
>>   include/uapi/drm/amdgpu_drm.h           |  4 ++++
>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index e84a7f7f642e..9875d64ae7d5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -550,6 +550,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>>          }
>>          case AMDGPU_INFO_DEV_INFO: {
>>                  struct drm_amdgpu_info_device dev_info = {};
>> +               uint64_t vm_size;
>>
>>                  dev_info.device_id = dev->pdev->device;
>>                  dev_info.chip_rev = adev->rev_id;
>> @@ -577,10 +578,17 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>>                          dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>>                  if (amdgpu_sriov_vf(adev))
>>                          dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
>> +
>> +               vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>>                  dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>>                  dev_info.virtual_address_max =
>> -                       min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
>> -                           AMDGPU_VA_HOLE_START);
>> +                       min(vm_size, AMDGPU_VA_HOLE_START);
>> +
>> +               vm_size -= AMDGPU_VA_RESERVED_SIZE;
>> +               if (vm_size > AMDGPU_VA_HOLE_START) {
>> +                       dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
>> +                       dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>> +               }
>>                  dev_info.virtual_address_alignment = max((int)PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE);
>>                  dev_info.pte_fragment_size = (1 << adev->vm_manager.fragment_size) * AMDGPU_GPU_PAGE_SIZE;
>>                  dev_info.gart_page_size = AMDGPU_GPU_PAGE_SIZE;
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index f7a4cf1b6ef6..1dc8089b480a 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -880,6 +880,10 @@ struct drm_amdgpu_info_device {
>>          __u32 _pad1;
>>          /* always on cu bitmap */
>>          __u32 cu_ao_bitmap[4][4];
>> +       /** Starting high virtual address for UMDs. */
>> +       __u64 high_va_offset;
>> +       /** The maximum high virtual address */
>> +       __u64 high_va_max;
>>   };
>>
>>   struct drm_amdgpu_info_hw_ip {
>> --
>> 2.11.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Christian König]

Am 17.11.2017 um 17:09 schrieb Michel Dänzer:
> On 17/11/17 11:28 AM, Christian König wrote:
>> Ping? Michel, Alex can somebody take a look?
> Patch 2 is
>
> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
>
>
> With patches 1 & 3, it's not 100% clear to me what the idea is behind
> the handling of the hole on the kernel and userspace side. Maybe you can
> add some explanation in code comments or the commit logs?
Yeah, that is actually a bit of a mess because the hardware 
documentation wasn't very clear on how this works.

How about this as extra code comment on patch 1 to the assignment of 
dev_info.virtual_address_max:

/*
  * Old userspace isn't aware of the VA hole on Vega10. So in theory an 
client could get invalid VA addresses assigned.
  * To fix this and keep backward compatibility we limit the VA space 
reported in this field to the range below the hole.
  */

The last patch is then to report the VA space above the hole, cause that 
is actually what libdrm should use.

The crux is when I put the VA space above the hole directly into the old 
fields older versions of libdrm would break and we can't do that.

Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Michel Dänzer]

On 18/11/17 03:31 PM, Christian König wrote:
> Am 17.11.2017 um 17:09 schrieb Michel Dänzer:
>> On 17/11/17 11:28 AM, Christian König wrote:
>>> Ping? Michel, Alex can somebody take a look?
>> Patch 2 is
>>
>> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
>>
>>
>> With patches 1 & 3, it's not 100% clear to me what the idea is behind
>> the handling of the hole on the kernel and userspace side. Maybe you can
>> add some explanation in code comments or the commit logs?
> Yeah, that is actually a bit of a mess because the hardware
> documentation wasn't very clear on how this works.
> 
> How about this as extra code comment on patch 1 to the assignment of
> dev_info.virtual_address_max:
> 
> /*
>  * Old userspace isn't aware of the VA hole on Vega10. So in theory an
> client could get invalid VA addresses assigned.
>  * To fix this and keep backward compatibility we limit the VA space
> reported in this field to the range below the hole.
>  */
> 
> The last patch is then to report the VA space above the hole, cause that
> is actually what libdrm should use.
> 
> The crux is when I put the VA space above the hole directly into the old
> fields older versions of libdrm would break and we can't do that.

That much was actually clear. :) Maybe some questions will expose what
I'm missing:


Patch 1:

> @@ -880,14 +880,14 @@  static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>  			if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
>  				continue;
>  
> -			r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
> -						   &aobj, &m);
> +			va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
> +			r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);

I don't understand how userspace can make use of the address space above
the hole, since AMDGPU_VA_HOLE_MASK is 0x0000ffffffffffffULL and
AMDGPU_VA_HOLE_END is 0xffff800000000000ULL, so masking with
AMDGPU_VA_HOLE_MASK always results in an address below or inside the hole?


> @@ -566,6 +566,17 @@  int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> +	if (args->va_address >= AMDGPU_VA_HOLE_START &&
> +	    args->va_address < AMDGPU_VA_HOLE_END) {
> +		dev_dbg(&dev->pdev->dev,
> +			"va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
> +			args->va_address, AMDGPU_VA_HOLE_START,
> +			AMDGPU_VA_HOLE_END);
> +		return -EINVAL;
> +	}
> +
> +	args->va_address &= AMDGPU_VA_HOLE_MASK;

Ignoring the above, I think the masking with AMDGPU_VA_HOLE_MASK would
need to be done before checking for the hole, otherwise the masked
address could be inside the hole?


Patch 3:

> @@ -577,10 +578,17 @@  static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>  			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>  		if (amdgpu_sriov_vf(adev))
>  			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
> +
> +		vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>  		dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>  		dev_info.virtual_address_max =
> -			min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
> -			    AMDGPU_VA_HOLE_START);
> +			min(vm_size, AMDGPU_VA_HOLE_START);
> +
> +		vm_size -= AMDGPU_VA_RESERVED_SIZE;
> +		if (vm_size > AMDGPU_VA_HOLE_START) {
> +			dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
> +			dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
> +		}

This mostly makes sense, except for the

		dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;

line. Can you explain how simply or'ing together the address of the end
of the hole and the VM size works correctly in all cases?

As an extreme example, with vm_size == 1, this value would compute as
0xffff800000000001ULL, which seems wrong or at least weird.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Christian König]

Am 20.11.2017 um 11:36 schrieb Michel Dänzer:
> On 18/11/17 03:31 PM, Christian König wrote:
>> Am 17.11.2017 um 17:09 schrieb Michel Dänzer:
>>> On 17/11/17 11:28 AM, Christian König wrote:
>>>> Ping? Michel, Alex can somebody take a look?
>>> Patch 2 is
>>>
>>> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>>
>>> With patches 1 & 3, it's not 100% clear to me what the idea is behind
>>> the handling of the hole on the kernel and userspace side. Maybe you can
>>> add some explanation in code comments or the commit logs?
>> Yeah, that is actually a bit of a mess because the hardware
>> documentation wasn't very clear on how this works.
>>
>> How about this as extra code comment on patch 1 to the assignment of
>> dev_info.virtual_address_max:
>>
>> /*
>>   * Old userspace isn't aware of the VA hole on Vega10. So in theory an
>> client could get invalid VA addresses assigned.
>>   * To fix this and keep backward compatibility we limit the VA space
>> reported in this field to the range below the hole.
>>   */
>>
>> The last patch is then to report the VA space above the hole, cause that
>> is actually what libdrm should use.
>>
>> The crux is when I put the VA space above the hole directly into the old
>> fields older versions of libdrm would break and we can't do that.
> That much was actually clear. :) Maybe some questions will expose what
> I'm missing:
>
>
> Patch 1:
>
>> @@ -880,14 +880,14 @@  static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>>   			if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
>>   				continue;
>>   
>> -			r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
>> -						   &aobj, &m);
>> +			va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
>> +			r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
> I don't understand how userspace can make use of the address space above
> the hole, since AMDGPU_VA_HOLE_MASK is 0x0000ffffffffffffULL and
> AMDGPU_VA_HOLE_END is 0xffff800000000000ULL, so masking with
> AMDGPU_VA_HOLE_MASK always results in an address below or inside the hole?

Yes, and exactly that is intended here.

See for programming the MC and filling the page tables you handle this 
as if there isn't a hole in the address space.

Only when you start to use the address from userspace you will find that 
you need to sign extend bit 47 into bits 48-63.

Applying the AMDGPU_VA_HOLE_MASK to and address masks out the upper 16 
bits and so removes the sign extension again.

>
>
>> @@ -566,6 +566,17 @@  int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>   		return -EINVAL;
>>   	}
>>   
>> +	if (args->va_address >= AMDGPU_VA_HOLE_START &&
>> +	    args->va_address < AMDGPU_VA_HOLE_END) {
>> +		dev_dbg(&dev->pdev->dev,
>> +			"va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
>> +			args->va_address, AMDGPU_VA_HOLE_START,
>> +			AMDGPU_VA_HOLE_END);
>> +		return -EINVAL;
>> +	}
>> +
>> +	args->va_address &= AMDGPU_VA_HOLE_MASK;
> Ignoring the above, I think the masking with AMDGPU_VA_HOLE_MASK would
> need to be done before checking for the hole, otherwise the masked
> address could be inside the hole?

The masking just removes the sign extension which created the hole in 
the first place.

The VM and MC is programmed as if there isn't a hole.

> Patch 3:
>
>> @@ -577,10 +578,17 @@  static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>>   			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>>   		if (amdgpu_sriov_vf(adev))
>>   			dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
>> +
>> +		vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>>   		dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>>   		dev_info.virtual_address_max =
>> -			min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
>> -			    AMDGPU_VA_HOLE_START);
>> +			min(vm_size, AMDGPU_VA_HOLE_START);
>> +
>> +		vm_size -= AMDGPU_VA_RESERVED_SIZE;
>> +		if (vm_size > AMDGPU_VA_HOLE_START) {
>> +			dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
>> +			dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>> +		}
> This mostly makes sense, except for the
>
> 		dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>
> line. Can you explain how simply or'ing together the address of the end
> of the hole and the VM size works correctly in all cases?
>
> As an extreme example, with vm_size == 1, this value would compute as
> 0xffff800000000001ULL, which seems wrong or at least weird.

We check above if vm_size is larger than AMDGPU_VA_HOLE_START, in other 
words larger than 0x8000000000.

Since vm_size is once more in the value range the MC expects it (without 
the hole) that should give us the right result to sign extend bit 47 
into bits 48-63, e.g. 0xffff800000000.


And yes that initially confused me as well. Especially that you need to 
program the MC and fill the page tables as if there wouldn't be a hole 
in the address space.

Applying "& AMDGPU_VA_HOLE_MASK" and "| AMDGPU_VA_HOLE_END" just 
converts between the representation with and without the hole in it.

Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Michel Dänzer]

On 20/11/17 02:20 PM, Christian König wrote:
> Am 20.11.2017 um 11:36 schrieb Michel Dänzer:
>> On 18/11/17 03:31 PM, Christian König wrote:
>>> Am 17.11.2017 um 17:09 schrieb Michel Dänzer:
>>>> On 17/11/17 11:28 AM, Christian König wrote:
>>>>> Ping? Michel, Alex can somebody take a look?
>>>> Patch 2 is
>>>>
>>>> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
>>>>
>>>>
>>>> With patches 1 & 3, it's not 100% clear to me what the idea is behind
>>>> the handling of the hole on the kernel and userspace side. Maybe you
>>>> can
>>>> add some explanation in code comments or the commit logs?
>>> Yeah, that is actually a bit of a mess because the hardware
>>> documentation wasn't very clear on how this works.
>>>
>>> How about this as extra code comment on patch 1 to the assignment of
>>> dev_info.virtual_address_max:
>>>
>>> /*
>>>   * Old userspace isn't aware of the VA hole on Vega10. So in theory an
>>> client could get invalid VA addresses assigned.
>>>   * To fix this and keep backward compatibility we limit the VA space
>>> reported in this field to the range below the hole.
>>>   */
>>>
>>> The last patch is then to report the VA space above the hole, cause that
>>> is actually what libdrm should use.
>>>
>>> The crux is when I put the VA space above the hole directly into the old
>>> fields older versions of libdrm would break and we can't do that.
>> That much was actually clear. :) Maybe some questions will expose what
>> I'm missing:
>>
>>
>> Patch 1:
>>
>>> @@ -880,14 +880,14 @@  static int amdgpu_cs_ib_vm_chunk(struct
>>> amdgpu_device *adev,
>>>               if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
>>>                   continue;
>>>   -            r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
>>> -                           &aobj, &m);
>>> +            va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
>>> +            r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
>> I don't understand how userspace can make use of the address space above
>> the hole, since AMDGPU_VA_HOLE_MASK is 0x0000ffffffffffffULL and
>> AMDGPU_VA_HOLE_END is 0xffff800000000000ULL, so masking with
>> AMDGPU_VA_HOLE_MASK always results in an address below or inside the
>> hole?
> 
> Yes, and exactly that is intended here.
> 
> See for programming the MC and filling the page tables you handle this
> as if there isn't a hole in the address space.
> 
> Only when you start to use the address from userspace you will find that
> you need to sign extend bit 47 into bits 48-63.
> 
> Applying the AMDGPU_VA_HOLE_MASK to and address masks out the upper 16
> bits and so removes the sign extension again.
> 
>>
>>
>>> @@ -566,6 +566,17 @@  int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>> void *data,
>>>           return -EINVAL;
>>>       }
>>>   +    if (args->va_address >= AMDGPU_VA_HOLE_START &&
>>> +        args->va_address < AMDGPU_VA_HOLE_END) {
>>> +        dev_dbg(&dev->pdev->dev,
>>> +            "va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
>>> +            args->va_address, AMDGPU_VA_HOLE_START,
>>> +            AMDGPU_VA_HOLE_END);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    args->va_address &= AMDGPU_VA_HOLE_MASK;
>> Ignoring the above, I think the masking with AMDGPU_VA_HOLE_MASK would
>> need to be done before checking for the hole, otherwise the masked
>> address could be inside the hole?
> 
> The masking just removes the sign extension which created the hole in
> the first place.
> 
> The VM and MC is programmed as if there isn't a hole.
> 
>> Patch 3:
>>
>>> @@ -577,10 +578,17 @@  static int amdgpu_info_ioctl(struct drm_device
>>> *dev, void *data, struct drm_file
>>>               dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>>>           if (amdgpu_sriov_vf(adev))
>>>               dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
>>> +
>>> +        vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>>>           dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>>>           dev_info.virtual_address_max =
>>> -            min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
>>> -                AMDGPU_VA_HOLE_START);
>>> +            min(vm_size, AMDGPU_VA_HOLE_START);
>>> +
>>> +        vm_size -= AMDGPU_VA_RESERVED_SIZE;
>>> +        if (vm_size > AMDGPU_VA_HOLE_START) {
>>> +            dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
>>> +            dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>>> +        }
>> This mostly makes sense, except for the
>>
>>         dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>>
>> line. Can you explain how simply or'ing together the address of the end
>> of the hole and the VM size works correctly in all cases?
>>
>> As an extreme example, with vm_size == 1, this value would compute as
>> 0xffff800000000001ULL, which seems wrong or at least weird.
> 
> We check above if vm_size is larger than AMDGPU_VA_HOLE_START, in other
> words larger than 0x8000000000.
> 
> Since vm_size is once more in the value range the MC expects it (without
> the hole) that should give us the right result to sign extend bit 47
> into bits 48-63, e.g. 0xffff800000000.
> 
> 
> And yes that initially confused me as well. Especially that you need to
> program the MC and fill the page tables as if there wouldn't be a hole
> in the address space.
> 
> Applying "& AMDGPU_VA_HOLE_MASK" and "| AMDGPU_VA_HOLE_END" just
> converts between the representation with and without the hole in it.

Thanks for the explanation, I get it now.

For the benefit of future readers of this code, it would be nice to have
a short version of this explanation as a comment, e.g. above the
AMDGPU_VA_HOLE_* defines.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Christian König]

Am 20.11.2017 um 15:46 schrieb Michel Dänzer:
> On 20/11/17 02:20 PM, Christian König wrote:
>> Am 20.11.2017 um 11:36 schrieb Michel Dänzer:
>>> On 18/11/17 03:31 PM, Christian König wrote:
>>>> Am 17.11.2017 um 17:09 schrieb Michel Dänzer:
>>>>> On 17/11/17 11:28 AM, Christian König wrote:
>>>>>> Ping? Michel, Alex can somebody take a look?
>>>>> Patch 2 is
>>>>>
>>>>> Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
>>>>>
>>>>>
>>>>> With patches 1 & 3, it's not 100% clear to me what the idea is behind
>>>>> the handling of the hole on the kernel and userspace side. Maybe you
>>>>> can
>>>>> add some explanation in code comments or the commit logs?
>>>> Yeah, that is actually a bit of a mess because the hardware
>>>> documentation wasn't very clear on how this works.
>>>>
>>>> How about this as extra code comment on patch 1 to the assignment of
>>>> dev_info.virtual_address_max:
>>>>
>>>> /*
>>>>    * Old userspace isn't aware of the VA hole on Vega10. So in theory an
>>>> client could get invalid VA addresses assigned.
>>>>    * To fix this and keep backward compatibility we limit the VA space
>>>> reported in this field to the range below the hole.
>>>>    */
>>>>
>>>> The last patch is then to report the VA space above the hole, cause that
>>>> is actually what libdrm should use.
>>>>
>>>> The crux is when I put the VA space above the hole directly into the old
>>>> fields older versions of libdrm would break and we can't do that.
>>> That much was actually clear. :) Maybe some questions will expose what
>>> I'm missing:
>>>
>>>
>>> Patch 1:
>>>
>>>> @@ -880,14 +880,14 @@  static int amdgpu_cs_ib_vm_chunk(struct
>>>> amdgpu_device *adev,
>>>>                if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
>>>>                    continue;
>>>>    -            r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
>>>> -                           &aobj, &m);
>>>> +            va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
>>>> +            r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
>>> I don't understand how userspace can make use of the address space above
>>> the hole, since AMDGPU_VA_HOLE_MASK is 0x0000ffffffffffffULL and
>>> AMDGPU_VA_HOLE_END is 0xffff800000000000ULL, so masking with
>>> AMDGPU_VA_HOLE_MASK always results in an address below or inside the
>>> hole?
>> Yes, and exactly that is intended here.
>>
>> See for programming the MC and filling the page tables you handle this
>> as if there isn't a hole in the address space.
>>
>> Only when you start to use the address from userspace you will find that
>> you need to sign extend bit 47 into bits 48-63.
>>
>> Applying the AMDGPU_VA_HOLE_MASK to and address masks out the upper 16
>> bits and so removes the sign extension again.
>>
>>>
>>>> @@ -566,6 +566,17 @@  int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>>> void *data,
>>>>            return -EINVAL;
>>>>        }
>>>>    +    if (args->va_address >= AMDGPU_VA_HOLE_START &&
>>>> +        args->va_address < AMDGPU_VA_HOLE_END) {
>>>> +        dev_dbg(&dev->pdev->dev,
>>>> +            "va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
>>>> +            args->va_address, AMDGPU_VA_HOLE_START,
>>>> +            AMDGPU_VA_HOLE_END);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    args->va_address &= AMDGPU_VA_HOLE_MASK;
>>> Ignoring the above, I think the masking with AMDGPU_VA_HOLE_MASK would
>>> need to be done before checking for the hole, otherwise the masked
>>> address could be inside the hole?
>> The masking just removes the sign extension which created the hole in
>> the first place.
>>
>> The VM and MC is programmed as if there isn't a hole.
>>
>>> Patch 3:
>>>
>>>> @@ -577,10 +578,17 @@  static int amdgpu_info_ioctl(struct drm_device
>>>> *dev, void *data, struct drm_file
>>>>                dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>>>>            if (amdgpu_sriov_vf(adev))
>>>>                dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
>>>> +
>>>> +        vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>>>>            dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>>>>            dev_info.virtual_address_max =
>>>> -            min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
>>>> -                AMDGPU_VA_HOLE_START);
>>>> +            min(vm_size, AMDGPU_VA_HOLE_START);
>>>> +
>>>> +        vm_size -= AMDGPU_VA_RESERVED_SIZE;
>>>> +        if (vm_size > AMDGPU_VA_HOLE_START) {
>>>> +            dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
>>>> +            dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>>>> +        }
>>> This mostly makes sense, except for the
>>>
>>>          dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>>>
>>> line. Can you explain how simply or'ing together the address of the end
>>> of the hole and the VM size works correctly in all cases?
>>>
>>> As an extreme example, with vm_size == 1, this value would compute as
>>> 0xffff800000000001ULL, which seems wrong or at least weird.
>> We check above if vm_size is larger than AMDGPU_VA_HOLE_START, in other
>> words larger than 0x8000000000.
>>
>> Since vm_size is once more in the value range the MC expects it (without
>> the hole) that should give us the right result to sign extend bit 47
>> into bits 48-63, e.g. 0xffff800000000.
>>
>>
>> And yes that initially confused me as well. Especially that you need to
>> program the MC and fill the page tables as if there wouldn't be a hole
>> in the address space.
>>
>> Applying "& AMDGPU_VA_HOLE_MASK" and "| AMDGPU_VA_HOLE_END" just
>> converts between the representation with and without the hole in it.
> Thanks for the explanation, I get it now.
>
> For the benefit of future readers of this code, it would be nice to have
> a short version of this explanation as a comment, e.g. above the
> AMDGPU_VA_HOLE_* defines.

I've added a comment and pushed the resulting patch with a CC: stable 
tag added.

What about the libdrm side of the change? Anybody who could take a look?

Thanks,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx