Hi Rex,
Thanks. BTW I fixed the one liner {} in the PP code (removed the {} braces) in my worktree after I sent that in case anyone notices that :-)
Tom
________________________________
From: Zhu, Rex
Sent: Thursday, July 28, 2016 08:43
To: StDenis, Tom; Alex Deucher
Cc: amd-gfx list
Subject: Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
Looks good to me.
Best Regards
Rex
________________________________
From: StDenis, Tom
Sent: Thursday, July 28, 2016 8:19:52 PM
To: Zhu, Rex; Alex Deucher
Cc: amd-gfx list
Subject: Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
Nevermind I moved the locking into amdgpu_pm.c and that did the trick.
Attached is a patch that contains all the changes. If you guys want to give it a quick once-through I can then start splitting it up per Alex's comments.
Tom
________________________________
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> on behalf of StDenis, Tom <Tom.StDenis at amd.com>
Sent: Thursday, July 28, 2016 07:10
To: Zhu, Rex; Alex Deucher
Cc: amd-gfx list
Subject: Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
Quick question, how am I meant to get access to pm.mutex from powerplay?
I need a lock I can see around the SMU calls and in the amdgpu side (for userspace locking).
Tom
________________________________
From: Zhu, Rex
Sent: Thursday, July 28, 2016 03:43
To: Alex Deucher; Tom St Denis
Cc: StDenis, Tom; amd-gfx list
Subject: RE: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
From: amd-gfx [mailto:amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Alex Deucher
Sent: Thursday, July 28, 2016 1:46 PM
To: Tom St Denis
Cc: StDenis, Tom; amd-gfx list
Subject: Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
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.
Rex: I also think so.
1. We can move
+ WREG32(mmUVD_POWER_STATUS,
+ UVD_POWER_STATUS__UVD_PG_EN_MASK |
+ UVD_POWER_STATUS__UVD_PG_MODE_MASK);
+ else
+ WREG32(mmUVD_POWER_STATUS,
+ UVD_POWER_STATUS__UVD_PG_EN_MASK);
to uvd_v6_0_start. no need to visit adev in powerplay and dpm. And uvd test also can pass.
2. for the lock, we can just use pm.mutex.
3. please also delete enable_clock_power_gatings_tasks in resume_action_chain in a separate patch for powerplay.
4. do we need to add cg_state, pg_state?
Best Regards
Rex
> ---
> 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
_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20160728/5f3366b4/attachment-0001.html>
