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/eeb8a0f1/attachment-0001.html>
