>Still has risk for s3, we would better not put PG disabling (gfxoff) behind of CG
>disabling, even here is SMC clockgating. It is to avoid any MMU using when
>gfx is in "off" state.
I think logically we need to power on/off GFX ip through SMU, so first ungate SMU block first.
Currently, it is not a big deal, because SMU don't support CG/PG on all legacy asics.
Best Regards
Rex
________________________________
From: Huang Rui <ray.huang@xxxxxxx>
Sent: Wednesday, June 6, 2018 10:33 AM
To: Zhu, Rex
Cc: amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/amdgpu: Make gfx_off control by GFX ip
On Tue, Jun 05, 2018 at 07:51:17PM +0800, Rex Zhu wrote:
> v3: 1. Delete the dead gfx off code in powerplay late_init.
> 2. Revert v2.
> 3. call smu to power on gfx at the begin of ip_suspend/device_fini
> 4. only power off gfx ip in the end of gfx power gate function
> v2: Delete the dead gfx off code in ip_suspend.
>
> gfx off should be controlled by GFX IP.
> Powerplay only export interface to gfx ip.
> This logic is same as uvd/vce cg/pg.
>
> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++------
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 4 ++++
> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 8 --------
> drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c | 4 ++--
> 4 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index df6ef9c..ee87fea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1830,6 +1830,8 @@ static int amdgpu_device_ip_fini(struct amdgpu_device *adev)
> adev->ip_blocks[i].version->funcs->name, r);
> return r;
> }
> + if (adev->powerplay.pp_funcs->set_powergating_by_smu)
> + amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, false);
> r = adev->ip_blocks[i].version->funcs->hw_fini((void *)adev);
> /* XXX handle errors */
> if (r) {
> @@ -1940,12 +1942,6 @@ int amdgpu_device_ip_suspend(struct amdgpu_device *adev)
> if (amdgpu_sriov_vf(adev))
> amdgpu_virt_request_full_gpu(adev, false);
>
> - /* ungate SMC block powergating */
> - if (adev->powerplay.pp_feature & PP_GFXOFF_MASK)
> - amdgpu_device_ip_set_powergating_state(adev,
> - AMD_IP_BLOCK_TYPE_SMC,
> - AMD_CG_STATE_UNGATE);
> -
> /* ungate SMC block first */
> r = amdgpu_device_ip_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_SMC,
> AMD_CG_STATE_UNGATE);
> @@ -1953,6 +1949,10 @@ int amdgpu_device_ip_suspend(struct amdgpu_device *adev)
> DRM_ERROR("set_clockgating_state(ungate) SMC failed %d\n", r);
> }
>
> + /* call smu to disable gfx off feature first when suspend */
> + if (adev->powerplay.pp_funcs->set_powergating_by_smu)
> + amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, false);
> +
Still has risk for s3, we would better not put PG disabling (gfxoff) behind of CG
disabling, even here is SMC clockgating. It is to avoid any MMU using when
gfx is in "off" state.
Thanks,
Ray
> for (i = adev->num_ip_blocks - 1; i >= 0; i--) {
> if (!adev->ip_blocks[i].status.valid)
> continue;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index c382969..43c8023 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -3712,6 +3712,10 @@ static int gfx_v9_0_set_powergating_state(void *handle,
>
> /* update mgcg state */
> gfx_v9_0_update_gfx_mg_power_gating(adev, enable);
> +
> + /* set gfx off through smu */
> + if (enable && adev->powerplay.pp_funcs->set_powergating_by_smu)
> + amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, true);
> break;
> default:
> break;
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index fe3ed8c..8bb3a9a 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -180,7 +180,6 @@ static int pp_late_init(void *handle)
> {
> struct amdgpu_device *adev = handle;
> struct pp_hwmgr *hwmgr = adev->powerplay.pp_handle;
> - int ret;
>
> if (hwmgr && hwmgr->pm_en) {
> mutex_lock(&hwmgr->smu_lock);
> @@ -191,13 +190,6 @@ static int pp_late_init(void *handle)
> if (adev->pm.smu_prv_buffer_size != 0)
> pp_reserve_vram_for_smu(adev);
>
> - if (hwmgr->hwmgr_func->gfx_off_control &&
> - (hwmgr->feature_mask & PP_GFXOFF_MASK)) {
> - ret = hwmgr->hwmgr_func->gfx_off_control(hwmgr, true);
> - if (ret)
> - pr_err("gfx off enabling failed!\n");
> - }
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
> index b9e6dfb..5abae28 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
> @@ -314,7 +314,7 @@ static int smu10_disable_gfx_off(struct pp_hwmgr *hwmgr)
>
> static int smu10_disable_dpm_tasks(struct pp_hwmgr *hwmgr)
> {
> - return smu10_disable_gfx_off(hwmgr);
> + return 0;
> }
>
> static int smu10_enable_gfx_off(struct pp_hwmgr *hwmgr)
> @@ -329,7 +329,7 @@ static int smu10_enable_gfx_off(struct pp_hwmgr *hwmgr)
>
> static int smu10_enable_dpm_tasks(struct pp_hwmgr *hwmgr)
> {
> - return smu10_enable_gfx_off(hwmgr);
> + return 0;
> }
>
> static int smu10_gfx_off_control(struct pp_hwmgr *hwmgr, bool enable)
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
amd-gfx Info Page - freedesktop.org<https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
lists.freedesktop.org
Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the following form. Use of all freedesktop.org lists is subject to our Code of Conduct.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180606/59d359c0/attachment.html>
