[PATCH 0/5] Change the hw ip initialize sequence

[PATCH 0/5] Change the hw ip initialize sequence

[Rex Zhu]

we are suggested to initialize gfx/sdma before power
feature enabled. and On Vi, the gfx/sdma fw will be loaded
by smu, Export load_firmware interface to 
gfx/sdma, so gfx/sdma can trigger fw loading if they were
initialized before smu.


Rex Zhu (5):
  drm/amdgpu: Don't reallocate ucode bo when suspend
  drm/amd/pp: Allocate ucode bo in request_smu_load_fw
  drm/amd/pp: Implement load_firmware interface
  drm/amdgpu: Add fw load in gfx_v8 and sdma_v3
  drm/amdgpu: Change VI gfx/sdma/smu init sequence

 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c          |  2 +-
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c              | 11 ++++++++++
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c             |  8 ++++++++
 drivers/gpu/drm/amd/amdgpu/vi.c                    | 24 +++++++++++-----------
 drivers/gpu/drm/amd/powerplay/amd_powerplay.c      | 18 +++++++++++++---
 drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c |  2 ++
 drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c |  2 ++
 7 files changed, 51 insertions(+), 16 deletions(-)

-- 
1.9.1

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

[PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Rex Zhu]

driver don't release the ucode memory when suspend. so don't
need to allocate bo when resume back.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
index 9878212..adfeb93 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
@@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
 		return 0;
 	}
 
-	if (!adev->in_gpu_reset) {
+	if (!adev->in_gpu_reset && !adev->in_suspend) {
 		err = amdgpu_bo_create_kernel(adev, adev->firmware.fw_size, PAGE_SIZE,
 					amdgpu_sriov_vf(adev) ? AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
 					&adev->firmware.fw_buf,
-- 
1.9.1

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

[PATCH 2/5] drm/amd/pp: Allocate ucode bo in request_smu_load_fw

[Rex Zhu]

ucode bo is needed by request_smu_load_fw,
the request_smu_load_fw maybe called by gfx/sdma
before smu hw init.
so move amdgpu_ucode_bo_init to request_smu_lowd_fw
from smu hw init.

Reviewed-by: Evan Quan <evan.quan@amd.com>
Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/amd_powerplay.c      | 3 ---
 drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 2 ++
 drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c | 2 ++
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
index e51d961..b2ebcb1 100644
--- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
@@ -124,9 +124,6 @@ static int pp_hw_init(void *handle)
 	struct amdgpu_device *adev = handle;
 	struct pp_hwmgr *hwmgr = adev->powerplay.pp_handle;
 
-	if (adev->firmware.load_type == AMDGPU_FW_LOAD_SMU)
-		amdgpu_ucode_init_bo(adev);
-
 	ret = hwmgr_hw_init(hwmgr);
 
 	if (ret)
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
index 794a165..99b4e4f 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
@@ -346,6 +346,8 @@ int smu7_request_smu_load_fw(struct pp_hwmgr *hwmgr)
 	if (!hwmgr->reload_fw)
 		return 0;
 
+	amdgpu_ucode_init_bo(hwmgr->adev);
+
 	if (smu_data->soft_regs_start)
 		cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
 					smu_data->soft_regs_start + smum_get_offsetof(hwmgr,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
index 7b3b66d..abbf2f2 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
@@ -664,6 +664,8 @@ static int smu8_request_smu_load_fw(struct pp_hwmgr *hwmgr)
 	if (!hwmgr->reload_fw)
 		return 0;
 
+	amdgpu_ucode_init_bo(hwmgr->adev);
+
 	smu8_smu_populate_firmware_entries(hwmgr);
 
 	smu8_smu_construct_toc(hwmgr);
-- 
1.9.1

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

[PATCH 3/5] drm/amd/pp: Implement load_firmware interface

[Rex Zhu]

with this interface, gfx/sdma can be initialized
before smu.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
index b2ebcb1..6bc8e9c 100644
--- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
@@ -270,8 +270,23 @@ static int pp_set_clockgating_state(void *handle,
 	.funcs = &pp_ip_funcs,
 };
 
+/* This interface only be supported On Vi,
+ * because only smu7/8 can help to load gfx/sdma fw,
+ * smu need to be enabled before load other ip's fw.
+ * so call start smu to load smu7 fw and other ip's fw
+ */
 static int pp_dpm_load_fw(void *handle)
 {
+	struct pp_hwmgr *hwmgr = handle;
+
+	if (!hwmgr || !hwmgr->smumgr_funcs || !hwmgr->smumgr_funcs->start_smu)
+		return -EINVAL;
+
+	if (hwmgr->smumgr_funcs->start_smu(hwmgr)) {
+		pr_err("fw load failed\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
1.9.1

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

[PATCH 4/5] drm/amdgpu: Add fw load in gfx_v8 and sdma_v3

[Rex Zhu]

gfx and sdma can be initialized before smu.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c  | 11 +++++++++++
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 6b1954e..77e05c1 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -4180,9 +4180,20 @@ static void gfx_v8_0_rlc_start(struct amdgpu_device *adev)
 
 static int gfx_v8_0_rlc_resume(struct amdgpu_device *adev)
 {
+	int r;
+
 	gfx_v8_0_rlc_stop(adev);
 	gfx_v8_0_rlc_reset(adev);
 	gfx_v8_0_init_pg(adev);
+
+	if (adev->powerplay.pp_funcs->load_firmware) {
+		r = adev->powerplay.pp_funcs->load_firmware(adev->powerplay.pp_handle);
+		if (r) {
+			pr_err("firmware loading failed\n");
+			return r;
+		}
+	}
+
 	gfx_v8_0_rlc_start(adev);
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index 6fb3eda..0bdde7f 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -788,6 +788,14 @@ static int sdma_v3_0_start(struct amdgpu_device *adev)
 {
 	int r;
 
+	if (adev->powerplay.pp_funcs->load_firmware) {
+		r = adev->powerplay.pp_funcs->load_firmware(adev->powerplay.pp_handle);
+		if (r) {
+			pr_err("firmware loading failed\n");
+			return r;
+		}
+	}
+
 	/* disable sdma engine before programing it */
 	sdma_v3_0_ctx_switch_enable(adev, false);
 	sdma_v3_0_enable(adev, false);
-- 
1.9.1

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

[PATCH 5/5] drm/amdgpu: Change VI gfx/sdma/smu init sequence

[Rex Zhu]

initialize gfx/sdma before dpm features enabled.

Acked-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vi.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 88b57a5..07880d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -1596,16 +1596,18 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 		amdgpu_device_ip_block_add(adev, &vi_common_ip_block);
 		amdgpu_device_ip_block_add(adev, &gmc_v7_4_ip_block);
 		amdgpu_device_ip_block_add(adev, &iceland_ih_ip_block);
+		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
+		amdgpu_device_ip_block_add(adev, &sdma_v2_4_ip_block);
 		amdgpu_device_ip_block_add(adev, &pp_smu_ip_block);
 		if (adev->enable_virtual_display)
 			amdgpu_device_ip_block_add(adev, &dce_virtual_ip_block);
-		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &sdma_v2_4_ip_block);
 		break;
 	case CHIP_FIJI:
 		amdgpu_device_ip_block_add(adev, &vi_common_ip_block);
 		amdgpu_device_ip_block_add(adev, &gmc_v8_5_ip_block);
 		amdgpu_device_ip_block_add(adev, &tonga_ih_ip_block);
+		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
+		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &pp_smu_ip_block);
 		if (adev->enable_virtual_display || amdgpu_sriov_vf(adev))
 			amdgpu_device_ip_block_add(adev, &dce_virtual_ip_block);
@@ -1615,8 +1617,6 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 #endif
 		else
 			amdgpu_device_ip_block_add(adev, &dce_v10_1_ip_block);
-		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		if (!amdgpu_sriov_vf(adev)) {
 			amdgpu_device_ip_block_add(adev, &uvd_v6_0_ip_block);
 			amdgpu_device_ip_block_add(adev, &vce_v3_0_ip_block);
@@ -1626,6 +1626,8 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 		amdgpu_device_ip_block_add(adev, &vi_common_ip_block);
 		amdgpu_device_ip_block_add(adev, &gmc_v8_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &tonga_ih_ip_block);
+		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
+		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &pp_smu_ip_block);
 		if (adev->enable_virtual_display || amdgpu_sriov_vf(adev))
 			amdgpu_device_ip_block_add(adev, &dce_virtual_ip_block);
@@ -1635,8 +1637,6 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 #endif
 		else
 			amdgpu_device_ip_block_add(adev, &dce_v10_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		if (!amdgpu_sriov_vf(adev)) {
 			amdgpu_device_ip_block_add(adev, &uvd_v5_0_ip_block);
 			amdgpu_device_ip_block_add(adev, &vce_v3_0_ip_block);
@@ -1649,6 +1649,8 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 		amdgpu_device_ip_block_add(adev, &vi_common_ip_block);
 		amdgpu_device_ip_block_add(adev, &gmc_v8_1_ip_block);
 		amdgpu_device_ip_block_add(adev, &tonga_ih_ip_block);
+		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
+		amdgpu_device_ip_block_add(adev, &sdma_v3_1_ip_block);
 		amdgpu_device_ip_block_add(adev, &pp_smu_ip_block);
 		if (adev->enable_virtual_display)
 			amdgpu_device_ip_block_add(adev, &dce_virtual_ip_block);
@@ -1658,8 +1660,6 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 #endif
 		else
 			amdgpu_device_ip_block_add(adev, &dce_v11_2_ip_block);
-		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &sdma_v3_1_ip_block);
 		amdgpu_device_ip_block_add(adev, &uvd_v6_3_ip_block);
 		amdgpu_device_ip_block_add(adev, &vce_v3_4_ip_block);
 		break;
@@ -1667,6 +1667,8 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 		amdgpu_device_ip_block_add(adev, &vi_common_ip_block);
 		amdgpu_device_ip_block_add(adev, &gmc_v8_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &cz_ih_ip_block);
+		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
+		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &pp_smu_ip_block);
 		if (adev->enable_virtual_display)
 			amdgpu_device_ip_block_add(adev, &dce_virtual_ip_block);
@@ -1676,8 +1678,6 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 #endif
 		else
 			amdgpu_device_ip_block_add(adev, &dce_v11_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &gfx_v8_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &uvd_v6_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &vce_v3_1_ip_block);
 #if defined(CONFIG_DRM_AMD_ACP)
@@ -1688,6 +1688,8 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 		amdgpu_device_ip_block_add(adev, &vi_common_ip_block);
 		amdgpu_device_ip_block_add(adev, &gmc_v8_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &cz_ih_ip_block);
+		amdgpu_device_ip_block_add(adev, &gfx_v8_1_ip_block);
+		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &pp_smu_ip_block);
 		if (adev->enable_virtual_display)
 			amdgpu_device_ip_block_add(adev, &dce_virtual_ip_block);
@@ -1697,8 +1699,6 @@ int vi_set_ip_blocks(struct amdgpu_device *adev)
 #endif
 		else
 			amdgpu_device_ip_block_add(adev, &dce_v11_0_ip_block);
-		amdgpu_device_ip_block_add(adev, &gfx_v8_1_ip_block);
-		amdgpu_device_ip_block_add(adev, &sdma_v3_0_ip_block);
 		amdgpu_device_ip_block_add(adev, &uvd_v6_2_ip_block);
 		amdgpu_device_ip_block_add(adev, &vce_v3_4_ip_block);
 #if defined(CONFIG_DRM_AMD_ACP)
-- 
1.9.1

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

Re: [PATCH 4/5] drm/amdgpu: Add fw load in gfx_v8 and sdma_v3

[Alex Deucher]

On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>
> gfx and sdma can be initialized before smu.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

I think sdma_v2_4.c needs a similar update for iceland.  With that fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c  | 11 +++++++++++
>  drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c |  8 ++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 6b1954e..77e05c1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -4180,9 +4180,20 @@ static void gfx_v8_0_rlc_start(struct amdgpu_device *adev)
>
>  static int gfx_v8_0_rlc_resume(struct amdgpu_device *adev)
>  {
> +       int r;
> +
>         gfx_v8_0_rlc_stop(adev);
>         gfx_v8_0_rlc_reset(adev);
>         gfx_v8_0_init_pg(adev);
> +
> +       if (adev->powerplay.pp_funcs->load_firmware) {
> +               r = adev->powerplay.pp_funcs->load_firmware(adev->powerplay.pp_handle);
> +               if (r) {
> +                       pr_err("firmware loading failed\n");
> +                       return r;
> +               }
> +       }
> +
>         gfx_v8_0_rlc_start(adev);
>
>         return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> index 6fb3eda..0bdde7f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> @@ -788,6 +788,14 @@ static int sdma_v3_0_start(struct amdgpu_device *adev)
>  {
>         int r;
>
> +       if (adev->powerplay.pp_funcs->load_firmware) {
> +               r = adev->powerplay.pp_funcs->load_firmware(adev->powerplay.pp_handle);
> +               if (r) {
> +                       pr_err("firmware loading failed\n");
> +                       return r;
> +               }
> +       }
> +
>         /* disable sdma engine before programing it */
>         sdma_v3_0_ctx_switch_enable(adev, false);
>         sdma_v3_0_enable(adev, false);
> --
> 1.9.1
>
> _______________________________________________
> 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/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Alex Deucher]

On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>
> driver don't release the ucode memory when suspend. so don't
> need to allocate bo when resume back.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index 9878212..adfeb93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>                 return 0;
>         }
>
> -       if (!adev->in_gpu_reset) {
> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
>                 err = amdgpu_bo_create_kernel(adev, adev->firmware.fw_size, PAGE_SIZE,
>                                         amdgpu_sriov_vf(adev) ? AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
>                                         &adev->firmware.fw_buf,

Not sure if we support S3 in SR-IOV, but I think this will break it
because we'll lose vram contents and not re-init it.

Alex

> --
> 1.9.1
>
> _______________________________________________
> 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 2/5] drm/amd/pp: Allocate ucode bo in request_smu_load_fw

[Alex Deucher]

On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>
> ucode bo is needed by request_smu_load_fw,
> the request_smu_load_fw maybe called by gfx/sdma
> before smu hw init.
> so move amdgpu_ucode_bo_init to request_smu_lowd_fw
> from smu hw init.
>
> Reviewed-by: Evan Quan <evan.quan@amd.com>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


> ---
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c      | 3 ---
>  drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 2 ++
>  drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c | 2 ++
>  3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index e51d961..b2ebcb1 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -124,9 +124,6 @@ static int pp_hw_init(void *handle)
>         struct amdgpu_device *adev = handle;
>         struct pp_hwmgr *hwmgr = adev->powerplay.pp_handle;
>
> -       if (adev->firmware.load_type == AMDGPU_FW_LOAD_SMU)
> -               amdgpu_ucode_init_bo(adev);
> -
>         ret = hwmgr_hw_init(hwmgr);
>
>         if (ret)
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
> index 794a165..99b4e4f 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
> @@ -346,6 +346,8 @@ int smu7_request_smu_load_fw(struct pp_hwmgr *hwmgr)
>         if (!hwmgr->reload_fw)
>                 return 0;
>
> +       amdgpu_ucode_init_bo(hwmgr->adev);
> +
>         if (smu_data->soft_regs_start)
>                 cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
>                                         smu_data->soft_regs_start + smum_get_offsetof(hwmgr,
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
> index 7b3b66d..abbf2f2 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
> @@ -664,6 +664,8 @@ static int smu8_request_smu_load_fw(struct pp_hwmgr *hwmgr)
>         if (!hwmgr->reload_fw)
>                 return 0;
>
> +       amdgpu_ucode_init_bo(hwmgr->adev);
> +
>         smu8_smu_populate_firmware_entries(hwmgr);
>
>         smu8_smu_construct_toc(hwmgr);
> --
> 1.9.1
>
> _______________________________________________
> 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/5] drm/amd/pp: Implement load_firmware interface

[Alex Deucher]

On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>
> with this interface, gfx/sdma can be initialized
> before smu.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


> ---
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index b2ebcb1..6bc8e9c 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -270,8 +270,23 @@ static int pp_set_clockgating_state(void *handle,
>         .funcs = &pp_ip_funcs,
>  };
>
> +/* This interface only be supported On Vi,
> + * because only smu7/8 can help to load gfx/sdma fw,
> + * smu need to be enabled before load other ip's fw.
> + * so call start smu to load smu7 fw and other ip's fw
> + */
>  static int pp_dpm_load_fw(void *handle)
>  {
> +       struct pp_hwmgr *hwmgr = handle;
> +
> +       if (!hwmgr || !hwmgr->smumgr_funcs || !hwmgr->smumgr_funcs->start_smu)
> +               return -EINVAL;
> +
> +       if (hwmgr->smumgr_funcs->start_smu(hwmgr)) {
> +               pr_err("fw load failed\n");
> +               return -EINVAL;
> +       }
> +
>         return 0;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> 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/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Zhu, Rex]

> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Thursday, October 4, 2018 11:35 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> suspend
> 
> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
> >
> > driver don't release the ucode memory when suspend. so don't need to
> > allocate bo when resume back.
> >
> > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > index 9878212..adfeb93 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct amdgpu_device
> *adev)
> >                 return 0;
> >         }
> >
> > -       if (!adev->in_gpu_reset) {
> > +       if (!adev->in_gpu_reset && !adev->in_suspend) {
> >                 err = amdgpu_bo_create_kernel(adev, adev->firmware.fw_size,
> PAGE_SIZE,
> >                                         amdgpu_sriov_vf(adev) ?
> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> >                                         &adev->firmware.fw_buf,
> 
> Not sure if we support S3 in SR-IOV, but I think this will break it because we'll
> lose vram contents and not re-init it.

Confirm with SR-IOV team, S3 was not supported in SR-IOV.

 But I still confused why this patch will break the suspend if in SRIOV case?

Rex

> Alex
> 
> > --
> > 1.9.1
> >
> > _______________________________________________
> > 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 4/5] drm/amdgpu: Add fw load in gfx_v8 and sdma_v3

[Zhu, Rex]

Thanks.
I will fix this error in a following patch.

Best Regards
Rex

> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Thursday, October 4, 2018 11:33 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 4/5] drm/amdgpu: Add fw load in gfx_v8 and sdma_v3
> 
> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
> >
> > gfx and sdma can be initialized before smu.
> >
> > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> 
> I think sdma_v2_4.c needs a similar update for iceland.  With that fixed:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> > ---
> >  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c  | 11 +++++++++++
> > drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c |  8 ++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > index 6b1954e..77e05c1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > @@ -4180,9 +4180,20 @@ static void gfx_v8_0_rlc_start(struct
> > amdgpu_device *adev)
> >
> >  static int gfx_v8_0_rlc_resume(struct amdgpu_device *adev)  {
> > +       int r;
> > +
> >         gfx_v8_0_rlc_stop(adev);
> >         gfx_v8_0_rlc_reset(adev);
> >         gfx_v8_0_init_pg(adev);
> > +
> > +       if (adev->powerplay.pp_funcs->load_firmware) {
> > +               r = adev->powerplay.pp_funcs->load_firmware(adev-
> >powerplay.pp_handle);
> > +               if (r) {
> > +                       pr_err("firmware loading failed\n");
> > +                       return r;
> > +               }
> > +       }
> > +
> >         gfx_v8_0_rlc_start(adev);
> >
> >         return 0;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> > b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> > index 6fb3eda..0bdde7f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> > @@ -788,6 +788,14 @@ static int sdma_v3_0_start(struct amdgpu_device
> > *adev)  {
> >         int r;
> >
> > +       if (adev->powerplay.pp_funcs->load_firmware) {
> > +               r = adev->powerplay.pp_funcs->load_firmware(adev-
> >powerplay.pp_handle);
> > +               if (r) {
> > +                       pr_err("firmware loading failed\n");
> > +                       return r;
> > +               }
> > +       }
> > +
> >         /* disable sdma engine before programing it */
> >         sdma_v3_0_ctx_switch_enable(adev, false);
> >         sdma_v3_0_enable(adev, false);
> > --
> > 1.9.1
> >
> > _______________________________________________
> > 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/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhu,
> Rex
> Sent: Monday, October 8, 2018 11:57 AM
> To: Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> suspend
> 
> 
> 
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com>
> > Sent: Thursday, October 4, 2018 11:35 AM
> > To: Zhu, Rex <Rex.Zhu@amd.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> > Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> > suspend
> >
> > On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
> > >
> > > driver don't release the ucode memory when suspend. so don't need to
> > > allocate bo when resume back.
> > >
> > > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > index 9878212..adfeb93 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct amdgpu_device
> > *adev)
> > >                 return 0;
> > >         }
> > >
> > > -       if (!adev->in_gpu_reset) {
> > > +       if (!adev->in_gpu_reset && !adev->in_suspend) {
> > >                 err = amdgpu_bo_create_kernel(adev,
> > > adev->firmware.fw_size,
> > PAGE_SIZE,
> > >                                         amdgpu_sriov_vf(adev) ?
> > AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> > >                                         &adev->firmware.fw_buf,
> >
> > Not sure if we support S3 in SR-IOV, but I think this will break it
> > because we'll lose vram contents and not re-init it.
> 
> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
> 
>  But I still confused why this patch will break the suspend if in SRIOV case?

Pinned buffers don't get evicted so if we lose VRAM due to a gpu reset or S3, the data is lost.  GTT is retained since the OS manages that.

Alex

> 
> Rex
> 
> > Alex
> >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > 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

RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Zhu, Rex]

> -----Original Message-----
> From: Deucher, Alexander
> Sent: Tuesday, October 9, 2018 12:21 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> suspend
> 
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Zhu, Rex
> > Sent: Monday, October 8, 2018 11:57 AM
> > To: Alex Deucher <alexdeucher@gmail.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> > Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> > suspend
> >
> >
> >
> > > -----Original Message-----
> > > From: Alex Deucher <alexdeucher@gmail.com>
> > > Sent: Thursday, October 4, 2018 11:35 AM
> > > To: Zhu, Rex <Rex.Zhu@amd.com>
> > > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> > > Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> > > suspend
> > >
> > > On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
> > > >
> > > > driver don't release the ucode memory when suspend. so don't need
> > > > to allocate bo when resume back.
> > > >
> > > > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> > > > ---
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > > index 9878212..adfeb93 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> > > > @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
> amdgpu_device
> > > *adev)
> > > >                 return 0;
> > > >         }
> > > >
> > > > -       if (!adev->in_gpu_reset) {
> > > > +       if (!adev->in_gpu_reset && !adev->in_suspend) {
> > > >                 err = amdgpu_bo_create_kernel(adev,
> > > > adev->firmware.fw_size,
> > > PAGE_SIZE,
> > > >                                         amdgpu_sriov_vf(adev) ?
> > > AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> > > >                                         &adev->firmware.fw_buf,
> > >
> > > Not sure if we support S3 in SR-IOV, but I think this will break it
> > > because we'll lose vram contents and not re-init it.
> >
> > Confirm with SR-IOV team, S3 was not supported in SR-IOV.
> >
> >  But I still confused why this patch will break the suspend if in SRIOV case?
> 
> Pinned buffers don't get evicted so if we lose VRAM due to a gpu reset or S3,
> the data is lost.  GTT is retained since the OS manages that.

The gart table was unpinned when suspend.so don't need to create the bo again. we still copy the ucode to the bo.
And in baremetal,  this function can return directly for S3. 

Rex


> Alex
> 
> >
> > Rex
> >
> > > Alex
> > >
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > 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

Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Christian König]

Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Deucher, Alexander
>> Sent: Tuesday, October 9, 2018 12:21 AM
>> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher <alexdeucher@gmail.com>
>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>> suspend
>>
>>> -----Original Message-----
>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>> Zhu, Rex
>>> Sent: Monday, October 8, 2018 11:57 AM
>>> To: Alex Deucher <alexdeucher@gmail.com>
>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>> suspend
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alex Deucher <alexdeucher@gmail.com>
>>>> Sent: Thursday, October 4, 2018 11:35 AM
>>>> To: Zhu, Rex <Rex.Zhu@amd.com>
>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>> suspend
>>>>
>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>>>>> driver don't release the ucode memory when suspend. so don't need
>>>>> to allocate bo when resume back.
>>>>>
>>>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> index 9878212..adfeb93 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
>> amdgpu_device
>>>> *adev)
>>>>>                  return 0;
>>>>>          }
>>>>>
>>>>> -       if (!adev->in_gpu_reset) {
>>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
>>>>>                  err = amdgpu_bo_create_kernel(adev,
>>>>> adev->firmware.fw_size,
>>>> PAGE_SIZE,
>>>>>                                          amdgpu_sriov_vf(adev) ?
>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
>>>>>                                          &adev->firmware.fw_buf,
>>>> Not sure if we support S3 in SR-IOV, but I think this will break it
>>>> because we'll lose vram contents and not re-init it.
>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
>>>
>>>   But I still confused why this patch will break the suspend if in SRIOV case?
>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu reset or S3,
>> the data is lost.  GTT is retained since the OS manages that.
> The gart table was unpinned when suspend.so don't need to create the bo again. we still copy the ucode to the bo.
> And in baremetal,  this function can return directly for S3.

That's irrelevant.

The whole code is buggy since amdgpu_ucode_fini_bo() will drop the BO 
independent if we are in reset or in suspend.

The correct handling here is to remove the if all together and make sure 
amdgpu_bo_create_kernel() is ALWAYS called.

Cause then it is always re-created if it isn't there already.

Alternatively we could fix up the callers of amdgpu_ucode_init_bo() and 
amdgpu_ucode_fini_bo() to be correctly balanced.

Christian.

>
> Rex
>
>
>> Alex
>>
>>> Rex
>>>
>>>> Alex
>>>>
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> 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

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

RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Zhu, Rex]

> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, October 9, 2018 1:32 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> suspend
> 
> Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
> >
> >> -----Original Message-----
> >> From: Deucher, Alexander
> >> Sent: Tuesday, October 9, 2018 12:21 AM
> >> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher
> <alexdeucher@gmail.com>
> >> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >> suspend
> >>
> >>> -----Original Message-----
> >>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> >>> Zhu, Rex
> >>> Sent: Monday, October 8, 2018 11:57 AM
> >>> To: Alex Deucher <alexdeucher@gmail.com>
> >>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >>> suspend
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Alex Deucher <alexdeucher@gmail.com>
> >>>> Sent: Thursday, October 4, 2018 11:35 AM
> >>>> To: Zhu, Rex <Rex.Zhu@amd.com>
> >>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >>>> suspend
> >>>>
> >>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
> >>>>> driver don't release the ucode memory when suspend. so don't need
> >>>>> to allocate bo when resume back.
> >>>>>
> >>>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> >>>>> ---
> >>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
> >>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>> index 9878212..adfeb93 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
> >> amdgpu_device
> >>>> *adev)
> >>>>>                  return 0;
> >>>>>          }
> >>>>>
> >>>>> -       if (!adev->in_gpu_reset) {
> >>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
> >>>>>                  err = amdgpu_bo_create_kernel(adev,
> >>>>> adev->firmware.fw_size,
> >>>> PAGE_SIZE,
> >>>>>                                          amdgpu_sriov_vf(adev) ?
> >>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> >>>>>                                          &adev->firmware.fw_buf,
> >>>> Not sure if we support S3 in SR-IOV, but I think this will break it
> >>>> because we'll lose vram contents and not re-init it.
> >>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
> >>>
> >>>   But I still confused why this patch will break the suspend if in SRIOV
> case?
> >> Pinned buffers don't get evicted so if we lose VRAM due to a gpu
> >> reset or S3, the data is lost.  GTT is retained since the OS manages that.
> > The gart table was unpinned when suspend.so don't need to create the bo
> again. we still copy the ucode to the bo.
> > And in baremetal,  this function can return directly for S3.
> 
> That's irrelevant.
> 
> The whole code is buggy since amdgpu_ucode_fini_bo() will drop the BO
> independent if we are in reset or in suspend.

We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset.

Rex

> The correct handling here is to remove the if all together and make sure
> amdgpu_bo_create_kernel() is ALWAYS called.
> 
> Cause then it is always re-created if it isn't there already.
> 
> Alternatively we could fix up the callers of amdgpu_ucode_init_bo() and
> amdgpu_ucode_fini_bo() to be correctly balanced.
> 
> Christian.
> 
> >
> > Rex
> >
> >
> >> Alex
> >>
> >>> Rex
> >>>
> >>>> Alex
> >>>>
> >>>>> --
> >>>>> 1.9.1
> >>>>>
> >>>>> _______________________________________________
> >>>>> 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

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

Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Koenig, Christian]

Am 08.10.2018 um 19:58 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Tuesday, October 9, 2018 1:32 AM
>> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>> suspend
>>
>> Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
>>>> -----Original Message-----
>>>> From: Deucher, Alexander
>>>> Sent: Tuesday, October 9, 2018 12:21 AM
>>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher
>> <alexdeucher@gmail.com>
>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>> suspend
>>>>
>>>>> -----Original Message-----
>>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>>> Zhu, Rex
>>>>> Sent: Monday, October 8, 2018 11:57 AM
>>>>> To: Alex Deucher <alexdeucher@gmail.com>
>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>>> suspend
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Alex Deucher <alexdeucher@gmail.com>
>>>>>> Sent: Thursday, October 4, 2018 11:35 AM
>>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>
>>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>>>> suspend
>>>>>>
>>>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>>>>>>> driver don't release the ucode memory when suspend. so don't need
>>>>>>> to allocate bo when resume back.
>>>>>>>
>>>>>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>> index 9878212..adfeb93 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
>>>> amdgpu_device
>>>>>> *adev)
>>>>>>>                   return 0;
>>>>>>>           }
>>>>>>>
>>>>>>> -       if (!adev->in_gpu_reset) {
>>>>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
>>>>>>>                   err = amdgpu_bo_create_kernel(adev,
>>>>>>> adev->firmware.fw_size,
>>>>>> PAGE_SIZE,
>>>>>>>                                           amdgpu_sriov_vf(adev) ?
>>>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
>>>>>>>                                           &adev->firmware.fw_buf,
>>>>>> Not sure if we support S3 in SR-IOV, but I think this will break it
>>>>>> because we'll lose vram contents and not re-init it.
>>>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
>>>>>
>>>>>    But I still confused why this patch will break the suspend if in SRIOV
>> case?
>>>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu
>>>> reset or S3, the data is lost.  GTT is retained since the OS manages that.
>>> The gart table was unpinned when suspend.so don't need to create the bo
>> again. we still copy the ucode to the bo.
>>> And in baremetal,  this function can return directly for S3.
>> That's irrelevant.
>>
>> The whole code is buggy since amdgpu_ucode_fini_bo() will drop the BO
>> independent if we are in reset or in suspend.
> We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset.

Yeah and exactly that's the bug which should be fixed instead.

Christian.

>
> Rex
>
>> The correct handling here is to remove the if all together and make sure
>> amdgpu_bo_create_kernel() is ALWAYS called.
>>
>> Cause then it is always re-created if it isn't there already.
>>
>> Alternatively we could fix up the callers of amdgpu_ucode_init_bo() and
>> amdgpu_ucode_fini_bo() to be correctly balanced.
>>
>> Christian.
>>
>>> Rex
>>>
>>>
>>>> Alex
>>>>
>>>>> Rex
>>>>>
>>>>>> Alex
>>>>>>
>>>>>>> --
>>>>>>> 1.9.1
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> 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

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

RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Zhu, Rex]

> -----Original Message-----
> From: Koenig, Christian
> Sent: Tuesday, October 9, 2018 2:03 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> suspend
> 
> Am 08.10.2018 um 19:58 schrieb Zhu, Rex:
> >
> >> -----Original Message-----
> >> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >> Sent: Tuesday, October 9, 2018 1:32 AM
> >> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> >> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> >> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >> suspend
> >>
> >> Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
> >>>> -----Original Message-----
> >>>> From: Deucher, Alexander
> >>>> Sent: Tuesday, October 9, 2018 12:21 AM
> >>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher
> >> <alexdeucher@gmail.com>
> >>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >>>> suspend
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf
> Of
> >>>>> Zhu, Rex
> >>>>> Sent: Monday, October 8, 2018 11:57 AM
> >>>>> To: Alex Deucher <alexdeucher@gmail.com>
> >>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>> when suspend
> >>>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Alex Deucher <alexdeucher@gmail.com>
> >>>>>> Sent: Thursday, October 4, 2018 11:35 AM
> >>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>
> >>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>>> when suspend
> >>>>>>
> >>>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
> >>>>>>> driver don't release the ucode memory when suspend. so don't
> >>>>>>> need to allocate bo when resume back.
> >>>>>>>
> >>>>>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> >>>>>>> ---
> >>>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
> >>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>> index 9878212..adfeb93 100644
> >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
> >>>> amdgpu_device
> >>>>>> *adev)
> >>>>>>>                   return 0;
> >>>>>>>           }
> >>>>>>>
> >>>>>>> -       if (!adev->in_gpu_reset) {
> >>>>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
> >>>>>>>                   err = amdgpu_bo_create_kernel(adev,
> >>>>>>> adev->firmware.fw_size,
> >>>>>> PAGE_SIZE,
> >>>>>>>                                           amdgpu_sriov_vf(adev) ?
> >>>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> >>>>>>>
> >>>>>>> &adev->firmware.fw_buf,
> >>>>>> Not sure if we support S3 in SR-IOV, but I think this will break
> >>>>>> it because we'll lose vram contents and not re-init it.
> >>>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
> >>>>>
> >>>>>    But I still confused why this patch will break the suspend if
> >>>>> in SRIOV
> >> case?
> >>>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu
> >>>> reset or S3, the data is lost.  GTT is retained since the OS manages that.
> >>> The gart table was unpinned when suspend.so don't need to create the
> >>> bo
> >> again. we still copy the ucode to the bo.
> >>> And in baremetal,  this function can return directly for S3.
> >> That's irrelevant.
> >>
> >> The whole code is buggy since amdgpu_ucode_fini_bo() will drop the BO
> >> independent if we are in reset or in suspend.
> > We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset.
> 
> Yeah and exactly that's the bug which should be fixed instead.

Sorry, I don't understand.
Why we need to release the bo when suspend?
In amdgpu, we create a bunch of bo(vram_scratch.robj, e.g), we only free them when driver unload.

Rex

> Christian.
> 
> >
> > Rex
> >
> >> The correct handling here is to remove the if all together and make
> >> sure
> >> amdgpu_bo_create_kernel() is ALWAYS called.
> >>
> >> Cause then it is always re-created if it isn't there already.
> >>
> >> Alternatively we could fix up the callers of amdgpu_ucode_init_bo()
> >> and
> >> amdgpu_ucode_fini_bo() to be correctly balanced.
> >>
> >> Christian.
> >>
> >>> Rex
> >>>
> >>>
> >>>> Alex
> >>>>
> >>>>> Rex
> >>>>>
> >>>>>> Alex
> >>>>>>
> >>>>>>> --
> >>>>>>> 1.9.1
> >>>>>>>
> >>>>>>> _______________________________________________
> >>>>>>> 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

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

Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Christian König]

Am 08.10.2018 um 20:15 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Tuesday, October 9, 2018 2:03 AM
>> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>> suspend
>>
>> Am 08.10.2018 um 19:58 schrieb Zhu, Rex:
>>>> -----Original Message-----
>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>> Sent: Tuesday, October 9, 2018 1:32 AM
>>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
>>>> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>> suspend
>>>>
>>>> Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
>>>>>> -----Original Message-----
>>>>>> From: Deucher, Alexander
>>>>>> Sent: Tuesday, October 9, 2018 12:21 AM
>>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher
>>>> <alexdeucher@gmail.com>
>>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>>>> suspend
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf
>> Of
>>>>>>> Zhu, Rex
>>>>>>> Sent: Monday, October 8, 2018 11:57 AM
>>>>>>> To: Alex Deucher <alexdeucher@gmail.com>
>>>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
>>>>>>> when suspend
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Alex Deucher <alexdeucher@gmail.com>
>>>>>>>> Sent: Thursday, October 4, 2018 11:35 AM
>>>>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>
>>>>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>>>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
>>>>>>>> when suspend
>>>>>>>>
>>>>>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com> wrote:
>>>>>>>>> driver don't release the ucode memory when suspend. so don't
>>>>>>>>> need to allocate bo when resume back.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
>>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>>>> index 9878212..adfeb93 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
>>>>>> amdgpu_device
>>>>>>>> *adev)
>>>>>>>>>                    return 0;
>>>>>>>>>            }
>>>>>>>>>
>>>>>>>>> -       if (!adev->in_gpu_reset) {
>>>>>>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
>>>>>>>>>                    err = amdgpu_bo_create_kernel(adev,
>>>>>>>>> adev->firmware.fw_size,
>>>>>>>> PAGE_SIZE,
>>>>>>>>>                                            amdgpu_sriov_vf(adev) ?
>>>>>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
>>>>>>>>> &adev->firmware.fw_buf,
>>>>>>>> Not sure if we support S3 in SR-IOV, but I think this will break
>>>>>>>> it because we'll lose vram contents and not re-init it.
>>>>>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
>>>>>>>
>>>>>>>     But I still confused why this patch will break the suspend if
>>>>>>> in SRIOV
>>>> case?
>>>>>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu
>>>>>> reset or S3, the data is lost.  GTT is retained since the OS manages that.
>>>>> The gart table was unpinned when suspend.so don't need to create the
>>>>> bo
>>>> again. we still copy the ucode to the bo.
>>>>> And in baremetal,  this function can return directly for S3.
>>>> That's irrelevant.
>>>>
>>>> The whole code is buggy since amdgpu_ucode_fini_bo() will drop the BO
>>>> independent if we are in reset or in suspend.
>>> We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset.
>> Yeah and exactly that's the bug which should be fixed instead.
> Sorry, I don't understand.
> Why we need to release the bo when suspend?
> In amdgpu, we create a bunch of bo(vram_scratch.robj, e.g), we only free them when driver unload.

You should not release the BO while suspending, but the 
amdgpu_ucode_fini_bo() and amdgpu_ucode_init_bo() should be called under 
the same conditions.

This means we should either separate filling the BO from allocating it 
(e.g. split amdgpu_ucode_init_bo into two functions) and then only call 
the filling function during GPU reset and resume.

Or we add the same "if (!adev->in_gpu_reset && !adev->in_suspend)" into 
the amdgpu_ucode_fini_bo() function as well.

I consider the first one more cleaner.

Christian.

>
> Rex
>
>> Christian.
>>
>>> Rex
>>>
>>>> The correct handling here is to remove the if all together and make
>>>> sure
>>>> amdgpu_bo_create_kernel() is ALWAYS called.
>>>>
>>>> Cause then it is always re-created if it isn't there already.
>>>>
>>>> Alternatively we could fix up the callers of amdgpu_ucode_init_bo()
>>>> and
>>>> amdgpu_ucode_fini_bo() to be correctly balanced.
>>>>
>>>> Christian.
>>>>
>>>>> Rex
>>>>>
>>>>>
>>>>>> Alex
>>>>>>
>>>>>>> Rex
>>>>>>>
>>>>>>>> Alex
>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 1.9.1
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> 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
> _______________________________________________
> 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/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Zhu, Rex]

> This means we should either separate filling the BO from allocating it (e.g.
> split amdgpu_ucode_init_bo into two functions) and then only call the filling
> function during GPU reset and resume.

Got it. Thanks.
I will refine this function.

Regards
Rex
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, October 9, 2018 2:22 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> suspend
> 
> Am 08.10.2018 um 20:15 schrieb Zhu, Rex:
> >
> >> -----Original Message-----
> >> From: Koenig, Christian
> >> Sent: Tuesday, October 9, 2018 2:03 AM
> >> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> >> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> >> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >> suspend
> >>
> >> Am 08.10.2018 um 19:58 schrieb Zhu, Rex:
> >>>> -----Original Message-----
> >>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >>>> Sent: Tuesday, October 9, 2018 1:32 AM
> >>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> >>>> <Alexander.Deucher@amd.com>; Alex Deucher
> <alexdeucher@gmail.com>
> >>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
> >>>> suspend
> >>>>
> >>>> Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
> >>>>>> -----Original Message-----
> >>>>>> From: Deucher, Alexander
> >>>>>> Sent: Tuesday, October 9, 2018 12:21 AM
> >>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Alex Deucher
> >>>> <alexdeucher@gmail.com>
> >>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>>> when suspend
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf
> >> Of
> >>>>>>> Zhu, Rex
> >>>>>>> Sent: Monday, October 8, 2018 11:57 AM
> >>>>>>> To: Alex Deucher <alexdeucher@gmail.com>
> >>>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>>>> when suspend
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Alex Deucher <alexdeucher@gmail.com>
> >>>>>>>> Sent: Thursday, October 4, 2018 11:35 AM
> >>>>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>
> >>>>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> >>>>>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>>>>> when suspend
> >>>>>>>>
> >>>>>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@amd.com>
> wrote:
> >>>>>>>>> driver don't release the ucode memory when suspend. so don't
> >>>>>>>>> need to allocate bo when resume back.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> >>>>>>>>> ---
> >>>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
> >>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>>>> index 9878212..adfeb93 100644
> >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >>>>>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
> >>>>>> amdgpu_device
> >>>>>>>> *adev)
> >>>>>>>>>                    return 0;
> >>>>>>>>>            }
> >>>>>>>>>
> >>>>>>>>> -       if (!adev->in_gpu_reset) {
> >>>>>>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
> >>>>>>>>>                    err = amdgpu_bo_create_kernel(adev,
> >>>>>>>>> adev->firmware.fw_size,
> >>>>>>>> PAGE_SIZE,
> >>>>>>>>>                                            amdgpu_sriov_vf(adev) ?
> >>>>>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> >>>>>>>>> &adev->firmware.fw_buf,
> >>>>>>>> Not sure if we support S3 in SR-IOV, but I think this will
> >>>>>>>> break it because we'll lose vram contents and not re-init it.
> >>>>>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
> >>>>>>>
> >>>>>>>     But I still confused why this patch will break the suspend
> >>>>>>> if in SRIOV
> >>>> case?
> >>>>>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu
> >>>>>> reset or S3, the data is lost.  GTT is retained since the OS manages
> that.
> >>>>> The gart table was unpinned when suspend.so don't need to create
> >>>>> the bo
> >>>> again. we still copy the ucode to the bo.
> >>>>> And in baremetal,  this function can return directly for S3.
> >>>> That's irrelevant.
> >>>>
> >>>> The whole code is buggy since amdgpu_ucode_fini_bo() will drop the
> >>>> BO independent if we are in reset or in suspend.
> >>> We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset.
> >> Yeah and exactly that's the bug which should be fixed instead.
> > Sorry, I don't understand.
> > Why we need to release the bo when suspend?
> > In amdgpu, we create a bunch of bo(vram_scratch.robj, e.g), we only free
> them when driver unload.
> 
> You should not release the BO while suspending, but the
> amdgpu_ucode_fini_bo() and amdgpu_ucode_init_bo() should be called
> under the same conditions.
> 
> This means we should either separate filling the BO from allocating it (e.g.
> split amdgpu_ucode_init_bo into two functions) and then only call the filling
> function during GPU reset and resume.
> 
> Or we add the same "if (!adev->in_gpu_reset && !adev->in_suspend)" into
> the amdgpu_ucode_fini_bo() function as well.
> 
> I consider the first one more cleaner.
> 
> Christian.
> 
> >
> > Rex
> >
> >> Christian.
> >>
> >>> Rex
> >>>
> >>>> The correct handling here is to remove the if all together and make
> >>>> sure
> >>>> amdgpu_bo_create_kernel() is ALWAYS called.
> >>>>
> >>>> Cause then it is always re-created if it isn't there already.
> >>>>
> >>>> Alternatively we could fix up the callers of amdgpu_ucode_init_bo()
> >>>> and
> >>>> amdgpu_ucode_fini_bo() to be correctly balanced.
> >>>>
> >>>> Christian.
> >>>>
> >>>>> Rex
> >>>>>
> >>>>>
> >>>>>> Alex
> >>>>>>
> >>>>>>> Rex
> >>>>>>>
> >>>>>>>> Alex
> >>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> 1.9.1
> >>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> 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
> > _______________________________________________
> > 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