release ex mode after hw_init if no KIQ

release ex mode after hw_init if no KIQ

[Pixel Ding]

Hi Oded,

There're 3 patches for releasing exclusive mode after hw_init if
KIQ is not enabled.

[PATCH 1/3] drm/amdgpu: wrap allocation for amdgpu_device
Allocation of amdgpu_device and base fields are wrapped put it ahead.

[PATCH 2/3] drm/amdgpu: release exclusive mode after hw_init if no

[PATCH 3/3] drm/amdkfd: pass kgd to kfd inside device_init
Still don't pass kgd into probe callback in case of that it's used
unsafely.

Please help review. Thanks very much!
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] drm/amdgpu: wrap allocation for amdgpu_device

[Pixel Ding]

From: pding <Pixel.Ding@amd.com>

Add amdgpu_device_alloc() which was part of previous
amdgpu_device_init(). Then it's flexible to handle init
sequence since kfd has dependency to amdgpu_device base
fields.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 54 +++++++++++++++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 18 +++-------
 3 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 2730a75..6f2dc2b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1655,11 +1655,11 @@ static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev)
 {
 	return container_of(bdev, struct amdgpu_device, mman.bdev);
 }
-
-int amdgpu_device_init(struct amdgpu_device *adev,
+int amdgpu_device_alloc(struct amdgpu_device **padev,
 		       struct drm_device *ddev,
 		       struct pci_dev *pdev,
 		       uint32_t flags);
+int amdgpu_device_init(struct amdgpu_device *adev);
 void amdgpu_device_fini(struct amdgpu_device *adev);
 int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 068b56a..809e656 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2092,25 +2092,33 @@ bool amdgpu_device_has_dc_support(struct amdgpu_device *adev)
 }
 
 /**
- * amdgpu_device_init - initialize the driver
+ * amdgpu_device_alloc - allocate/init amdgpu_device structure
  *
- * @adev: amdgpu_device pointer
+ * @padev: pointer to amdgpu_device pointer
  * @pdev: drm dev pointer
  * @pdev: pci dev pointer
  * @flags: driver flags
  *
- * Initializes the driver info and hw (all asics).
  * Returns 0 for success or an error on failure.
- * Called at driver startup.
  */
-int amdgpu_device_init(struct amdgpu_device *adev,
-		       struct drm_device *ddev,
-		       struct pci_dev *pdev,
-		       uint32_t flags)
+int amdgpu_device_alloc(struct amdgpu_device **padev,
+			struct drm_device *ddev,
+			struct pci_dev *pdev,
+			uint32_t flags)
 {
-	int r, i;
-	bool runtime = false;
-	u32 max_MBps;
+	struct amdgpu_device *adev;
+
+	adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
+	if (adev == NULL)
+		return -ENOMEM;
+
+	if ((amdgpu_runtime_pm != 0) &&
+	    amdgpu_has_atpx() &&
+	    (amdgpu_is_atpx_hybrid() ||
+	     amdgpu_has_atpx_dgpu_power_cntl()) &&
+	    ((flags & AMD_IS_APU) == 0) &&
+	    !pci_is_thunderbolt_attached(pdev))
+		flags |= AMD_IS_PX;
 
 	adev->shutdown = false;
 	adev->dev = &pdev->dev;
@@ -2183,6 +2191,28 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 
 	INIT_DELAYED_WORK(&adev->late_init_work, amdgpu_late_init_func_handler);
 
+	*padev = adev;
+
+	return 0;
+}
+/**
+ * amdgpu_device_init - initialize the driver
+ *
+ * @adev: amdgpu_device pointer
+ * @pdev: drm dev pointer
+ * @pdev: pci dev pointer
+ * @flags: driver flags
+ *
+ * Initializes the driver info and hw (all asics).
+ * Returns 0 for success or an error on failure.
+ * Called at driver startup.
+ */
+int amdgpu_device_init(struct amdgpu_device *adev)
+{
+	int r, i;
+	bool runtime = false;
+	u32 max_MBps;
+
 	/* Registers mapping */
 	/* TODO: block userspace mapping of io register */
 	if (adev->asic_type >= CHIP_BONAIRE) {
@@ -2226,7 +2256,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 
 	if (amdgpu_runtime_pm == 1)
 		runtime = true;
-	if (amdgpu_device_is_px(ddev))
+	if (amdgpu_device_is_px(adev->ddev))
 		runtime = true;
 	if (!pci_is_thunderbolt_attached(adev->pdev))
 		vga_switcheroo_register_client(adev->pdev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 3e9760d..acdb010 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -124,19 +124,11 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 #endif
 retry_init:
 
-	adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
-	if (adev == NULL) {
-		return -ENOMEM;
-	}
-	dev->dev_private = (void *)adev;
+	r = amdgpu_device_alloc(&adev, dev, dev->pdev, flags);
+	if (r)
+		return r;
 
-	if ((amdgpu_runtime_pm != 0) &&
-	    amdgpu_has_atpx() &&
-	    (amdgpu_is_atpx_hybrid() ||
-	     amdgpu_has_atpx_dgpu_power_cntl()) &&
-	    ((flags & AMD_IS_APU) == 0) &&
-	    !pci_is_thunderbolt_attached(dev->pdev))
-		flags |= AMD_IS_PX;
+	dev->dev_private = (void *)adev;
 
 	/* amdgpu_device_init should report only fatal error
 	 * like memory allocation failure or iomapping failure,
@@ -144,7 +136,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 	 * properly initialize the GPU MC controller and permit
 	 * VRAM allocation
 	 */
-	r = amdgpu_device_init(adev, dev, dev->pdev, flags);
+	r = amdgpu_device_init(adev);
 	if (r == -EAGAIN && ++retry <= 3) {
 		adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
 		adev->virt.ops = NULL;
-- 
2.9.5

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

[PATCH 2/3] drm/amdgpu: release exclusive mode after hw_init if no kfd

[Pixel Ding]

From: pding <Pixel.Ding@amd.com>

KFD device init requires exclusive mode. Driver can release
exclusive mode after hw_init if KFD is not enabled.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 809e656..dc1d1af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1716,6 +1716,9 @@ static int amdgpu_init(struct amdgpu_device *adev)
 		adev->ip_blocks[i].status.hw = true;
 	}
 
+	if (amdgpu_sriov_vf(adev) && !adev->kfd)
+		amdgpu_virt_release_full_gpu(adev, true);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index acdb010..589b41f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -130,6 +130,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 
 	dev->dev_private = (void *)adev;
 
+	amdgpu_amdkfd_device_probe(adev);
 	/* amdgpu_device_init should report only fatal error
 	 * like memory allocation failure or iomapping failure,
 	 * or memory manager initialization failure, it must
@@ -141,6 +142,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 		adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
 		adev->virt.ops = NULL;
 		amdgpu_device_fini(adev);
+		kfree(adev->kfd);
 		kfree(adev);
 		dev->dev_private = NULL;
 		/* Don't request EX mode too frequently which is attacking */
@@ -162,7 +164,6 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 				"Error during ACPI methods call\n");
 	}
 
-	amdgpu_amdkfd_device_probe(adev);
 	amdgpu_amdkfd_device_init(adev);
 
 	if (amdgpu_device_is_px(dev)) {
@@ -174,7 +175,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 		pm_runtime_put_autosuspend(dev->dev);
 	}
 
-	if (amdgpu_sriov_vf(adev))
+	if (amdgpu_sriov_vf(adev) && adev->kfd)
 		amdgpu_virt_release_full_gpu(adev, true);
 
 out:
-- 
2.9.5

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

[PATCH 3/3] drm/amdkfd: pass kgd to kfd inside device_init

[Pixel Ding]

From: pding <Pixel.Ding@amd.com>

KGD is possible not fully initialised in probe phase, so it's not
safe to pass it in if kfd code tries to refer KGD here.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c      | 6 +++---
 drivers/gpu/drm/amd/amdkfd/kfd_device.c         | 8 ++++----
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h           | 6 +++---
 drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 4 ++--
 drivers/gpu/drm/radeon/radeon_kfd.c             | 6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index c70cda0..f2de9b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -89,8 +89,7 @@ void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev)
 		return;
 	}
 
-	adev->kfd = kgd2kfd->probe((struct kgd_dev *)adev,
-				   adev->pdev, kfd2kgd);
+	adev->kfd = kgd2kfd->probe(adev->pdev, kfd2kgd);
 }
 
 void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
@@ -131,7 +130,8 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
 				&gpu_resources.doorbell_aperture_size,
 				&gpu_resources.doorbell_start_offset);
 
-		kgd2kfd->device_init(adev->kfd, &gpu_resources);
+		kgd2kfd->device_init((struct kgd_dev *)adev, adev->kfd,
+				     &gpu_resources);
 	}
 }
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 5df12b2..84a5067 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -109,8 +109,8 @@ static const struct kfd_device_info *lookup_device_info(unsigned short did)
 	return NULL;
 }
 
-struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
-	struct pci_dev *pdev, const struct kfd2kgd_calls *f2g)
+struct kfd_dev *kgd2kfd_probe(struct pci_dev *pdev,
+			      const struct kfd2kgd_calls *f2g)
 {
 	struct kfd_dev *kfd;
 
@@ -126,7 +126,6 @@ struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
 	if (!kfd)
 		return NULL;
 
-	kfd->kgd = kgd;
 	kfd->device_info = device_info;
 	kfd->pdev = pdev;
 	kfd->init_complete = false;
@@ -217,11 +216,12 @@ static int iommu_invalid_ppr_cb(struct pci_dev *pdev, int pasid,
 	return AMD_IOMMU_INV_PRI_RSP_INVALID;
 }
 
-bool kgd2kfd_device_init(struct kfd_dev *kfd,
+bool kgd2kfd_device_init(struct kgd_dev *kgd, struct kfd_dev *kfd,
 			 const struct kgd2kfd_shared_resources *gpu_resources)
 {
 	unsigned int size;
 
+	kfd->kgd = kgd;
 	kfd->shared_resources = *gpu_resources;
 
 	/* calculate max size of mqds needed for queues */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 4cb90f5..a64d772 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -200,9 +200,9 @@ struct kfd_dev {
 
 /* KGD2KFD callbacks */
 void kgd2kfd_exit(void);
-struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
-			struct pci_dev *pdev, const struct kfd2kgd_calls *f2g);
-bool kgd2kfd_device_init(struct kfd_dev *kfd,
+struct kfd_dev *kgd2kfd_probe(struct pci_dev *pdev,
+			      const struct kfd2kgd_calls *f2g);
+bool kgd2kfd_device_init(struct kgd_dev *kgd, struct kfd_dev *kfd,
 			const struct kgd2kfd_shared_resources *gpu_resources);
 void kgd2kfd_device_exit(struct kfd_dev *kfd);
 
diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
index f516fd1..af814f7 100644
--- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
@@ -248,9 +248,9 @@ struct kfd2kgd_calls {
  */
 struct kgd2kfd_calls {
 	void (*exit)(void);
-	struct kfd_dev* (*probe)(struct kgd_dev *kgd, struct pci_dev *pdev,
+	struct kfd_dev* (*probe)(struct pci_dev *pdev,
 		const struct kfd2kgd_calls *f2g);
-	bool (*device_init)(struct kfd_dev *kfd,
+	bool (*device_init)(struct kgd_dev *kgd, struct kfd_dev *kfd,
 			const struct kgd2kfd_shared_resources *gpu_resources);
 	void (*device_exit)(struct kfd_dev *kfd);
 	void (*interrupt)(struct kfd_dev *kfd, const void *ih_ring_entry);
diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c b/drivers/gpu/drm/radeon/radeon_kfd.c
index 385b4d7..ac85400 100644
--- a/drivers/gpu/drm/radeon/radeon_kfd.c
+++ b/drivers/gpu/drm/radeon/radeon_kfd.c
@@ -181,8 +181,7 @@ void radeon_kfd_fini(void)
 void radeon_kfd_device_probe(struct radeon_device *rdev)
 {
 	if (kgd2kfd)
-		rdev->kfd = kgd2kfd->probe((struct kgd_dev *)rdev,
-			rdev->pdev, &kfd2kgd);
+		rdev->kfd = kgd2kfd->probe(rdev->pdev, &kfd2kgd);
 }
 
 void radeon_kfd_device_init(struct radeon_device *rdev)
@@ -214,7 +213,8 @@ void radeon_kfd_device_init(struct radeon_device *rdev)
 				&gpu_resources.doorbell_aperture_size,
 				&gpu_resources.doorbell_start_offset);
 
-		kgd2kfd->device_init(rdev->kfd, &gpu_resources);
+		kgd2kfd->device_init((struct kgd_dev *)rdev,
+				     rdev->kfd, &gpu_resources);
 	}
 }
 
-- 
2.9.5

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

Re: [PATCH 2/3] drm/amdgpu: release exclusive mode after hw_init if no kfd

[Felix Kuehling]

Hi Pixel,

I'm curious, which part of the KFD initialization requires exclusive
access? KFD doesn't access MMIO directly, only through callbacks to
amdgpu. Which of those callbacks are only used during initialization,
and require exclusive access? Maybe that's something that can be fixed.

Regards,
  Felix


On 2017-11-01 11:16 PM, Pixel Ding wrote:
> From: pding <Pixel.Ding@amd.com>
>
> KFD device init requires exclusive mode. Driver can release
> exclusive mode after hw_init if KFD is not enabled.
>
> Signed-off-by: pding <Pixel.Ding@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 5 +++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 809e656..dc1d1af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1716,6 +1716,9 @@ static int amdgpu_init(struct amdgpu_device *adev)
>  		adev->ip_blocks[i].status.hw = true;
>  	}
>  
> +	if (amdgpu_sriov_vf(adev) && !adev->kfd)
> +		amdgpu_virt_release_full_gpu(adev, true);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index acdb010..589b41f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -130,6 +130,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  
>  	dev->dev_private = (void *)adev;
>  
> +	amdgpu_amdkfd_device_probe(adev);
>  	/* amdgpu_device_init should report only fatal error
>  	 * like memory allocation failure or iomapping failure,
>  	 * or memory manager initialization failure, it must
> @@ -141,6 +142,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  		adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
>  		adev->virt.ops = NULL;
>  		amdgpu_device_fini(adev);
> +		kfree(adev->kfd);
>  		kfree(adev);
>  		dev->dev_private = NULL;
>  		/* Don't request EX mode too frequently which is attacking */
> @@ -162,7 +164,6 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  				"Error during ACPI methods call\n");
>  	}
>  
> -	amdgpu_amdkfd_device_probe(adev);
>  	amdgpu_amdkfd_device_init(adev);
>  
>  	if (amdgpu_device_is_px(dev)) {
> @@ -174,7 +175,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  		pm_runtime_put_autosuspend(dev->dev);
>  	}
>  
> -	if (amdgpu_sriov_vf(adev))
> +	if (amdgpu_sriov_vf(adev) && adev->kfd)
>  		amdgpu_virt_release_full_gpu(adev, true);
>  
>  out:

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

Re: [PATCH 2/3] drm/amdgpu: release exclusive mode after hw_init if no kfd

[Ding, Pixel]

Hi Felix,


KFD will set HQD. These registers must be accessed in exclusive mode, otherwise driver use KIQ to access them which causes world switch failure.

— 
Sincerely Yours,
Pixel







On 02/11/2017, 9:49 PM, "Kuehling, Felix" <Felix.Kuehling@amd.com> wrote:

>Hi Pixel,
>
>I'm curious, which part of the KFD initialization requires exclusive
>access? KFD doesn't access MMIO directly, only through callbacks to
>amdgpu. Which of those callbacks are only used during initialization,
>and require exclusive access? Maybe that's something that can be fixed.
>
>Regards,
>  Felix
>
>
>On 2017-11-01 11:16 PM, Pixel Ding wrote:
>> From: pding <Pixel.Ding@amd.com>
>>
>> KFD device init requires exclusive mode. Driver can release
>> exclusive mode after hw_init if KFD is not enabled.
>>
>> Signed-off-by: pding <Pixel.Ding@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 5 +++--
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 809e656..dc1d1af 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1716,6 +1716,9 @@ static int amdgpu_init(struct amdgpu_device *adev)
>>  		adev->ip_blocks[i].status.hw = true;
>>  	}
>>  
>> +	if (amdgpu_sriov_vf(adev) && !adev->kfd)
>> +		amdgpu_virt_release_full_gpu(adev, true);
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index acdb010..589b41f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -130,6 +130,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>>  
>>  	dev->dev_private = (void *)adev;
>>  
>> +	amdgpu_amdkfd_device_probe(adev);
>>  	/* amdgpu_device_init should report only fatal error
>>  	 * like memory allocation failure or iomapping failure,
>>  	 * or memory manager initialization failure, it must
>> @@ -141,6 +142,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>>  		adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
>>  		adev->virt.ops = NULL;
>>  		amdgpu_device_fini(adev);
>> +		kfree(adev->kfd);
>>  		kfree(adev);
>>  		dev->dev_private = NULL;
>>  		/* Don't request EX mode too frequently which is attacking */
>> @@ -162,7 +164,6 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>>  				"Error during ACPI methods call\n");
>>  	}
>>  
>> -	amdgpu_amdkfd_device_probe(adev);
>>  	amdgpu_amdkfd_device_init(adev);
>>  
>>  	if (amdgpu_device_is_px(dev)) {
>> @@ -174,7 +175,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>>  		pm_runtime_put_autosuspend(dev->dev);
>>  	}
>>  
>> -	if (amdgpu_sriov_vf(adev))
>> +	if (amdgpu_sriov_vf(adev) && adev->kfd)
>>  		amdgpu_virt_release_full_gpu(adev, true);
>>  
>>  out:
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amdgpu: release exclusive mode after hw_init if no kfd

[Felix Kuehling]

On 2017-11-01 11:16 PM, Pixel Ding wrote:
> From: pding <Pixel.Ding@amd.com>
>
> KFD device init requires exclusive mode. Driver can release
> exclusive mode after hw_init if KFD is not enabled.
>
> Signed-off-by: pding <Pixel.Ding@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 5 +++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 809e656..dc1d1af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1716,6 +1716,9 @@ static int amdgpu_init(struct amdgpu_device *adev)
>  		adev->ip_blocks[i].status.hw = true;
>  	}
>  
> +	if (amdgpu_sriov_vf(adev) && !adev->kfd)
> +		amdgpu_virt_release_full_gpu(adev, true);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index acdb010..589b41f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -130,6 +130,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  
>  	dev->dev_private = (void *)adev;
>  
> +	amdgpu_amdkfd_device_probe(adev);
>  	/* amdgpu_device_init should report only fatal error
>  	 * like memory allocation failure or iomapping failure,
>  	 * or memory manager initialization failure, it must
> @@ -141,6 +142,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  		adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
>  		adev->virt.ops = NULL;
>  		amdgpu_device_fini(adev);
> +		kfree(adev->kfd);

This is assuming that adev->kfd was allocated by kmalloc, and that
nothing else needs to be released. It would be safer (fewer assumptions)
to call amdgpu_amdkfd_device_fini instead, which will call
kgd2kfd->device_exit. It should be safe even without having done the
device_init yet.

Regards,
  Felix

>  		kfree(adev);
>  		dev->dev_private = NULL;
>  		/* Don't request EX mode too frequently which is attacking */
> @@ -162,7 +164,6 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  				"Error during ACPI methods call\n");
>  	}
>  
> -	amdgpu_amdkfd_device_probe(adev);
>  	amdgpu_amdkfd_device_init(adev);
>  
>  	if (amdgpu_device_is_px(dev)) {
> @@ -174,7 +175,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  		pm_runtime_put_autosuspend(dev->dev);
>  	}
>  
> -	if (amdgpu_sriov_vf(adev))
> +	if (amdgpu_sriov_vf(adev) && adev->kfd)
>  		amdgpu_virt_release_full_gpu(adev, true);
>  
>  out:

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

Re: [PATCH 1/3] drm/amdgpu: wrap allocation for amdgpu_device

[Felix Kuehling]

On 2017-11-01 11:16 PM, Pixel Ding wrote:
> From: pding <Pixel.Ding@amd.com>
>
> Add amdgpu_device_alloc() which was part of previous
> amdgpu_device_init(). Then it's flexible to handle init
> sequence since kfd has dependency to amdgpu_device base
> fields.
>
> Signed-off-by: pding <Pixel.Ding@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 54 +++++++++++++++++++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 18 +++-------
>  3 files changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 2730a75..6f2dc2b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1655,11 +1655,11 @@ static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev)
>  {
>  	return container_of(bdev, struct amdgpu_device, mman.bdev);
>  }
> -
> -int amdgpu_device_init(struct amdgpu_device *adev,
> +int amdgpu_device_alloc(struct amdgpu_device **padev,
>  		       struct drm_device *ddev,
>  		       struct pci_dev *pdev,
>  		       uint32_t flags);
> +int amdgpu_device_init(struct amdgpu_device *adev);
>  void amdgpu_device_fini(struct amdgpu_device *adev);
>  int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 068b56a..809e656 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2092,25 +2092,33 @@ bool amdgpu_device_has_dc_support(struct amdgpu_device *adev)
>  }
>  
>  /**
> - * amdgpu_device_init - initialize the driver
> + * amdgpu_device_alloc - allocate/init amdgpu_device structure

I'm not sure this is a good name for this function. It does a lot more
than just allocate. It also initializes a bunch of fields. Maybe call it
something like amdgpu_device_early_init.

Also, the allocation doesn't really have to be part of it. That could
just stay where it is in amdgpu_driver_load_kms. As I understand it, the
key is that you split amdgpu_device_init into two pieces, so that you
initialized just enough to start probing KFD before you get to the
actual device initialization.

That said, an easier and cleaner solution may be to move
amdgpu_amdkfd_device_probe into amdgpu_early_init and
amdgpu_amdkfd_device_init into amdgpu_init.

Regards,
  Felix

>   *
> - * @adev: amdgpu_device pointer
> + * @padev: pointer to amdgpu_device pointer
>   * @pdev: drm dev pointer
>   * @pdev: pci dev pointer
>   * @flags: driver flags
>   *
> - * Initializes the driver info and hw (all asics).
>   * Returns 0 for success or an error on failure.
> - * Called at driver startup.
>   */
> -int amdgpu_device_init(struct amdgpu_device *adev,
> -		       struct drm_device *ddev,
> -		       struct pci_dev *pdev,
> -		       uint32_t flags)
> +int amdgpu_device_alloc(struct amdgpu_device **padev,
> +			struct drm_device *ddev,
> +			struct pci_dev *pdev,
> +			uint32_t flags)
>  {
> -	int r, i;
> -	bool runtime = false;
> -	u32 max_MBps;
> +	struct amdgpu_device *adev;
> +
> +	adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
> +	if (adev == NULL)
> +		return -ENOMEM;
> +
> +	if ((amdgpu_runtime_pm != 0) &&
> +	    amdgpu_has_atpx() &&
> +	    (amdgpu_is_atpx_hybrid() ||
> +	     amdgpu_has_atpx_dgpu_power_cntl()) &&
> +	    ((flags & AMD_IS_APU) == 0) &&
> +	    !pci_is_thunderbolt_attached(pdev))
> +		flags |= AMD_IS_PX;
>  
>  	adev->shutdown = false;
>  	adev->dev = &pdev->dev;
> @@ -2183,6 +2191,28 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  
>  	INIT_DELAYED_WORK(&adev->late_init_work, amdgpu_late_init_func_handler);
>  
> +	*padev = adev;
> +
> +	return 0;
> +}
> +/**
> + * amdgpu_device_init - initialize the driver
> + *
> + * @adev: amdgpu_device pointer
> + * @pdev: drm dev pointer
> + * @pdev: pci dev pointer
> + * @flags: driver flags
> + *
> + * Initializes the driver info and hw (all asics).
> + * Returns 0 for success or an error on failure.
> + * Called at driver startup.
> + */
> +int amdgpu_device_init(struct amdgpu_device *adev)
> +{
> +	int r, i;
> +	bool runtime = false;
> +	u32 max_MBps;
> +
>  	/* Registers mapping */
>  	/* TODO: block userspace mapping of io register */
>  	if (adev->asic_type >= CHIP_BONAIRE) {
> @@ -2226,7 +2256,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  
>  	if (amdgpu_runtime_pm == 1)
>  		runtime = true;
> -	if (amdgpu_device_is_px(ddev))
> +	if (amdgpu_device_is_px(adev->ddev))
>  		runtime = true;
>  	if (!pci_is_thunderbolt_attached(adev->pdev))
>  		vga_switcheroo_register_client(adev->pdev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 3e9760d..acdb010 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -124,19 +124,11 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  #endif
>  retry_init:
>  
> -	adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
> -	if (adev == NULL) {
> -		return -ENOMEM;
> -	}
> -	dev->dev_private = (void *)adev;
> +	r = amdgpu_device_alloc(&adev, dev, dev->pdev, flags);
> +	if (r)
> +		return r;
>  
> -	if ((amdgpu_runtime_pm != 0) &&
> -	    amdgpu_has_atpx() &&
> -	    (amdgpu_is_atpx_hybrid() ||
> -	     amdgpu_has_atpx_dgpu_power_cntl()) &&
> -	    ((flags & AMD_IS_APU) == 0) &&
> -	    !pci_is_thunderbolt_attached(dev->pdev))
> -		flags |= AMD_IS_PX;
> +	dev->dev_private = (void *)adev;
>  
>  	/* amdgpu_device_init should report only fatal error
>  	 * like memory allocation failure or iomapping failure,
> @@ -144,7 +136,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
>  	 * properly initialize the GPU MC controller and permit
>  	 * VRAM allocation
>  	 */
> -	r = amdgpu_device_init(adev, dev, dev->pdev, flags);
> +	r = amdgpu_device_init(adev);
>  	if (r == -EAGAIN && ++retry <= 3) {
>  		adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
>  		adev->virt.ops = NULL;

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

Re: [PATCH 3/3] drm/amdkfd: pass kgd to kfd inside device_init

[Felix Kuehling]

Christian just removed radeon_kfd.c. You'll need to rebase your patch.

I'd also change the order of the patches. Put this patch before your
patch 2, so that we never have a case that an uninitialized adev is
passed to KFD.

Other than that, this patch is Reviewed-by: Felix Kuehling
<Felix.Kuehling@amd.com>.

Regards,
  Felix


On 2017-11-01 11:16 PM, Pixel Ding wrote:
> From: pding <Pixel.Ding@amd.com>
>
> KGD is possible not fully initialised in probe phase, so it's not
> safe to pass it in if kfd code tries to refer KGD here.
>
> Signed-off-by: pding <Pixel.Ding@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c      | 6 +++---
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c         | 8 ++++----
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h           | 6 +++---
>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 4 ++--
>  drivers/gpu/drm/radeon/radeon_kfd.c             | 6 +++---
>  5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index c70cda0..f2de9b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -89,8 +89,7 @@ void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev)
>  		return;
>  	}
>  
> -	adev->kfd = kgd2kfd->probe((struct kgd_dev *)adev,
> -				   adev->pdev, kfd2kgd);
> +	adev->kfd = kgd2kfd->probe(adev->pdev, kfd2kgd);
>  }
>  
>  void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
> @@ -131,7 +130,8 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>  				&gpu_resources.doorbell_aperture_size,
>  				&gpu_resources.doorbell_start_offset);
>  
> -		kgd2kfd->device_init(adev->kfd, &gpu_resources);
> +		kgd2kfd->device_init((struct kgd_dev *)adev, adev->kfd,
> +				     &gpu_resources);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 5df12b2..84a5067 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -109,8 +109,8 @@ static const struct kfd_device_info *lookup_device_info(unsigned short did)
>  	return NULL;
>  }
>  
> -struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
> -	struct pci_dev *pdev, const struct kfd2kgd_calls *f2g)
> +struct kfd_dev *kgd2kfd_probe(struct pci_dev *pdev,
> +			      const struct kfd2kgd_calls *f2g)
>  {
>  	struct kfd_dev *kfd;
>  
> @@ -126,7 +126,6 @@ struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
>  	if (!kfd)
>  		return NULL;
>  
> -	kfd->kgd = kgd;
>  	kfd->device_info = device_info;
>  	kfd->pdev = pdev;
>  	kfd->init_complete = false;
> @@ -217,11 +216,12 @@ static int iommu_invalid_ppr_cb(struct pci_dev *pdev, int pasid,
>  	return AMD_IOMMU_INV_PRI_RSP_INVALID;
>  }
>  
> -bool kgd2kfd_device_init(struct kfd_dev *kfd,
> +bool kgd2kfd_device_init(struct kgd_dev *kgd, struct kfd_dev *kfd,
>  			 const struct kgd2kfd_shared_resources *gpu_resources)
>  {
>  	unsigned int size;
>  
> +	kfd->kgd = kgd;
>  	kfd->shared_resources = *gpu_resources;
>  
>  	/* calculate max size of mqds needed for queues */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 4cb90f5..a64d772 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -200,9 +200,9 @@ struct kfd_dev {
>  
>  /* KGD2KFD callbacks */
>  void kgd2kfd_exit(void);
> -struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
> -			struct pci_dev *pdev, const struct kfd2kgd_calls *f2g);
> -bool kgd2kfd_device_init(struct kfd_dev *kfd,
> +struct kfd_dev *kgd2kfd_probe(struct pci_dev *pdev,
> +			      const struct kfd2kgd_calls *f2g);
> +bool kgd2kfd_device_init(struct kgd_dev *kgd, struct kfd_dev *kfd,
>  			const struct kgd2kfd_shared_resources *gpu_resources);
>  void kgd2kfd_device_exit(struct kfd_dev *kfd);
>  
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index f516fd1..af814f7 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -248,9 +248,9 @@ struct kfd2kgd_calls {
>   */
>  struct kgd2kfd_calls {
>  	void (*exit)(void);
> -	struct kfd_dev* (*probe)(struct kgd_dev *kgd, struct pci_dev *pdev,
> +	struct kfd_dev* (*probe)(struct pci_dev *pdev,
>  		const struct kfd2kgd_calls *f2g);
> -	bool (*device_init)(struct kfd_dev *kfd,
> +	bool (*device_init)(struct kgd_dev *kgd, struct kfd_dev *kfd,
>  			const struct kgd2kfd_shared_resources *gpu_resources);
>  	void (*device_exit)(struct kfd_dev *kfd);
>  	void (*interrupt)(struct kfd_dev *kfd, const void *ih_ring_entry);
> diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c b/drivers/gpu/drm/radeon/radeon_kfd.c
> index 385b4d7..ac85400 100644
> --- a/drivers/gpu/drm/radeon/radeon_kfd.c
> +++ b/drivers/gpu/drm/radeon/radeon_kfd.c
> @@ -181,8 +181,7 @@ void radeon_kfd_fini(void)
>  void radeon_kfd_device_probe(struct radeon_device *rdev)
>  {
>  	if (kgd2kfd)
> -		rdev->kfd = kgd2kfd->probe((struct kgd_dev *)rdev,
> -			rdev->pdev, &kfd2kgd);
> +		rdev->kfd = kgd2kfd->probe(rdev->pdev, &kfd2kgd);
>  }
>  
>  void radeon_kfd_device_init(struct radeon_device *rdev)
> @@ -214,7 +213,8 @@ void radeon_kfd_device_init(struct radeon_device *rdev)
>  				&gpu_resources.doorbell_aperture_size,
>  				&gpu_resources.doorbell_start_offset);
>  
> -		kgd2kfd->device_init(rdev->kfd, &gpu_resources);
> +		kgd2kfd->device_init((struct kgd_dev *)rdev,
> +				     rdev->kfd, &gpu_resources);
>  	}
>  }
>  

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