On Thu, Jul 28, 2016 at 8:59 AM, StDenis, Tom <Tom.StDenis at amd.com> wrote:
> Yup, I fixed that in my worktree already.
Looks good to me.
Alex
>
>
> Tom
>
>
>
> ________________________________
> From: Zhu, Rex
> Sent: Thursday, July 28, 2016 08:59
>
> To: StDenis, Tom; Alex Deucher
> Cc: amd-gfx list
> Subject: Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before
> init
>
>
> you mean
>
> + if (cz_hwmgr->uvd_power_gated == bgate) {
> return 0;
> + }
>
>
> I didn't pay any attention at first.
>
>
> Best Regards
>
> Rex
>
>
> ________________________________
> From: StDenis, Tom
> Sent: Thursday, July 28, 2016 8:44:11 PM
> To: Zhu, Rex; Alex Deucher
> Cc: amd-gfx list
> Subject: Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before
> init
>
>
> 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 at lists.freedesktop.org> 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 at lists.freedesktop.org] 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
