On Tue, Jul 26, 2016 at 11:38 AM, Tom St Denis <tstdenis82 at gmail.com> wrote:
> Because of the ip_blocks init order powerplay would power down
> the UVD block before UVD start is called. This results in a VCPU
> hang.
>
> This patch prevents power down before UVD is initialized.
>
> Also correct the power up order so clocking is set after power
> is ungated.
>
> With this applied comparable clock/power behaviour to powerplay=0 with
> DPM is observed.
>
> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
This patch needs to be split into several patches and reworked a bit.
Also, don't include amdgpu.h in powerplay. We have cgs for access to
registers and data from adev, etc. The idea is to minimize the
dependencies between components. We shouldn't be accessing adev
directly in powerplay. A couple more comments inline.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 6 ++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 5 -----
> drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 8 ++++---
> drivers/gpu/drm/amd/amdgpu/vi.c | 12 ++++-------
> .../drm/amd/powerplay/hwmgr/cz_clockpowergating.c | 25 ++++++++++++++++++----
> drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 7 ++++++
> 6 files changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index d0460ea2f85b..5616b16e6c0a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1692,6 +1692,7 @@ struct amdgpu_uvd {
> uint32_t srbm_soft_reset;
> int cg_state, pg_state;
> struct mutex pg_lock;
> + bool is_init;
> };
>
> /*
> @@ -2518,5 +2519,10 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev );
> static inline int amdgpu_dm_display_resume(struct amdgpu_device *adev) { return 0; }
> #endif
>
> +struct amdgpu_cgs_device {
> + struct cgs_device base;
> + struct amdgpu_device *adev;
> +};
> +
> #include "amdgpu_object.h"
> #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> index ee95e950a19b..d553e399a835 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> @@ -33,11 +33,6 @@
> #include "atom.h"
> #include "amdgpu_ucode.h"
>
> -struct amdgpu_cgs_device {
> - struct cgs_device base;
> - struct amdgpu_device *adev;
> -};
> -
> #define CGS_FUNC_ADEV \
> struct amdgpu_device *adev = \
> ((struct amdgpu_cgs_device *)cgs_device)->adev
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> index 422d5300b92e..3b93327c5e25 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> @@ -389,9 +389,9 @@ static int uvd_v6_0_start(struct amdgpu_device *adev)
> uint32_t mp_swap_cntl;
> int i, j, r;
>
> - /* is power gated? then we can't start (TODO: re-enable power) */
> - if (adev->uvd.pg_state)
> - return -EINVAL;
> + /* is power gated? then we can't start but don't return an error */
> + if (adev->uvd.is_init && adev->uvd.pg_state)
> + return 0;
>
> /* set CG state to -1 for unset */
> adev->uvd.cg_state = -1;
> @@ -662,6 +662,8 @@ static int uvd_v6_0_ring_test_ring(struct amdgpu_ring *ring)
> ring->idx, tmp);
> r = -EINVAL;
> }
> + if (!r)
> + adev->uvd.is_init = true;
> return r;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
> index 78fea940d120..f4fdde0641b0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -1583,10 +1583,8 @@ static int vi_common_early_init(void *handle)
> if (adev->rev_id != 0x00) {
> adev->pg_flags |= AMD_PG_SUPPORT_GFX_PG |
> AMD_PG_SUPPORT_GFX_SMG |
> - AMD_PG_SUPPORT_GFX_PIPELINE;
> - /* powerplay UVD PG doesn't work yet */
> - if (!amdgpu_powerplay)
> - adev->pg_flags |= AMD_PG_SUPPORT_UVD;
> + AMD_PG_SUPPORT_GFX_PIPELINE |
> + AMD_PG_SUPPORT_UVD;
This should be a separate patch.
> }
> adev->external_rev_id = adev->rev_id + 0x1;
> break;
> @@ -1608,10 +1606,8 @@ static int vi_common_early_init(void *handle)
> AMD_CG_SUPPORT_SDMA_LS;
> adev->pg_flags |= AMD_PG_SUPPORT_GFX_PG |
> AMD_PG_SUPPORT_GFX_SMG |
> - AMD_PG_SUPPORT_GFX_PIPELINE;
> - /* powerplay UVD PG doesn't work yet */
> - if (!amdgpu_powerplay)
> - adev->pg_flags |= AMD_PG_SUPPORT_UVD;
> + AMD_PG_SUPPORT_GFX_PIPELINE |
> + AMD_PG_SUPPORT_UVD;
Same with this.
> adev->external_rev_id = adev->rev_id + 0x1;
> break;
> default:
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_clockpowergating.c b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_clockpowergating.c
> index 2da548f6337e..baa7366fad53 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_clockpowergating.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_clockpowergating.c
> @@ -21,9 +21,13 @@
> *
> */
>
> +#include "amdgpu.h"
> #include "hwmgr.h"
> #include "cz_clockpowergating.h"
> #include "cz_ppsmc.h"
> +#include "cgs_linux.h"
> +#include "uvd/uvd_6_0_d.h"
> +#include "uvd/uvd_6_0_sh_mask.h"
>
> /* PhyID -> Status Mapping in DDI_PHY_GEN_STATUS
> 0 GFX0L (3:0), (27:24),
> @@ -160,12 +164,24 @@ int cz_enable_disable_vce_dpm(struct pp_hwmgr *hwmgr, bool enable)
> int cz_dpm_powergate_uvd(struct pp_hwmgr *hwmgr, bool bgate)
> {
> struct cz_hwmgr *cz_hwmgr = (struct cz_hwmgr *)(hwmgr->backend);
> + struct amdgpu_cgs_device *cgs_dev = hwmgr->device;
> + struct amdgpu_device *adev = cgs_dev->adev;
>
> - if (cz_hwmgr->uvd_power_gated == bgate)
> + if (!adev->uvd.is_init)
> return 0;
>
> + mutex_lock(&adev->uvd.pg_lock);
> +
> + if (cz_hwmgr->uvd_power_gated == bgate) {
> + mutex_unlock(&adev->uvd.pg_lock);
> + return 0;
> + }
> +
> + adev->uvd.pg_state = bgate;
> cz_hwmgr->uvd_power_gated = bgate;
>
> + WREG32(mmUVD_POWER_STATUS, UVD_POWER_STATUS__UVD_PG_EN_MASK);
> +
Could this be moved to the uvd 6 module?
> if (bgate) {
> cgs_set_clockgating_state(hwmgr->device,
> AMD_IP_BLOCK_TYPE_UVD,
> @@ -177,14 +193,15 @@ int cz_dpm_powergate_uvd(struct pp_hwmgr *hwmgr, bool bgate)
> cz_dpm_powerdown_uvd(hwmgr);
> } else {
> cz_dpm_powerup_uvd(hwmgr);
> - cgs_set_clockgating_state(hwmgr->device,
> - AMD_IP_BLOCK_TYPE_UVD,
> - AMD_PG_STATE_GATE);
> cgs_set_powergating_state(hwmgr->device,
> AMD_IP_BLOCK_TYPE_UVD,
> AMD_CG_STATE_UNGATE);
> + cgs_set_clockgating_state(hwmgr->device,
> + AMD_IP_BLOCK_TYPE_UVD,
> + AMD_PG_STATE_GATE);
I think this is a standalone fix and should be split into a separate patch.
> cz_dpm_update_uvd_dpm(hwmgr, false);
> }
> + mutex_unlock(&adev->uvd.pg_lock);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> index 9bf622e123b6..bed0013674a1 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> @@ -39,6 +39,7 @@
> #include "power_state.h"
> #include "cz_clockpowergating.h"
> #include "pp_debug.h"
> +#include "amdgpu.h"
>
> #define ixSMUSVI_NB_CURRENTVID 0xD8230044
> #define CURRENT_NB_VID_MASK 0xff000000
> @@ -1356,6 +1357,12 @@ static int cz_dpm_force_dpm_level(struct pp_hwmgr *hwmgr,
>
> int cz_dpm_powerdown_uvd(struct pp_hwmgr *hwmgr)
> {
> + struct amdgpu_cgs_device *cgs_dev = hwmgr->device;
> + struct amdgpu_device *adev = cgs_dev->adev;
> +
> + if (!adev->uvd.is_init)
> + return 0;
> +
> if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
> PHM_PlatformCaps_UVDPowerGating))
> return smum_send_msg_to_smc(hwmgr->smumgr,
> --
> 2.9.2
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
