[PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Monk Liu]

for SRIOV vf, Guest couldn't really access PCI registers so
gpu_reset() and asic_reset should be avoided.

for suspend it could run for SRIOV because cg/pg routine
already modified for SRIOV vf case, besides we should remove
the req/rel gpu access around it because the req/rel should be
used by invoker.

Change-Id: I678d3f2ce202458c1d1d404970fa47421766b666
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +--------
 drivers/gpu/drm/amd/amdgpu/vi.c            | 2 ++
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6106cd6..173df73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1566,9 +1566,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
 {
 	int i, r;
 
-	if (amdgpu_sriov_vf(adev))
-		amdgpu_virt_request_full_gpu(adev, false);
-
 	/* ungate SMC block first */
 	r = amdgpu_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_SMC,
 					 AMD_CG_STATE_UNGATE);
@@ -1597,9 +1594,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
 		}
 	}
 
-	if (amdgpu_sriov_vf(adev))
-		amdgpu_virt_release_full_gpu(adev, false);
-
 	return 0;
 }
 
@@ -2356,8 +2350,7 @@ int amdgpu_gpu_reset(struct amdgpu_device *adev)
 	int resched;
 	bool need_full_reset;
 
-	if (amdgpu_sriov_vf(adev))
-		return 0;
+	BUG_ON(amdgpu_sriov_vf(adev));
 
 	if (!amdgpu_check_soft_reset(adev)) {
 		DRM_INFO("No hardware hang detected. Did some blocks stall?\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 7810030..557994c 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -739,6 +739,8 @@ static int vi_asic_reset(struct amdgpu_device *adev)
 {
 	int r;
 
+	BUG_ON(amdgpu_sriov_vf(adev));
+
 	amdgpu_atombios_scratch_regs_engine_hung(adev, true);
 
 	r = vi_gpu_pci_config_reset(adev);
-- 
2.7.4

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

[PATCH 06/21] drm/amdgpu:cg & pg are not applied on VF

[Monk Liu]

Change-Id: I93a4e97f1d24a289ab20c2a62371f3e303322587
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c  | 9 +++++++++
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  | 6 ++++++
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 6 ++++++
 drivers/gpu/drm/amd/amdgpu/vi.c        | 6 ++++++
 4 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index cc223ae..837d57b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -5833,6 +5833,9 @@ static int gfx_v8_0_set_powergating_state(void *handle,
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	bool enable = (state == AMD_PG_STATE_GATE) ? true : false;
 
+	if (amdgpu_sriov_vf(adev))
+		return 0;
+
 	switch (adev->asic_type) {
 	case CHIP_CARRIZO:
 	case CHIP_STONEY:
@@ -5890,6 +5893,9 @@ static void gfx_v8_0_get_clockgating_state(void *handle, u32 *flags)
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	int data;
 
+	if (amdgpu_sriov_vf(adev))
+		*flags = 0;
+
 	/* AMD_CG_SUPPORT_GFX_MGCG */
 	data = RREG32(mmRLC_CGTT_MGCG_OVERRIDE);
 	if (!(data & RLC_CGTT_MGCG_OVERRIDE__CPF_MASK))
@@ -6403,6 +6409,9 @@ static int gfx_v8_0_set_clockgating_state(void *handle,
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	if (amdgpu_sriov_vf(adev))
+		return 0;
+
 	switch (adev->asic_type) {
 	case CHIP_FIJI:
 	case CHIP_CARRIZO:
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index a564e89..61a506e 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1427,6 +1427,9 @@ static int gmc_v8_0_set_clockgating_state(void *handle,
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	if (amdgpu_sriov_vf(adev))
+		return 0;
+
 	switch (adev->asic_type) {
 	case CHIP_FIJI:
 		fiji_update_mc_medium_grain_clock_gating(adev,
@@ -1451,6 +1454,9 @@ static void gmc_v8_0_get_clockgating_state(void *handle, u32 *flags)
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	int data;
 
+	if (amdgpu_sriov_vf(adev))
+		*flags = 0;
+
 	/* AMD_CG_SUPPORT_MC_MGCG */
 	data = RREG32(mmMC_HUB_MISC_HUB_CG);
 	if (data & MC_HUB_MISC_HUB_CG__ENABLE_MASK)
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index efcdf82..280d4ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -1512,6 +1512,9 @@ static int sdma_v3_0_set_clockgating_state(void *handle,
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	if (amdgpu_sriov_vf(adev))
+		return 0;
+
 	switch (adev->asic_type) {
 	case CHIP_FIJI:
 	case CHIP_CARRIZO:
@@ -1538,6 +1541,9 @@ static void sdma_v3_0_get_clockgating_state(void *handle, u32 *flags)
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	int data;
 
+	if (amdgpu_sriov_vf(adev))
+		*flags = 0;
+
 	/* AMD_CG_SUPPORT_SDMA_MGCG */
 	data = RREG32(mmSDMA0_CLK_CTRL + sdma_offsets[0]);
 	if (!(data & SDMA0_CLK_CTRL__SOFT_OVERRIDE0_MASK))
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 557994c..c880749 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -1390,6 +1390,9 @@ static int vi_common_set_clockgating_state(void *handle,
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	if (amdgpu_sriov_vf(adev))
+		return 0;
+
 	switch (adev->asic_type) {
 	case CHIP_FIJI:
 		vi_update_bif_medium_grain_light_sleep(adev,
@@ -1434,6 +1437,9 @@ static void vi_common_get_clockgating_state(void *handle, u32 *flags)
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	int data;
 
+	if (amdgpu_sriov_vf(adev))
+		*flags = 0;
+
 	/* AMD_CG_SUPPORT_BIF_LS */
 	data = RREG32_PCIE(ixPCIE_CNTL2);
 	if (data & PCIE_CNTL2__SLV_MEM_LS_EN_MASK)
-- 
2.7.4

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

[PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Monk Liu]

if this call is from resume, shouldn't enter pin logic at all

Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
index 964d2a9..5e907f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
@@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
 	uint64_t gpu_addr;
 	int r;
 
+	if (adev->gart.table_addr) {
+		/* it's a resume call, gart already pin */
+		return 0;
+	}
+
 	r = amdgpu_bo_reserve(adev->gart.robj, false);
 	if (unlikely(r != 0))
 		return r;
-- 
2.7.4

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

答复: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 1236 bytes --]

anyone to review those patches ?

they are for SRIOV case mainly, and they are prepare for later TDR feature (GPU-reset on SR-IOV)


________________________________
发件人: Monk Liu <Monk.Liu@amd.com>
发送时间: 2017年2月4日 18:34:59
收件人: amd-gfx@lists.freedesktop.org
抄送: Liu, Monk
主题: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

if this call is from resume, shouldn't enter pin logic at all

Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
index 964d2a9..5e907f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
@@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
         uint64_t gpu_addr;
         int r;

+       if (adev->gart.table_addr) {
+               /* it's a resume call, gart already pin */
+               return 0;
+       }
+
         r = amdgpu_bo_reserve(adev->gart.robj, false);
         if (unlikely(r != 0))
                 return r;
--
2.7.4


[-- Attachment #1.2: Type: text/html, Size: 2904 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

RE: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Yu, Xiangliang]

Have you tried the patch for unloading driver?

Ps:  the index of patch make people confused, I think there are 21 patches in the series.


Thanks!
Xiangliang Yu


> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Monk Liu
> Sent: Saturday, February 04, 2017 6:35 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk <Monk.Liu@amd.com>
> Subject: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from
> VF
> 
> for SRIOV vf, Guest couldn't really access PCI registers so
> gpu_reset() and asic_reset should be avoided.
> 
> for suspend it could run for SRIOV because cg/pg routine already modified
> for SRIOV vf case, besides we should remove the req/rel gpu access around
> it because the req/rel should be used by invoker.
> 
> Change-Id: I678d3f2ce202458c1d1d404970fa47421766b666
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +--------
>  drivers/gpu/drm/amd/amdgpu/vi.c            | 2 ++
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6106cd6..173df73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1566,9 +1566,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
> {
>  	int i, r;
> 
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_request_full_gpu(adev, false);
> -
>  	/* ungate SMC block first */
>  	r = amdgpu_set_clockgating_state(adev,
> AMD_IP_BLOCK_TYPE_SMC,
>  					 AMD_CG_STATE_UNGATE);
> @@ -1597,9 +1594,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>  		}
>  	}
> 
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_release_full_gpu(adev, false);
> -
>  	return 0;
>  }
> 
> @@ -2356,8 +2350,7 @@ int amdgpu_gpu_reset(struct amdgpu_device
> *adev)
>  	int resched;
>  	bool need_full_reset;
> 
> -	if (amdgpu_sriov_vf(adev))
> -		return 0;
> +	BUG_ON(amdgpu_sriov_vf(adev));
> 
>  	if (!amdgpu_check_soft_reset(adev)) {
>  		DRM_INFO("No hardware hang detected. Did some blocks
> stall?\n"); diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> b/drivers/gpu/drm/amd/amdgpu/vi.c index 7810030..557994c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -739,6 +739,8 @@ static int vi_asic_reset(struct amdgpu_device *adev)
> {
>  	int r;
> 
> +	BUG_ON(amdgpu_sriov_vf(adev));
> +
>  	amdgpu_atombios_scratch_regs_engine_hung(adev, true);
> 
>  	r = vi_gpu_pci_config_reset(adev);
> --
> 2.7.4
> 
> _______________________________________________
> 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

答复: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 3283 bytes --]

yeah, there are 21 patches totally, but no need to expose all in one time

________________________________
发件人: Yu, Xiangliang
发送时间: 2017年2月6日 10:38:23
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org
抄送: Liu, Monk
主题: RE: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

Have you tried the patch for unloading driver?

Ps:  the index of patch make people confused, I think there are 21 patches in the series.


Thanks!
Xiangliang Yu


> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Monk Liu
> Sent: Saturday, February 04, 2017 6:35 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk <Monk.Liu@amd.com>
> Subject: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from
> VF
>
> for SRIOV vf, Guest couldn't really access PCI registers so
> gpu_reset() and asic_reset should be avoided.
>
> for suspend it could run for SRIOV because cg/pg routine already modified
> for SRIOV vf case, besides we should remove the req/rel gpu access around
> it because the req/rel should be used by invoker.
>
> Change-Id: I678d3f2ce202458c1d1d404970fa47421766b666
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +--------
>  drivers/gpu/drm/amd/amdgpu/vi.c            | 2 ++
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6106cd6..173df73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1566,9 +1566,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
> {
>        int i, r;
>
> -     if (amdgpu_sriov_vf(adev))
> -             amdgpu_virt_request_full_gpu(adev, false);
> -
>        /* ungate SMC block first */
>        r = amdgpu_set_clockgating_state(adev,
> AMD_IP_BLOCK_TYPE_SMC,
>                                         AMD_CG_STATE_UNGATE);
> @@ -1597,9 +1594,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>                }
>        }
>
> -     if (amdgpu_sriov_vf(adev))
> -             amdgpu_virt_release_full_gpu(adev, false);
> -
>        return 0;
>  }
>
> @@ -2356,8 +2350,7 @@ int amdgpu_gpu_reset(struct amdgpu_device
> *adev)
>        int resched;
>        bool need_full_reset;
>
> -     if (amdgpu_sriov_vf(adev))
> -             return 0;
> +     BUG_ON(amdgpu_sriov_vf(adev));
>
>        if (!amdgpu_check_soft_reset(adev)) {
>                DRM_INFO("No hardware hang detected. Did some blocks
> stall?\n"); diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> b/drivers/gpu/drm/amd/amdgpu/vi.c index 7810030..557994c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -739,6 +739,8 @@ static int vi_asic_reset(struct amdgpu_device *adev)
> {
>        int r;
>
> +     BUG_ON(amdgpu_sriov_vf(adev));
> +
>        amdgpu_atombios_scratch_regs_engine_hung(adev, true);
>
>        r = vi_gpu_pci_config_reset(adev);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #1.2: Type: text/html, Size: 6329 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

答复: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 3647 bytes --]

thanks for the catch, without those full access protection reboot is failed, I'll remove it and adjust later patches for TDR


BR Monk

________________________________
发件人: Liu, Monk
发送时间: 2017年2月6日 10:39:08
收件人: Yu, Xiangliang; amd-gfx@lists.freedesktop.org
主题: 答复: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF


yeah, there are 21 patches totally, but no need to expose all in one time

________________________________
发件人: Yu, Xiangliang
发送时间: 2017年2月6日 10:38:23
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org
抄送: Liu, Monk
主题: RE: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

Have you tried the patch for unloading driver?

Ps:  the index of patch make people confused, I think there are 21 patches in the series.


Thanks!
Xiangliang Yu


> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Monk Liu
> Sent: Saturday, February 04, 2017 6:35 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk <Monk.Liu@amd.com>
> Subject: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from
> VF
>
> for SRIOV vf, Guest couldn't really access PCI registers so
> gpu_reset() and asic_reset should be avoided.
>
> for suspend it could run for SRIOV because cg/pg routine already modified
> for SRIOV vf case, besides we should remove the req/rel gpu access around
> it because the req/rel should be used by invoker.
>
> Change-Id: I678d3f2ce202458c1d1d404970fa47421766b666
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +--------
>  drivers/gpu/drm/amd/amdgpu/vi.c            | 2 ++
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6106cd6..173df73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1566,9 +1566,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
> {
>        int i, r;
>
> -     if (amdgpu_sriov_vf(adev))
> -             amdgpu_virt_request_full_gpu(adev, false);
> -
>        /* ungate SMC block first */
>        r = amdgpu_set_clockgating_state(adev,
> AMD_IP_BLOCK_TYPE_SMC,
>                                         AMD_CG_STATE_UNGATE);
> @@ -1597,9 +1594,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>                }
>        }
>
> -     if (amdgpu_sriov_vf(adev))
> -             amdgpu_virt_release_full_gpu(adev, false);
> -
>        return 0;
>  }
>
> @@ -2356,8 +2350,7 @@ int amdgpu_gpu_reset(struct amdgpu_device
> *adev)
>        int resched;
>        bool need_full_reset;
>
> -     if (amdgpu_sriov_vf(adev))
> -             return 0;
> +     BUG_ON(amdgpu_sriov_vf(adev));
>
>        if (!amdgpu_check_soft_reset(adev)) {
>                DRM_INFO("No hardware hang detected. Did some blocks
> stall?\n"); diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> b/drivers/gpu/drm/amd/amdgpu/vi.c index 7810030..557994c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -739,6 +739,8 @@ static int vi_asic_reset(struct amdgpu_device *adev)
> {
>        int r;
>
> +     BUG_ON(amdgpu_sriov_vf(adev));
> +
>        amdgpu_atombios_scratch_regs_engine_hung(adev, true);
>
>        r = vi_gpu_pci_config_reset(adev);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #1.2: Type: text/html, Size: 7205 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: 答复: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Michel Dänzer]

On 06/02/17 11:39 AM, Liu, Monk wrote:
> yeah, there are 21 patches totally, but no need to expose all in one time

Then please edit the numbers in the mail subjects accordingly in the
future. :)


-- 
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

答复: 答复: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 932 bytes --]

I'll re-send the patch serials later, couple patches once


BR Monk

________________________________
发件人: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 代表 Michel Dänzer <michel@daenzer.net>
发送时间: 2017年2月6日 11:07:08
收件人: Liu, Monk; Yu, Xiangliang
抄送: amd-gfx@lists.freedesktop.org
主题: Re: 答复: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

On 06/02/17 11:39 AM, Liu, Monk wrote:
> yeah, there are 21 patches totally, but no need to expose all in one time

Then please edit the numbers in the mail subjects accordingly in the
future. :)


--
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

[-- Attachment #1.2: Type: text/html, Size: 2328 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Christian König]

A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during 
suspend.

Otherwise the GART table can be corrupted and we run into a whole bunch 
of problems.

We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double 
check that, but just ignoring that something went horrible wrong is 
clearly the wrong approach.

Regards,
Christian.

Am 04.02.2017 um 11:34 schrieb Monk Liu:
> if this call is from resume, shouldn't enter pin logic at all
>
> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index 964d2a9..5e907f7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>   	uint64_t gpu_addr;
>   	int r;
>   
> +	if (adev->gart.table_addr) {
> +		/* it's a resume call, gart already pin */
> +		return 0;
> +	}
> +
>   	r = amdgpu_bo_reserve(adev->gart.robj, false);
>   	if (unlikely(r != 0))
>   		return r;


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

Re: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Christian König]

Am 04.02.2017 um 11:34 schrieb Monk Liu:
> for SRIOV vf, Guest couldn't really access PCI registers so
> gpu_reset() and asic_reset should be avoided.
>
> for suspend it could run for SRIOV because cg/pg routine
> already modified for SRIOV vf case, besides we should remove
> the req/rel gpu access around it because the req/rel should be
> used by invoker.
>
> Change-Id: I678d3f2ce202458c1d1d404970fa47421766b666
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>

Mhm, isn't a BUG_ON() a bit hard here? I mean that usually terminates 
the current process if not the brings down the whole system.

Maybe just adding a WARN() to the "if (amdgpu_sriov_vf(adev)) return 0;" 
is more appropriate.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +--------
>   drivers/gpu/drm/amd/amdgpu/vi.c            | 2 ++
>   2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6106cd6..173df73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1566,9 +1566,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>   {
>   	int i, r;
>   
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_request_full_gpu(adev, false);
> -
>   	/* ungate SMC block first */
>   	r = amdgpu_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_SMC,
>   					 AMD_CG_STATE_UNGATE);
> @@ -1597,9 +1594,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>   		}
>   	}
>   
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_release_full_gpu(adev, false);
> -
>   	return 0;
>   }
>   
> @@ -2356,8 +2350,7 @@ int amdgpu_gpu_reset(struct amdgpu_device *adev)
>   	int resched;
>   	bool need_full_reset;
>   
> -	if (amdgpu_sriov_vf(adev))
> -		return 0;
> +	BUG_ON(amdgpu_sriov_vf(adev));
>   
>   	if (!amdgpu_check_soft_reset(adev)) {
>   		DRM_INFO("No hardware hang detected. Did some blocks stall?\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
> index 7810030..557994c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -739,6 +739,8 @@ static int vi_asic_reset(struct amdgpu_device *adev)
>   {
>   	int r;
>   
> +	BUG_ON(amdgpu_sriov_vf(adev));
> +
>   	amdgpu_atombios_scratch_regs_engine_hung(adev, true);
>   
>   	r = vi_gpu_pci_config_reset(adev);


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

RE: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

[Liu, Monk]

Actually I have more patches for gpu_reset feature for SRIOV case, but I didn't send out them, so this patch can be skipped.

I'll cleanup all those patches more and send out for review later

BR Monk

-----Original Message-----
From: Christian König [mailto:deathsimple@vodafone.de] 
Sent: Monday, February 06, 2017 4:24 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/21] drm/amdgpu:BUG if gpu_reste and asic_reset from VF

Am 04.02.2017 um 11:34 schrieb Monk Liu:
> for SRIOV vf, Guest couldn't really access PCI registers so
> gpu_reset() and asic_reset should be avoided.
>
> for suspend it could run for SRIOV because cg/pg routine already 
> modified for SRIOV vf case, besides we should remove the req/rel gpu 
> access around it because the req/rel should be used by invoker.
>
> Change-Id: I678d3f2ce202458c1d1d404970fa47421766b666
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>

Mhm, isn't a BUG_ON() a bit hard here? I mean that usually terminates the current process if not the brings down the whole system.

Maybe just adding a WARN() to the "if (amdgpu_sriov_vf(adev)) return 0;" 
is more appropriate.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +--------
>   drivers/gpu/drm/amd/amdgpu/vi.c            | 2 ++
>   2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6106cd6..173df73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1566,9 +1566,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>   {
>   	int i, r;
>   
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_request_full_gpu(adev, false);
> -
>   	/* ungate SMC block first */
>   	r = amdgpu_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_SMC,
>   					 AMD_CG_STATE_UNGATE);
> @@ -1597,9 +1594,6 @@ int amdgpu_suspend(struct amdgpu_device *adev)
>   		}
>   	}
>   
> -	if (amdgpu_sriov_vf(adev))
> -		amdgpu_virt_release_full_gpu(adev, false);
> -
>   	return 0;
>   }
>   
> @@ -2356,8 +2350,7 @@ int amdgpu_gpu_reset(struct amdgpu_device *adev)
>   	int resched;
>   	bool need_full_reset;
>   
> -	if (amdgpu_sriov_vf(adev))
> -		return 0;
> +	BUG_ON(amdgpu_sriov_vf(adev));
>   
>   	if (!amdgpu_check_soft_reset(adev)) {
>   		DRM_INFO("No hardware hang detected. Did some blocks stall?\n"); 
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c 
> b/drivers/gpu/drm/amd/amdgpu/vi.c index 7810030..557994c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -739,6 +739,8 @@ static int vi_asic_reset(struct amdgpu_device *adev)
>   {
>   	int r;
>   
> +	BUG_ON(amdgpu_sriov_vf(adev));
> +
>   	amdgpu_atombios_scratch_regs_engine_hung(adev, true);
>   
>   	r = vi_gpu_pci_config_reset(adev);


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

RE: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Liu, Monk]

Emmmm looks like I missed the part of S3 function

But if this is from a GPU reset ,  we also shouldn't continue run this function otherwise GPU reset will fail (SRIOV reset test)

BR Monk 

-----Original Message-----
From: Christian König [mailto:deathsimple@vodafone.de] 
Sent: Monday, February 06, 2017 4:14 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during suspend.

Otherwise the GART table can be corrupted and we run into a whole bunch of problems.

We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double check that, but just ignoring that something went horrible wrong is clearly the wrong approach.

Regards,
Christian.

Am 04.02.2017 um 11:34 schrieb Monk Liu:
> if this call is from resume, shouldn't enter pin logic at all
>
> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index 964d2a9..5e907f7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>   	uint64_t gpu_addr;
>   	int r;
>   
> +	if (adev->gart.table_addr) {
> +		/* it's a resume call, gart already pin */
> +		return 0;
> +	}
> +
>   	r = amdgpu_bo_reserve(adev->gart.robj, false);
>   	if (unlikely(r != 0))
>   		return r;


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

Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Christian König]

Hui? We shouldn't need to call this function from a GPU reset, do we 
really do so?

But even if we call it from GPU reset we certainly should have called 
the matching unpin function before.

Otherwise we certainly won't be able to resume from the next suspend 
after a GPU reset.

Regards,
Christian.

Am 06.02.2017 um 15:25 schrieb Liu, Monk:
> Emmmm looks like I missed the part of S3 function
>
> But if this is from a GPU reset ,  we also shouldn't continue run this function otherwise GPU reset will fail (SRIOV reset test)
>
> BR Monk
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Monday, February 06, 2017 4:14 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>
> A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during suspend.
>
> Otherwise the GART table can be corrupted and we run into a whole bunch of problems.
>
> We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double check that, but just ignoring that something went horrible wrong is clearly the wrong approach.
>
> Regards,
> Christian.
>
> Am 04.02.2017 um 11:34 schrieb Monk Liu:
>> if this call is from resume, shouldn't enter pin logic at all
>>
>> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>>    1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> index 964d2a9..5e907f7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>>    	uint64_t gpu_addr;
>>    	int r;
>>    
>> +	if (adev->gart.table_addr) {
>> +		/* it's a resume call, gart already pin */
>> +		return 0;
>> +	}
>> +
>>    	r = amdgpu_bo_reserve(adev->gart.robj, false);
>>    	if (unlikely(r != 0))
>>    		return r;
>
> _______________________________________________
> 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 07/21] drm/amdgpu:fix gart table vram pin

[Liu, Monk]

I recall why I made this patch

When testing SRIOV gpu reset feature, I it will always waiting and not return if without this patch, with more look into it:

Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend(), so the gart table BO won't get unpin, which lead to driver infinite wait loop  if we pin it again in resume.
 
For bare-metal case, gpu_reset will call amdgpu_suspend so the gart bo will unpin.

BTW:
GPU_SRIOV_RESET is invoked after HYPERVISOR call VF_FLR on this vf device, so all IP blocks's suspend routine is not needed at all.

What about:
>> +	if (adev->gart.table_addr && amdgpu_sriov_vf(adev)) {
>> +		/* it's a resume call, gart already pin */
>> +		return 0;
>> +	}


BR Monk


-----Original Message-----
From: Christian König [mailto:deathsimple@vodafone.de] 
Sent: Monday, February 06, 2017 10:31 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

Hui? We shouldn't need to call this function from a GPU reset, do we really do so?

But even if we call it from GPU reset we certainly should have called the matching unpin function before.

Otherwise we certainly won't be able to resume from the next suspend after a GPU reset.

Regards,
Christian.

Am 06.02.2017 um 15:25 schrieb Liu, Monk:
> Emmmm looks like I missed the part of S3 function
>
> But if this is from a GPU reset ,  we also shouldn't continue run this 
> function otherwise GPU reset will fail (SRIOV reset test)
>
> BR Monk
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Monday, February 06, 2017 4:14 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>
> A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during suspend.
>
> Otherwise the GART table can be corrupted and we run into a whole bunch of problems.
>
> We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double check that, but just ignoring that something went horrible wrong is clearly the wrong approach.
>
> Regards,
> Christian.
>
> Am 04.02.2017 um 11:34 schrieb Monk Liu:
>> if this call is from resume, shouldn't enter pin logic at all
>>
>> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>>    1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> index 964d2a9..5e907f7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>>    	uint64_t gpu_addr;
>>    	int r;
>>    
>> +	if (adev->gart.table_addr) {
>> +		/* it's a resume call, gart already pin */
>> +		return 0;
>> +	}
>> +
>>    	r = amdgpu_bo_reserve(adev->gart.robj, false);
>>    	if (unlikely(r != 0))
>>    		return r;
>
> _______________________________________________
> 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 07/21] drm/amdgpu:fix gart table vram pin

[Christian König]

> Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend()
That's most likely not a good idea.

Suspend and resume should always be paired, otherwise you run into 
exactly those problems and the GART is most likely only the tip of the 
iceberg here.

For example you also mess up the reference counting for buffer 
containing the UVD and VCE firmware (ok that won't affect SRIOV for now).

Maybe you just want to call hw_init() instead of a resume here?

Regards,
Christian.

Am 06.02.2017 um 16:55 schrieb Liu, Monk:
> I recall why I made this patch
>
> When testing SRIOV gpu reset feature, I it will always waiting and not return if without this patch, with more look into it:
>
> Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend(), so the gart table BO won't get unpin, which lead to driver infinite wait loop  if we pin it again in resume.
>   
> For bare-metal case, gpu_reset will call amdgpu_suspend so the gart bo will unpin.
>
> BTW:
> GPU_SRIOV_RESET is invoked after HYPERVISOR call VF_FLR on this vf device, so all IP blocks's suspend routine is not needed at all.
>
> What about:
>>> +	if (adev->gart.table_addr && amdgpu_sriov_vf(adev)) {
>>> +		/* it's a resume call, gart already pin */
>>> +		return 0;
>>> +	}
>
> BR Monk
>
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Monday, February 06, 2017 10:31 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>
> Hui? We shouldn't need to call this function from a GPU reset, do we really do so?
>
> But even if we call it from GPU reset we certainly should have called the matching unpin function before.
>
> Otherwise we certainly won't be able to resume from the next suspend after a GPU reset.
>
> Regards,
> Christian.
>
> Am 06.02.2017 um 15:25 schrieb Liu, Monk:
>> Emmmm looks like I missed the part of S3 function
>>
>> But if this is from a GPU reset ,  we also shouldn't continue run this
>> function otherwise GPU reset will fail (SRIOV reset test)
>>
>> BR Monk
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple@vodafone.de]
>> Sent: Monday, February 06, 2017 4:14 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>>
>> A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during suspend.
>>
>> Otherwise the GART table can be corrupted and we run into a whole bunch of problems.
>>
>> We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double check that, but just ignoring that something went horrible wrong is clearly the wrong approach.
>>
>> Regards,
>> Christian.
>>
>> Am 04.02.2017 um 11:34 schrieb Monk Liu:
>>> if this call is from resume, shouldn't enter pin logic at all
>>>
>>> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>> ---
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>>>     1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> index 964d2a9..5e907f7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>>>     	uint64_t gpu_addr;
>>>     	int r;
>>>     
>>> +	if (adev->gart.table_addr) {
>>> +		/* it's a resume call, gart already pin */
>>> +		return 0;
>>> +	}
>>> +
>>>     	r = amdgpu_bo_reserve(adev->gart.robj, false);
>>>     	if (unlikely(r != 0))
>>>     		return r;
>> _______________________________________________
>> 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


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

答复: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 4763 bytes --]

no, we already make it work, and suspend is totally nonsense for SRIOV,

because when guest side need a GPU reset, hypervisor will do VF flr, so after vf flr amdgpu_suspen() is redundant at all.


those 20 patches serial is verified on KVM/XEN platform, it can recover back from guest hang.



________________________________
发件人: Christian König <deathsimple@vodafone.de>
发送时间: 2017年2月7日 19:12:04
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org
主题: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

> Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend()
That's most likely not a good idea.

Suspend and resume should always be paired, otherwise you run into
exactly those problems and the GART is most likely only the tip of the
iceberg here.

For example you also mess up the reference counting for buffer
containing the UVD and VCE firmware (ok that won't affect SRIOV for now).

Maybe you just want to call hw_init() instead of a resume here?

Regards,
Christian.

Am 06.02.2017 um 16:55 schrieb Liu, Monk:
> I recall why I made this patch
>
> When testing SRIOV gpu reset feature, I it will always waiting and not return if without this patch, with more look into it:
>
> Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend(), so the gart table BO won't get unpin, which lead to driver infinite wait loop  if we pin it again in resume.
>
> For bare-metal case, gpu_reset will call amdgpu_suspend so the gart bo will unpin.
>
> BTW:
> GPU_SRIOV_RESET is invoked after HYPERVISOR call VF_FLR on this vf device, so all IP blocks's suspend routine is not needed at all.
>
> What about:
>>> +   if (adev->gart.table_addr && amdgpu_sriov_vf(adev)) {
>>> +           /* it's a resume call, gart already pin */
>>> +           return 0;
>>> +   }
>
> BR Monk
>
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Monday, February 06, 2017 10:31 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>
> Hui? We shouldn't need to call this function from a GPU reset, do we really do so?
>
> But even if we call it from GPU reset we certainly should have called the matching unpin function before.
>
> Otherwise we certainly won't be able to resume from the next suspend after a GPU reset.
>
> Regards,
> Christian.
>
> Am 06.02.2017 um 15:25 schrieb Liu, Monk:
>> Emmmm looks like I missed the part of S3 function
>>
>> But if this is from a GPU reset ,  we also shouldn't continue run this
>> function otherwise GPU reset will fail (SRIOV reset test)
>>
>> BR Monk
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple@vodafone.de]
>> Sent: Monday, February 06, 2017 4:14 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>>
>> A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during suspend.
>>
>> Otherwise the GART table can be corrupted and we run into a whole bunch of problems.
>>
>> We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double check that, but just ignoring that something went horrible wrong is clearly the wrong approach.
>>
>> Regards,
>> Christian.
>>
>> Am 04.02.2017 um 11:34 schrieb Monk Liu:
>>> if this call is from resume, shouldn't enter pin logic at all
>>>
>>> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>> ---
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>>>     1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> index 964d2a9..5e907f7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>>>      uint64_t gpu_addr;
>>>      int r;
>>>
>>> +   if (adev->gart.table_addr) {
>>> +           /* it's a resume call, gart already pin */
>>> +           return 0;
>>> +   }
>>> +
>>>      r = amdgpu_bo_reserve(adev->gart.robj, false);
>>>      if (unlikely(r != 0))
>>>              return r;
>> _______________________________________________
>> 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



[-- Attachment #1.2: Type: text/html, Size: 7580 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

答复: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 4596 bytes --]

this patch is dropped, we can call gart_vram_pin even the bo is already pinned, the internal of pin function will check that, thanks

________________________________
发件人: Christian König <deathsimple@vodafone.de>
发送时间: 2017年2月7日 19:12:04
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org
主题: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin

> Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend()
That's most likely not a good idea.

Suspend and resume should always be paired, otherwise you run into
exactly those problems and the GART is most likely only the tip of the
iceberg here.

For example you also mess up the reference counting for buffer
containing the UVD and VCE firmware (ok that won't affect SRIOV for now).

Maybe you just want to call hw_init() instead of a resume here?

Regards,
Christian.

Am 06.02.2017 um 16:55 schrieb Liu, Monk:
> I recall why I made this patch
>
> When testing SRIOV gpu reset feature, I it will always waiting and not return if without this patch, with more look into it:
>
> Because gpu_srio_reset (will send patch for this routine later) doesn't call amdgpu_suspend(), so the gart table BO won't get unpin, which lead to driver infinite wait loop  if we pin it again in resume.
>
> For bare-metal case, gpu_reset will call amdgpu_suspend so the gart bo will unpin.
>
> BTW:
> GPU_SRIOV_RESET is invoked after HYPERVISOR call VF_FLR on this vf device, so all IP blocks's suspend routine is not needed at all.
>
> What about:
>>> +   if (adev->gart.table_addr && amdgpu_sriov_vf(adev)) {
>>> +           /* it's a resume call, gart already pin */
>>> +           return 0;
>>> +   }
>
> BR Monk
>
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Monday, February 06, 2017 10:31 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>
> Hui? We shouldn't need to call this function from a GPU reset, do we really do so?
>
> But even if we call it from GPU reset we certainly should have called the matching unpin function before.
>
> Otherwise we certainly won't be able to resume from the next suspend after a GPU reset.
>
> Regards,
> Christian.
>
> Am 06.02.2017 um 15:25 schrieb Liu, Monk:
>> Emmmm looks like I missed the part of S3 function
>>
>> But if this is from a GPU reset ,  we also shouldn't continue run this
>> function otherwise GPU reset will fail (SRIOV reset test)
>>
>> BR Monk
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple@vodafone.de]
>> Sent: Monday, February 06, 2017 4:14 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 07/21] drm/amdgpu:fix gart table vram pin
>>
>> A bug NAK on this! amdgpu_gart_table_vram_unpin() must be called during suspend.
>>
>> Otherwise the GART table can be corrupted and we run into a whole bunch of problems.
>>
>> We could add a "BUG_ON(adev->gart.table_addr != NULL);" here to double check that, but just ignoring that something went horrible wrong is clearly the wrong approach.
>>
>> Regards,
>> Christian.
>>
>> Am 04.02.2017 um 11:34 schrieb Monk Liu:
>>> if this call is from resume, shouldn't enter pin logic at all
>>>
>>> Change-Id: I40a5cdc2a716c4c20d2812fd74ece4ea284b6765
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>> ---
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 +++++
>>>     1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> index 964d2a9..5e907f7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
>>> @@ -151,6 +151,11 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev)
>>>      uint64_t gpu_addr;
>>>      int r;
>>>
>>> +   if (adev->gart.table_addr) {
>>> +           /* it's a resume call, gart already pin */
>>> +           return 0;
>>> +   }
>>> +
>>>      r = amdgpu_bo_reserve(adev->gart.robj, false);
>>>      if (unlikely(r != 0))
>>>              return r;
>> _______________________________________________
>> 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



[-- Attachment #1.2: Type: text/html, Size: 7362 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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