Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
Yup, I fixed that in my worktree already.
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 on behalf of StDenis, Tom
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_init;
> };
>
> /*
> @@ -2518,5 +2519,10 @@ int amdgpu_dm_display_resu
Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
On Thu, Jul 28, 2016 at 8:59 AM, StDenis, Tom 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 on behalf of StDenis,
> Tom
> 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-boun...@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 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
>
> 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
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 on behalf of StDenis, Tom
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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;
>
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 on behalf of StDenis, Tom
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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 @@
>
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 on behalf of StDenis, Tom
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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_
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 on behalf of StDenis, Tom
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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->u
Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
I found that moving the PG_EN to start would cause problems. I'm pretty sure
you have to write it just before calling the firmware otherwise bad things
happen which is why I moved it there. We could probably move it into the
uvd_v6_0's powergating function which is called just before the smu call.
I'll take care of comments 2/3. For 4 it was a sanity check to prevent
out-of-order operations. It can probably be safely removed now.
My work tree also has a half-implemented VCE PG/CG (PG works, CG doesn't) that
has some of the same issues so I'll fix them too and sort the tree out a bit
and re-send the whole lot sometime today.
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-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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
&g
RE: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
From: amd-gfx [mailto:amd-gfx-boun...@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 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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->
Re: [PATCH 4/4] drm/amd/powerplay: Prevent UVD powerdown before init
On Tue, Jul 26, 2016 at 11:38 AM, Tom St Denis 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
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_tsrbm_soft_reset;
> int cg_state, pg_state;
> struct mutexpg_lock;
> + boolis_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;
> -
