diff mbox series

[RFC,1/5] drm/amdkfd: add SPM support for SVM

Message ID 20210527230809.3701-2-Felix.Kuehling@amd.com (mailing list archive)
State New, archived
Headers show
Series Support DEVICE_GENERIC memory in migrate_vma_* | expand

Commit Message

Felix Kuehling May 27, 2021, 11:08 p.m. UTC 
From: Alex Sierra <alex.sierra@amd.com>

When CPU is connected throug XGMI, it has coherent
access to VRAM resource. In this case that resource
is taken from a table in the device gmc aperture base.
This resource is used along with the device type, which could
be DEVICE_PRIVATE or DEVICE_GENERIC to create the device
page map region.

Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 12 +++++++++---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.h     |  1 -
 kernel/resource.c                        |  2 +-
 3 files changed, 10 insertions(+), 5 deletions(-)

Comments

Christoph Hellwig May 29, 2021, 6:38 a.m. UTC | #1 
On Thu, May 27, 2021 at 07:08:05PM -0400, Felix Kuehling wrote:
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 627e61b0c124..da137553b83e 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -783,7 +783,7 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start)
>  
>  	return res;
>  }
> -
> +EXPORT_SYMBOL(lookup_resource);

NAK on hiding random exports in a driver patch.  This needs to be a proper
patch with a proper rationale, a kerneldoc comment and use
EXPORT_SYMBOL_GPL.  And even then it smells rather fishy, but I'll wait
for the rationale.
Felix Kuehling May 29, 2021, 6:42 p.m. UTC | #2 
Am 2021-05-29 um 2:38 a.m. schrieb Christoph Hellwig:
> On Thu, May 27, 2021 at 07:08:05PM -0400, Felix Kuehling wrote:
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 627e61b0c124..da137553b83e 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -783,7 +783,7 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start)
>>  
>>  	return res;
>>  }
>> -
>> +EXPORT_SYMBOL(lookup_resource);
> NAK on hiding random exports in a driver patch.  This needs to be a proper
> patch with a proper rationale, a kerneldoc comment and use
> EXPORT_SYMBOL_GPL.  And even then it smells rather fishy, but I'll wait
> for the rationale.

Yeah, I missed that in internal review. I agree this needs to be a
separate patch.

The rationale is, that the GPU driver needs to be able to look up and
claim the resource corresponding to its VRAM in order to register it as
DEVICE_GENERIC memory with devmap. If there is a better way to do this,
I'm open to suggestions.

Regards,
  Felix
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index c8ca3252cbc2..f5939449a99f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -895,6 +895,7 @@  int svm_migrate_init(struct amdgpu_device *adev)
 	struct resource *res;
 	unsigned long size;
 	void *r;
+	bool xgmi_connected_to_cpu = adev->gmc.xgmi.connected_to_cpu;
 
 	/* Page migration works on Vega10 or newer */
 	if (kfddev->device_info->asic_family < CHIP_VEGA10)
@@ -907,17 +908,22 @@  int svm_migrate_init(struct amdgpu_device *adev)
 	 * should remove reserved size
 	 */
 	size = ALIGN(adev->gmc.real_vram_size, 2ULL << 20);
-	res = devm_request_free_mem_region(adev->dev, &iomem_resource, size);
+	if (xgmi_connected_to_cpu)
+		res = lookup_resource(&iomem_resource, adev->gmc.aper_base);
+	else
+		res = devm_request_free_mem_region(adev->dev, &iomem_resource, size);
+
 	if (IS_ERR(res))
 		return -ENOMEM;
 
-	pgmap->type = MEMORY_DEVICE_PRIVATE;
 	pgmap->nr_range = 1;
 	pgmap->range.start = res->start;
 	pgmap->range.end = res->end;
+	pgmap->type = xgmi_connected_to_cpu ?
+				MEMORY_DEVICE_GENERIC : MEMORY_DEVICE_PRIVATE;
 	pgmap->ops = &svm_migrate_pgmap_ops;
 	pgmap->owner = SVM_ADEV_PGMAP_OWNER(adev);
-	pgmap->flags = MIGRATE_VMA_SELECT_DEVICE_PRIVATE;
+	pgmap->flags = 0;
 	r = devm_memremap_pages(adev->dev, pgmap);
 	if (IS_ERR(r)) {
 		pr_err("failed to register HMM device memory\n");
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
index 21f693767a0d..3881a93192ed 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
@@ -38,7 +38,6 @@ 
 #define SVM_RANGE_VRAM_DOMAIN (1UL << 0)
 #define SVM_ADEV_PGMAP_OWNER(adev)\
 			((adev)->hive ? (void *)(adev)->hive : (void *)(adev))
-
 struct svm_range_bo {
 	struct amdgpu_bo		*bo;
 	struct kref			kref;
diff --git a/kernel/resource.c b/kernel/resource.c
index 627e61b0c124..da137553b83e 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -783,7 +783,7 @@  struct resource *lookup_resource(struct resource *root, resource_size_t start)
 
 	return res;
 }
-
+EXPORT_SYMBOL(lookup_resource);
 /*
  * Insert a resource into the resource tree. If successful, return NULL,
  * otherwise return the conflicting resource (compare to __request_resource())