* [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged
@ 2018-09-10 16:49 James Zhu
[not found] ` <1536598142-19689-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 22+ messages in thread
From: James Zhu @ 2018-09-10 16:49 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: james.zhu-5C7GfCeVMHo
Signed-off-by: James Zhu <James.Zhu@amd.com>
When VCN PG state is unchanged, it is unnecessary to reset
power gate state again.
---
drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
index 2664bb2..86d98d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
@@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle,
* revisit this when there is a cleaner line between
* the smc and the hw blocks
*/
+ int ret;
+ static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+ if (state == cur_state)
+ return 0;
+
if (state == AMD_PG_STATE_GATE)
- return vcn_v1_0_stop(adev);
+ ret = vcn_v1_0_stop(adev);
else
- return vcn_v1_0_start(adev);
+ ret = vcn_v1_0_start(adev);
+
+ if (!ret)
+ cur_state = state;
+ return ret;
}
static const struct amd_ip_funcs vcn_v1_0_ip_funcs = {
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread[parent not found: <1536598142-19689-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged [not found] ` <1536598142-19689-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org> @ 2018-09-11 14:44 ` Alex Deucher [not found] ` <CADnq5_P5Oj_Fdxhv2n8ZxDTm1mV_NQo6M1kmgGzLV0fHhhR8sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-09-13 20:55 ` [PATCH v2] drm/amdgpu:No action " James Zhu 1 sibling, 1 reply; 22+ messages in thread From: Alex Deucher @ 2018-09-11 14:44 UTC (permalink / raw) To: James Zhu; +Cc: James Zhu, amd-gfx list On Mon, Sep 10, 2018 at 4:34 PM James Zhu <jzhums@gmail.com> wrote: > > Signed-off-by: James Zhu <James.Zhu@amd.com> > > When VCN PG state is unchanged, it is unnecessary to reset > power gate state again. Don't you need to initialize and store the PG state somewhere? You are just using a local variable here. Alex > --- > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > index 2664bb2..86d98d2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > @@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle, > * revisit this when there is a cleaner line between > * the smc and the hw blocks > */ > + int ret; > + static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE; > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > + if (state == cur_state) > + return 0; > + > if (state == AMD_PG_STATE_GATE) > - return vcn_v1_0_stop(adev); > + ret = vcn_v1_0_stop(adev); > else > - return vcn_v1_0_start(adev); > + ret = vcn_v1_0_start(adev); > + > + if (!ret) > + cur_state = state; > + return ret; > } > > static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > -- > 2.7.4 > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CADnq5_P5Oj_Fdxhv2n8ZxDTm1mV_NQo6M1kmgGzLV0fHhhR8sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged [not found] ` <CADnq5_P5Oj_Fdxhv2n8ZxDTm1mV_NQo6M1kmgGzLV0fHhhR8sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2018-09-11 15:06 ` James Zhu [not found] ` <d358b2be-26d1-627a-7105-5d90685d0f3b-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-11 15:06 UTC (permalink / raw) To: Alex Deucher, James Zhu; +Cc: James Zhu, amd-gfx list [-- Attachment #1.1: Type: text/plain, Size: 2225 bytes --] On 2018-09-11 10:44 AM, Alex Deucher wrote: > On Mon, Sep 10, 2018 at 4:34 PM James Zhu <jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: >> Signed-off-by: James Zhu <James.Zhu-5C7GfCeVMHo@public.gmane.org> >> >> When VCN PG state is unchanged, it is unnecessary to reset >> power gate state again. > Don't you need to initialize and store the PG state somewhere? You > are just using a local variable here. > > Alex > Hi Alex, I used */static/* for this local state variable(*cur_state*) with initialization state AMD_PG_STATE_GATE. this variable's scope is only inside this function, but it's initialization is done once at compile time and it's lifetime will last until the driver exit. Since it is only used inside this function, I didn't put it into struct amdgpu_vcn. Best Regards! James Zhu >> --- >> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >> index 2664bb2..86d98d2 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >> @@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle, >> * revisit this when there is a cleaner line between >> * the smc and the hw blocks >> */ >> + int ret; >> + static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE; >> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >> >> + if (state == cur_state) >> + return 0; >> + >> if (state == AMD_PG_STATE_GATE) >> - return vcn_v1_0_stop(adev); >> + ret = vcn_v1_0_stop(adev); >> else >> - return vcn_v1_0_start(adev); >> + ret = vcn_v1_0_start(adev); >> + >> + if (!ret) >> + cur_state = state; >> + return ret; >> } >> >> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >> -- >> 2.7.4 >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx [-- Attachment #1.2: Type: text/html, Size: 3361 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <d358b2be-26d1-627a-7105-5d90685d0f3b-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged [not found] ` <d358b2be-26d1-627a-7105-5d90685d0f3b-5C7GfCeVMHo@public.gmane.org> @ 2018-09-11 15:17 ` Christian König [not found] ` <68b30f71-86eb-89a0-0f8d-fcfb18e89cc2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Christian König @ 2018-09-11 15:17 UTC (permalink / raw) To: James Zhu, Alex Deucher, James Zhu; +Cc: James Zhu, amd-gfx list [-- Attachment #1.1: Type: text/plain, Size: 2657 bytes --] Am 11.09.2018 um 17:06 schrieb James Zhu: > > > > On 2018-09-11 10:44 AM, Alex Deucher wrote: >> On Mon, Sep 10, 2018 at 4:34 PM James Zhu<jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: >>> Signed-off-by: James Zhu<James.Zhu-5C7GfCeVMHo@public.gmane.org> >>> >>> When VCN PG state is unchanged, it is unnecessary to reset >>> power gate state again. >> Don't you need to initialize and store the PG state somewhere? You >> are just using a local variable here. >> >> Alex >> > Hi Alex, > > I used */static/* for this local state variable(*cur_state*) with > initialization state AMD_PG_STATE_GATE. That won't work correctly. A "static" variable is global, but the power state is per device. Regards, Christian. > this variable's scope is only inside this function, but it's > initialization is done > once at compile time and it's lifetime will last until the driver exit. > > Since it is only used inside this function, I didn't put it into > struct amdgpu_vcn. > > Best Regards! > James Zhu >>> --- >>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>> index 2664bb2..86d98d2 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>> @@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle, >>> * revisit this when there is a cleaner line between >>> * the smc and the hw blocks >>> */ >>> + int ret; >>> + static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE; >>> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >>> >>> + if (state == cur_state) >>> + return 0; >>> + >>> if (state == AMD_PG_STATE_GATE) >>> - return vcn_v1_0_stop(adev); >>> + ret = vcn_v1_0_stop(adev); >>> else >>> - return vcn_v1_0_start(adev); >>> + ret = vcn_v1_0_start(adev); >>> + >>> + if (!ret) >>> + cur_state = state; >>> + return ret; >>> } >>> >>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx [-- Attachment #1.2: Type: text/html, Size: 4622 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <68b30f71-86eb-89a0-0f8d-fcfb18e89cc2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged [not found] ` <68b30f71-86eb-89a0-0f8d-fcfb18e89cc2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2018-09-11 15:29 ` James Zhu [not found] ` <62baa235-e113-3635-6060-f5ffcc14e967-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-11 15:29 UTC (permalink / raw) To: christian.koenig-5C7GfCeVMHo, Alex Deucher, James Zhu Cc: James Zhu, amd-gfx list [-- Attachment #1.1: Type: text/plain, Size: 2970 bytes --] On 2018-09-11 11:17 AM, Christian König wrote: > Am 11.09.2018 um 17:06 schrieb James Zhu: >> >> >> >> On 2018-09-11 10:44 AM, Alex Deucher wrote: >>> On Mon, Sep 10, 2018 at 4:34 PM James Zhu<jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: >>>> Signed-off-by: James Zhu<James.Zhu-5C7GfCeVMHo@public.gmane.org> >>>> >>>> When VCN PG state is unchanged, it is unnecessary to reset >>>> power gate state again. >>> Don't you need to initialize and store the PG state somewhere? You >>> are just using a local variable here. >>> >>> Alex >>> >> Hi Alex, >> >> I used */static/* for this local state variable(*cur_state*) with >> initialization state AMD_PG_STATE_GATE. > > That won't work correctly. A "static" variable is global, but the > power state is per device. > > Regards, > Christian. > This "static" variable under local scope is mainly for VCN used only. It only tracks VCN's PG state. (currently VCN only have one hardware instance) Best Regards! James Zhu >> this variable's scope is only inside this function, but it's >> initialization is done >> once at compile time and it's lifetime will last until the driver exit. >> >> Since it is only used inside this function, I didn't put it into >> struct amdgpu_vcn. >> >> Best Regards! >> James Zhu >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>> index 2664bb2..86d98d2 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>> @@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle, >>>> * revisit this when there is a cleaner line between >>>> * the smc and the hw blocks >>>> */ >>>> + int ret; >>>> + static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE; >>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >>>> >>>> + if (state == cur_state) >>>> + return 0; >>>> + >>>> if (state == AMD_PG_STATE_GATE) >>>> - return vcn_v1_0_stop(adev); >>>> + ret = vcn_v1_0_stop(adev); >>>> else >>>> - return vcn_v1_0_start(adev); >>>> + ret = vcn_v1_0_start(adev); >>>> + >>>> + if (!ret) >>>> + cur_state = state; >>>> + return ret; >>>> } >>>> >>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> >> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > [-- Attachment #1.2: Type: text/html, Size: 5417 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <62baa235-e113-3635-6060-f5ffcc14e967-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged [not found] ` <62baa235-e113-3635-6060-f5ffcc14e967-5C7GfCeVMHo@public.gmane.org> @ 2018-09-11 15:36 ` Christian König [not found] ` <cbd39fc8-bd5e-cd11-b9f2-4b91185429f7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Christian König @ 2018-09-11 15:36 UTC (permalink / raw) To: James Zhu, christian.koenig-5C7GfCeVMHo, Alex Deucher, James Zhu Cc: James Zhu, amd-gfx list [-- Attachment #1.1: Type: text/plain, Size: 3432 bytes --] Am 11.09.2018 um 17:29 schrieb James Zhu: > > > > On 2018-09-11 11:17 AM, Christian König wrote: >> Am 11.09.2018 um 17:06 schrieb James Zhu: >>> >>> >>> >>> On 2018-09-11 10:44 AM, Alex Deucher wrote: >>>> On Mon, Sep 10, 2018 at 4:34 PM James Zhu<jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: >>>>> Signed-off-by: James Zhu<James.Zhu-5C7GfCeVMHo@public.gmane.org> >>>>> >>>>> When VCN PG state is unchanged, it is unnecessary to reset >>>>> power gate state again. >>>> Don't you need to initialize and store the PG state somewhere? You >>>> are just using a local variable here. >>>> >>>> Alex >>>> >>> Hi Alex, >>> >>> I used */static/* for this local state variable(*cur_state*) with >>> initialization state AMD_PG_STATE_GATE. >> >> That won't work correctly. A "static" variable is global, but the >> power state is per device. >> >> Regards, >> Christian. >> > This "static" variable under local scope is mainly for VCN used only. > It only tracks VCN's PG state. > (currently VCN only have one hardware instance) Still an absolute no-go for kernel coding. VCN will soon be used for dGPUs as well. Please fix and reiterate, Christian. > > Best Regards! > James Zhu >>> this variable's scope is only inside this function, but it's >>> initialization is done >>> once at compile time and it's lifetime will last until the driver exit. >>> >>> Since it is only used inside this function, I didn't put it into >>> struct amdgpu_vcn. >>> >>> Best Regards! >>> James Zhu >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++-- >>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> index 2664bb2..86d98d2 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> @@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle, >>>>> * revisit this when there is a cleaner line between >>>>> * the smc and the hw blocks >>>>> */ >>>>> + int ret; >>>>> + static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE; >>>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >>>>> >>>>> + if (state == cur_state) >>>>> + return 0; >>>>> + >>>>> if (state == AMD_PG_STATE_GATE) >>>>> - return vcn_v1_0_stop(adev); >>>>> + ret = vcn_v1_0_stop(adev); >>>>> else >>>>> - return vcn_v1_0_start(adev); >>>>> + ret = vcn_v1_0_start(adev); >>>>> + >>>>> + if (!ret) >>>>> + cur_state = state; >>>>> + return ret; >>>>> } >>>>> >>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>> -- >>>>> 2.7.4 >>>>> >>>>> _______________________________________________ >>>>> amd-gfx mailing list >>>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> >>> >>> >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx [-- Attachment #1.2: Type: text/html, Size: 6682 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <cbd39fc8-bd5e-cd11-b9f2-4b91185429f7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged [not found] ` <cbd39fc8-bd5e-cd11-b9f2-4b91185429f7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2018-09-11 15:44 ` James Zhu 0 siblings, 0 replies; 22+ messages in thread From: James Zhu @ 2018-09-11 15:44 UTC (permalink / raw) To: christian.koenig-5C7GfCeVMHo, Alex Deucher, James Zhu Cc: James Zhu, amd-gfx list [-- Attachment #1.1: Type: text/plain, Size: 3858 bytes --] On 2018-09-11 11:36 AM, Christian König wrote: > Am 11.09.2018 um 17:29 schrieb James Zhu: >> >> >> >> On 2018-09-11 11:17 AM, Christian König wrote: >>> Am 11.09.2018 um 17:06 schrieb James Zhu: >>>> >>>> >>>> >>>> On 2018-09-11 10:44 AM, Alex Deucher wrote: >>>>> On Mon, Sep 10, 2018 at 4:34 PM James Zhu<jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: >>>>>> Signed-off-by: James Zhu<James.Zhu-5C7GfCeVMHo@public.gmane.org> >>>>>> >>>>>> When VCN PG state is unchanged, it is unnecessary to reset >>>>>> power gate state again. >>>>> Don't you need to initialize and store the PG state somewhere? You >>>>> are just using a local variable here. >>>>> >>>>> Alex >>>>> >>>> Hi Alex, >>>> >>>> I used */static/* for this local state variable(*cur_state*) with >>>> initialization state AMD_PG_STATE_GATE. >>> >>> That won't work correctly. A "static" variable is global, but the >>> power state is per device. >>> >>> Regards, >>> Christian. >>> >> This "static" variable under local scope is mainly for VCN used only. >> It only tracks VCN's PG state. >> (currently VCN only have one hardware instance) > > Still an absolute no-go for kernel coding. VCN will soon be used for > dGPUs as well. > > Please fix and reiterate, > Christian. I see, then I will put this current state track into struct amdgpu_vcn. By the way, under multiple dPGU situation, I though per dPGU will create it's own driver instance. this static variable won't be shared cross driver instance. Am I right? Thanks! James Zhu >> >> Best Regards! >> James Zhu >>>> this variable's scope is only inside this function, but it's >>>> initialization is done >>>> once at compile time and it's lifetime will last until the driver exit. >>>> >>>> Since it is only used inside this function, I didn't put it into >>>> struct amdgpu_vcn. >>>> >>>> Best Regards! >>>> James Zhu >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 13 +++++++++++-- >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> index 2664bb2..86d98d2 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> @@ -1633,12 +1633,21 @@ static int vcn_v1_0_set_powergating_state(void *handle, >>>>>> * revisit this when there is a cleaner line between >>>>>> * the smc and the hw blocks >>>>>> */ >>>>>> + int ret; >>>>>> + static enum amd_powergating_state cur_state = AMD_PG_STATE_GATE; >>>>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >>>>>> >>>>>> + if (state == cur_state) >>>>>> + return 0; >>>>>> + >>>>>> if (state == AMD_PG_STATE_GATE) >>>>>> - return vcn_v1_0_stop(adev); >>>>>> + ret = vcn_v1_0_stop(adev); >>>>>> else >>>>>> - return vcn_v1_0_start(adev); >>>>>> + ret = vcn_v1_0_start(adev); >>>>>> + >>>>>> + if (!ret) >>>>>> + cur_state = state; >>>>>> + return ret; >>>>>> } >>>>>> >>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>>> -- >>>>>> 2.7.4 >>>>>> >>>>>> _______________________________________________ >>>>>> amd-gfx mailing list >>>>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>> >>>> >>>> >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> >> >> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > [-- Attachment #1.2: Type: text/html, Size: 7571 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <1536598142-19689-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org> 2018-09-11 14:44 ` Alex Deucher @ 2018-09-13 20:55 ` James Zhu [not found] ` <1536872144-18347-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org> 1 sibling, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-13 20:55 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: james.zhu-5C7GfCeVMHo When VCN PG state is unchanged, it is unnecessary to reset power gate state Signed-off-by: James Zhu <James.Zhu@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h index 0b0b863..d2219ab 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h @@ -69,6 +69,7 @@ struct amdgpu_vcn { struct amdgpu_ring ring_jpeg; struct amdgpu_irq_src irq; unsigned num_enc_rings; + enum amd_powergating_state cur_state; }; int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c index 2664bb2..2cde0b4 100644 --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c @@ -1633,12 +1633,20 @@ static int vcn_v1_0_set_powergating_state(void *handle, * revisit this when there is a cleaner line between * the smc and the hw blocks */ + int ret; struct amdgpu_device *adev = (struct amdgpu_device *)handle; + if(state == adev->vcn.cur_state) + return 0; + if (state == AMD_PG_STATE_GATE) - return vcn_v1_0_stop(adev); + ret = vcn_v1_0_stop(adev); else - return vcn_v1_0_start(adev); + ret = vcn_v1_0_start(adev); + + if(!ret) + adev->vcn.cur_state = state; + return ret; } static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { -- 2.7.4 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
[parent not found: <1536872144-18347-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <1536872144-18347-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org> @ 2018-09-20 14:19 ` Zhu, James 2018-09-20 15:14 ` Alex Deucher 1 sibling, 0 replies; 22+ messages in thread From: Zhu, James @ 2018-09-20 14:19 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Deucher, Alexander, Koenig, Christian [-- Attachment #1.1: Type: text/plain, Size: 2218 bytes --] Ping Alex and Christian, Could you give a review on this updated patch? Thanks & Best Regards! James Zhu ________________________________ From: James Zhu <jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sent: Thursday, September 13, 2018 4:55 PM To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Cc: Zhu, James Subject: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged When VCN PG state is unchanged, it is unnecessary to reset power gate state Signed-off-by: James Zhu <James.Zhu-5C7GfCeVMHo@public.gmane.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h index 0b0b863..d2219ab 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h @@ -69,6 +69,7 @@ struct amdgpu_vcn { struct amdgpu_ring ring_jpeg; struct amdgpu_irq_src irq; unsigned num_enc_rings; + enum amd_powergating_state cur_state; }; int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c index 2664bb2..2cde0b4 100644 --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c @@ -1633,12 +1633,20 @@ static int vcn_v1_0_set_powergating_state(void *handle, * revisit this when there is a cleaner line between * the smc and the hw blocks */ + int ret; struct amdgpu_device *adev = (struct amdgpu_device *)handle; + if(state == adev->vcn.cur_state) + return 0; + if (state == AMD_PG_STATE_GATE) - return vcn_v1_0_stop(adev); + ret = vcn_v1_0_stop(adev); else - return vcn_v1_0_start(adev); + ret = vcn_v1_0_start(adev); + + if(!ret) + adev->vcn.cur_state = state; + return ret; } static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { -- 2.7.4 [-- Attachment #1.2: Type: text/html, Size: 5066 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <1536872144-18347-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org> 2018-09-20 14:19 ` Zhu, James @ 2018-09-20 15:14 ` Alex Deucher [not found] ` <CADnq5_Nb-QPFox25ggaaOVO12P2_BUN7gmVWoJYpim1mYS9r3Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 22+ messages in thread From: Alex Deucher @ 2018-09-20 15:14 UTC (permalink / raw) To: James Zhu; +Cc: James Zhu, amd-gfx list On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: > > When VCN PG state is unchanged, it is unnecessary to reset power > gate state > > Signed-off-by: James Zhu <James.Zhu@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > index 0b0b863..d2219ab 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > @@ -69,6 +69,7 @@ struct amdgpu_vcn { > struct amdgpu_ring ring_jpeg; > struct amdgpu_irq_src irq; > unsigned num_enc_rings; > + enum amd_powergating_state cur_state; Does the default value (0) at init time properly reflect the default powergating state? If so, Acked-by: Alex Deucher <alexander.deucher@amd.com> > }; > > int amdgpu_vcn_sw_init(struct amdgpu_device *adev); > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > index 2664bb2..2cde0b4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > @@ -1633,12 +1633,20 @@ static int vcn_v1_0_set_powergating_state(void *handle, > * revisit this when there is a cleaner line between > * the smc and the hw blocks > */ > + int ret; > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > + if(state == adev->vcn.cur_state) > + return 0; > + > if (state == AMD_PG_STATE_GATE) > - return vcn_v1_0_stop(adev); > + ret = vcn_v1_0_stop(adev); > else > - return vcn_v1_0_start(adev); > + ret = vcn_v1_0_start(adev); > + > + if(!ret) > + adev->vcn.cur_state = state; > + return ret; > } > > static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > -- > 2.7.4 > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CADnq5_Nb-QPFox25ggaaOVO12P2_BUN7gmVWoJYpim1mYS9r3Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <CADnq5_Nb-QPFox25ggaaOVO12P2_BUN7gmVWoJYpim1mYS9r3Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2018-09-20 15:27 ` James Zhu [not found] ` <f7433b95-909a-8a6b-6659-2f9d04c108e5-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-20 15:27 UTC (permalink / raw) To: Alex Deucher, James Zhu; +Cc: James Zhu, amd-gfx list On 2018-09-20 11:14 AM, Alex Deucher wrote: > On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: >> When VCN PG state is unchanged, it is unnecessary to reset power >> gate state >> >> Signed-off-by: James Zhu <James.Zhu@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >> index 0b0b863..d2219ab 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >> struct amdgpu_ring ring_jpeg; >> struct amdgpu_irq_src irq; >> unsigned num_enc_rings; >> + enum amd_powergating_state cur_state; > Does the default value (0) at init time properly reflect the default > powergating state? If so, > Acked-by: Alex Deucher <alexander.deucher@amd.com> Yes, the below code shows it will be set to 0 during driver load stage. int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags) .... adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); struct amdgpu_device { .... struct amdgpu_vcn vcn; Best Regards! James zhu >> }; >> >> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); >> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >> index 2664bb2..2cde0b4 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >> @@ -1633,12 +1633,20 @@ static int vcn_v1_0_set_powergating_state(void *handle, >> * revisit this when there is a cleaner line between >> * the smc and the hw blocks >> */ >> + int ret; >> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >> >> + if(state == adev->vcn.cur_state) >> + return 0; >> + >> if (state == AMD_PG_STATE_GATE) >> - return vcn_v1_0_stop(adev); >> + ret = vcn_v1_0_stop(adev); >> else >> - return vcn_v1_0_start(adev); >> + ret = vcn_v1_0_start(adev); >> + >> + if(!ret) >> + adev->vcn.cur_state = state; >> + return ret; >> } >> >> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >> -- >> 2.7.4 >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <f7433b95-909a-8a6b-6659-2f9d04c108e5-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <f7433b95-909a-8a6b-6659-2f9d04c108e5-5C7GfCeVMHo@public.gmane.org> @ 2018-09-20 15:49 ` Alex Deucher [not found] ` <CADnq5_P2T2R+AyDquvEDWPN09iE1ZUMfMxoMSiJtuUWTKxkarQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Alex Deucher @ 2018-09-20 15:49 UTC (permalink / raw) To: James Zhu; +Cc: James Zhu, James Zhu, amd-gfx list On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> wrote: > > > > On 2018-09-20 11:14 AM, Alex Deucher wrote: > > On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: > >> When VCN PG state is unchanged, it is unnecessary to reset power > >> gate state > >> > >> Signed-off-by: James Zhu <James.Zhu@amd.com> > >> --- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + > >> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- > >> 2 files changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >> index 0b0b863..d2219ab 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >> @@ -69,6 +69,7 @@ struct amdgpu_vcn { > >> struct amdgpu_ring ring_jpeg; > >> struct amdgpu_irq_src irq; > >> unsigned num_enc_rings; > >> + enum amd_powergating_state cur_state; > > Does the default value (0) at init time properly reflect the default > > powergating state? If so, > > Acked-by: Alex Deucher <alexander.deucher@amd.com> > Yes, the below code shows it will be set to 0 during driver load stage. Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as the default though? The first time the code runs are we going to do the right thing or is the code going to return early? IIRC, the hw default is ungated. Alex > > int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags) > .... > adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); > > struct amdgpu_device { > .... > struct amdgpu_vcn vcn; > > Best Regards! > James zhu > >> }; > >> > >> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); > >> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >> index 2664bb2..2cde0b4 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >> @@ -1633,12 +1633,20 @@ static int vcn_v1_0_set_powergating_state(void *handle, > >> * revisit this when there is a cleaner line between > >> * the smc and the hw blocks > >> */ > >> + int ret; > >> struct amdgpu_device *adev = (struct amdgpu_device *)handle; > >> > >> + if(state == adev->vcn.cur_state) > >> + return 0; > >> + > >> if (state == AMD_PG_STATE_GATE) > >> - return vcn_v1_0_stop(adev); > >> + ret = vcn_v1_0_stop(adev); > >> else > >> - return vcn_v1_0_start(adev); > >> + ret = vcn_v1_0_start(adev); > >> + > >> + if(!ret) > >> + adev->vcn.cur_state = state; > >> + return ret; > >> } > >> > >> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CADnq5_P2T2R+AyDquvEDWPN09iE1ZUMfMxoMSiJtuUWTKxkarQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <CADnq5_P2T2R+AyDquvEDWPN09iE1ZUMfMxoMSiJtuUWTKxkarQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2018-09-20 16:24 ` James Zhu [not found] ` <f0c2d5b3-a273-406f-16c8-8e419ae26dd8-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-20 16:24 UTC (permalink / raw) To: Alex Deucher; +Cc: James Zhu, James Zhu, amd-gfx list On 2018-09-20 11:49 AM, Alex Deucher wrote: > On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> wrote: >> >> >> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: >>>> When VCN PG state is unchanged, it is unnecessary to reset power >>>> gate state >>>> >>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>> index 0b0b863..d2219ab 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>> struct amdgpu_ring ring_jpeg; >>>> struct amdgpu_irq_src irq; >>>> unsigned num_enc_rings; >>>> + enum amd_powergating_state cur_state; >>> Does the default value (0) at init time properly reflect the default >>> powergating state? If so, >>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >> Yes, the below code shows it will be set to 0 during driver load stage. > Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as the > default though? The first time the code runs are we going to do the > right thing or is the code going to return early? IIRC, the hw > default is ungated. cur_state is used for tracking driver SW PG state, not HW PG state. I though no matter what HW PG state is after device powers up, when first vcn ring is scheduled to run, begin_use->set_powergating_state->vcn_v1_0_start->ungate power/clock->Boot_VCPU will be tried. For DPG mode, the ungate power/clock , boot VCPU will not actually be activated during start setup stage, and only be activated during ring run stage. James > Alex >> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags) >> .... >> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >> >> struct amdgpu_device { >> .... >> struct amdgpu_vcn vcn; >> >> Best Regards! >> James zhu >>>> }; >>>> >>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>> index 2664bb2..2cde0b4 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>> @@ -1633,12 +1633,20 @@ static int vcn_v1_0_set_powergating_state(void *handle, >>>> * revisit this when there is a cleaner line between >>>> * the smc and the hw blocks >>>> */ >>>> + int ret; >>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle; >>>> >>>> + if(state == adev->vcn.cur_state) >>>> + return 0; >>>> + >>>> if (state == AMD_PG_STATE_GATE) >>>> - return vcn_v1_0_stop(adev); >>>> + ret = vcn_v1_0_stop(adev); >>>> else >>>> - return vcn_v1_0_start(adev); >>>> + ret = vcn_v1_0_start(adev); >>>> + >>>> + if(!ret) >>>> + adev->vcn.cur_state = state; >>>> + return ret; >>>> } >>>> >>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <f0c2d5b3-a273-406f-16c8-8e419ae26dd8-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <f0c2d5b3-a273-406f-16c8-8e419ae26dd8-5C7GfCeVMHo@public.gmane.org> @ 2018-09-20 17:54 ` Christian König [not found] ` <1d95a0b3-3b35-53f3-4ef3-0a0bbcc0456e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Christian König @ 2018-09-20 17:54 UTC (permalink / raw) To: James Zhu, Alex Deucher; +Cc: James Zhu, James Zhu, amd-gfx list Am 20.09.2018 um 18:24 schrieb James Zhu: > > > On 2018-09-20 11:49 AM, Alex Deucher wrote: >> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> wrote: >>> >>> >>> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: >>>>> When VCN PG state is unchanged, it is unnecessary to reset power >>>>> gate state >>>>> >>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>> index 0b0b863..d2219ab 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>>> struct amdgpu_ring ring_jpeg; >>>>> struct amdgpu_irq_src irq; >>>>> unsigned num_enc_rings; >>>>> + enum amd_powergating_state cur_state; >>>> Does the default value (0) at init time properly reflect the default >>>> powergating state? If so, >>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >>> Yes, the below code shows it will be set to 0 during driver load stage. >> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as the >> default though? The first time the code runs are we going to do the >> right thing or is the code going to return early? IIRC, the hw >> default is ungated. > cur_state is used for tracking driver SW PG state, not HW PG state. > I though no matter what HW PG state is after device powers up, when > first vcn ring is scheduled to run, > begin_use->set_powergating_state->vcn_v1_0_start->ungate > power/clock->Boot_VCPU will be tried. > > For DPG mode, the ungate power/clock , boot VCPU will not actually be > activated during start setup stage, and > only be activated during ring run stage. Mhm, I wonder if it wouldn't be better to have that functionality one layer up. E.g. in amdgpu_device_ip_set_powergating_state so that it applies to all IP blocks in the same way. But on the other hand the correct solution looks good to me as well, so feel free to add my Acked-by as well. Christian. > > James >> Alex >>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags) >>> .... >>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >>> >>> struct amdgpu_device { >>> .... >>> struct amdgpu_vcn vcn; >>> >>> Best Regards! >>> James zhu >>>>> }; >>>>> >>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> index 2664bb2..2cde0b4 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> @@ -1633,12 +1633,20 @@ static int >>>>> vcn_v1_0_set_powergating_state(void *handle, >>>>> * revisit this when there is a cleaner line between >>>>> * the smc and the hw blocks >>>>> */ >>>>> + int ret; >>>>> struct amdgpu_device *adev = (struct amdgpu_device >>>>> *)handle; >>>>> >>>>> + if(state == adev->vcn.cur_state) >>>>> + return 0; >>>>> + >>>>> if (state == AMD_PG_STATE_GATE) >>>>> - return vcn_v1_0_stop(adev); >>>>> + ret = vcn_v1_0_stop(adev); >>>>> else >>>>> - return vcn_v1_0_start(adev); >>>>> + ret = vcn_v1_0_start(adev); >>>>> + >>>>> + if(!ret) >>>>> + adev->vcn.cur_state = state; >>>>> + return ret; >>>>> } >>>>> >>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>> -- >>>>> 2.7.4 >>>>> >>>>> _______________________________________________ >>>>> amd-gfx mailing list >>>>> amd-gfx@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <1d95a0b3-3b35-53f3-4ef3-0a0bbcc0456e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <1d95a0b3-3b35-53f3-4ef3-0a0bbcc0456e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2018-09-21 12:28 ` James Zhu [not found] ` <89710c26-e399-5f1f-d27c-d038bf275a23-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-21 12:28 UTC (permalink / raw) To: christian.koenig-5C7GfCeVMHo, Alex Deucher Cc: James Zhu, James Zhu, amd-gfx list On 2018-09-20 01:54 PM, Christian König wrote: > Am 20.09.2018 um 18:24 schrieb James Zhu: >> >> >> On 2018-09-20 11:49 AM, Alex Deucher wrote: >>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> wrote: >>>> >>>> >>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: >>>>>> When VCN PG state is unchanged, it is unnecessary to reset power >>>>>> gate state >>>>>> >>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>> index 0b0b863..d2219ab 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>>>> struct amdgpu_ring ring_jpeg; >>>>>> struct amdgpu_irq_src irq; >>>>>> unsigned num_enc_rings; >>>>>> + enum amd_powergating_state cur_state; >>>>> Does the default value (0) at init time properly reflect the default >>>>> powergating state? If so, >>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >>>> Yes, the below code shows it will be set to 0 during driver load >>>> stage. >>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as the >>> default though? The first time the code runs are we going to do the >>> right thing or is the code going to return early? IIRC, the hw >>> default is ungated. >> cur_state is used for tracking driver SW PG state, not HW PG state. >> I though no matter what HW PG state is after device powers up, when >> first vcn ring is scheduled to run, >> begin_use->set_powergating_state->vcn_v1_0_start->ungate >> power/clock->Boot_VCPU will be tried. >> >> For DPG mode, the ungate power/clock , boot VCPU will not actually be >> activated during start setup stage, and >> only be activated during ring run stage. > > Mhm, I wonder if it wouldn't be better to have that functionality one > layer up. > > E.g. in amdgpu_device_ip_set_powergating_state so that it applies to > all IP blocks in the same way. If necessary, I can add support for IP blocks later. James > > But on the other hand the correct solution looks good to me as well, > so feel free to add my Acked-by as well. > > Christian. > >> >> James >>> Alex >>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long >>>> flags) >>>> .... >>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >>>> >>>> struct amdgpu_device { >>>> .... >>>> struct amdgpu_vcn vcn; >>>> >>>> Best Regards! >>>> James zhu >>>>>> }; >>>>>> >>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> index 2664bb2..2cde0b4 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> @@ -1633,12 +1633,20 @@ static int >>>>>> vcn_v1_0_set_powergating_state(void *handle, >>>>>> * revisit this when there is a cleaner line between >>>>>> * the smc and the hw blocks >>>>>> */ >>>>>> + int ret; >>>>>> struct amdgpu_device *adev = (struct amdgpu_device >>>>>> *)handle; >>>>>> >>>>>> + if(state == adev->vcn.cur_state) >>>>>> + return 0; >>>>>> + >>>>>> if (state == AMD_PG_STATE_GATE) >>>>>> - return vcn_v1_0_stop(adev); >>>>>> + ret = vcn_v1_0_stop(adev); >>>>>> else >>>>>> - return vcn_v1_0_start(adev); >>>>>> + ret = vcn_v1_0_start(adev); >>>>>> + >>>>>> + if(!ret) >>>>>> + adev->vcn.cur_state = state; >>>>>> + return ret; >>>>>> } >>>>>> >>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>>> -- >>>>>> 2.7.4 >>>>>> >>>>>> _______________________________________________ >>>>>> amd-gfx mailing list >>>>>> amd-gfx@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <89710c26-e399-5f1f-d27c-d038bf275a23-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <89710c26-e399-5f1f-d27c-d038bf275a23-5C7GfCeVMHo@public.gmane.org> @ 2018-09-21 12:52 ` Christian König [not found] ` <1faf5014-572d-0260-0f50-ab63aedc274e-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Christian König @ 2018-09-21 12:52 UTC (permalink / raw) To: James Zhu, Alex Deucher; +Cc: James Zhu, James Zhu, amd-gfx list Am 21.09.2018 um 14:28 schrieb James Zhu: > > > On 2018-09-20 01:54 PM, Christian König wrote: >> Am 20.09.2018 um 18:24 schrieb James Zhu: >>> >>> >>> On 2018-09-20 11:49 AM, Alex Deucher wrote: >>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> wrote: >>>>> >>>>> >>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> wrote: >>>>>>> When VCN PG state is unchanged, it is unnecessary to reset power >>>>>>> gate state >>>>>>> >>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>> index 0b0b863..d2219ab 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>>>>> struct amdgpu_ring ring_jpeg; >>>>>>> struct amdgpu_irq_src irq; >>>>>>> unsigned num_enc_rings; >>>>>>> + enum amd_powergating_state cur_state; >>>>>> Does the default value (0) at init time properly reflect the default >>>>>> powergating state? If so, >>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >>>>> Yes, the below code shows it will be set to 0 during driver load >>>>> stage. >>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as the >>>> default though? The first time the code runs are we going to do the >>>> right thing or is the code going to return early? IIRC, the hw >>>> default is ungated. >>> cur_state is used for tracking driver SW PG state, not HW PG state. >>> I though no matter what HW PG state is after device powers up, when >>> first vcn ring is scheduled to run, >>> begin_use->set_powergating_state->vcn_v1_0_start->ungate >>> power/clock->Boot_VCPU will be tried. >>> >>> For DPG mode, the ungate power/clock , boot VCPU will not actually >>> be activated during start setup stage, and >>> only be activated during ring run stage. >> >> Mhm, I wonder if it wouldn't be better to have that functionality one >> layer up. >> >> E.g. in amdgpu_device_ip_set_powergating_state so that it applies to >> all IP blocks in the same way. > If necessary, I can add support for IP blocks later. Yeah, agree. In general I'm not doing much with power management, so can't judge if that is a good idea or not. Anyway feel free to add my Acked-by to the patch. Regards, Christian. > James >> >> But on the other hand the correct solution looks good to me as well, >> so feel free to add my Acked-by as well. >> >> Christian. >> >>> >>> James >>>> Alex >>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long >>>>> flags) >>>>> .... >>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >>>>> >>>>> struct amdgpu_device { >>>>> .... >>>>> struct amdgpu_vcn vcn; >>>>> >>>>> Best Regards! >>>>> James zhu >>>>>>> }; >>>>>>> >>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>> index 2664bb2..2cde0b4 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>> @@ -1633,12 +1633,20 @@ static int >>>>>>> vcn_v1_0_set_powergating_state(void *handle, >>>>>>> * revisit this when there is a cleaner line between >>>>>>> * the smc and the hw blocks >>>>>>> */ >>>>>>> + int ret; >>>>>>> struct amdgpu_device *adev = (struct amdgpu_device >>>>>>> *)handle; >>>>>>> >>>>>>> + if(state == adev->vcn.cur_state) >>>>>>> + return 0; >>>>>>> + >>>>>>> if (state == AMD_PG_STATE_GATE) >>>>>>> - return vcn_v1_0_stop(adev); >>>>>>> + ret = vcn_v1_0_stop(adev); >>>>>>> else >>>>>>> - return vcn_v1_0_start(adev); >>>>>>> + ret = vcn_v1_0_start(adev); >>>>>>> + >>>>>>> + if(!ret) >>>>>>> + adev->vcn.cur_state = state; >>>>>>> + return ret; >>>>>>> } >>>>>>> >>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>>>> -- >>>>>>> 2.7.4 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> amd-gfx mailing list >>>>>>> amd-gfx@lists.freedesktop.org >>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <1faf5014-572d-0260-0f50-ab63aedc274e-5C7GfCeVMHo@public.gmane.org>]
* RE: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <1faf5014-572d-0260-0f50-ab63aedc274e-5C7GfCeVMHo@public.gmane.org> @ 2018-09-21 14:05 ` Zhu, Rex [not found] ` <BYAPR12MB277586C5BB434DF23EC5B3C0FB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Zhu, Rex @ 2018-09-21 14:05 UTC (permalink / raw) To: Koenig, Christian, Alex Deucher; +Cc: James Zhu, Zhu, James, amd-gfx list In my understand, when dpg mode enabled, we don't need to power up/down VCN via SMU and Also don't need to set vcn power gate/ungate state. Just need to enable the dpg mode. If not true, please correct me. Best Regards Rex > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of > Christian König > Sent: Friday, September 21, 2018 8:53 PM > To: Zhu, James <James.Zhu@amd.com>; Alex Deucher > <alexdeucher@gmail.com> > Cc: James Zhu <jzhums@gmail.com>; Zhu, James <James.Zhu@amd.com>; > amd-gfx list <amd-gfx@lists.freedesktop.org> > Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is > unchanged > > Am 21.09.2018 um 14:28 schrieb James Zhu: > > > > > > On 2018-09-20 01:54 PM, Christian König wrote: > >> Am 20.09.2018 um 18:24 schrieb James Zhu: > >>> > >>> > >>> On 2018-09-20 11:49 AM, Alex Deucher wrote: > >>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> > wrote: > >>>>> > >>>>> > >>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: > >>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> > wrote: > >>>>>>> When VCN PG state is unchanged, it is unnecessary to reset power > >>>>>>> gate state > >>>>>>> > >>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> > >>>>>>> --- > >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + > >>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- > >>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>> index 0b0b863..d2219ab 100644 > >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { > >>>>>>> struct amdgpu_ring ring_jpeg; > >>>>>>> struct amdgpu_irq_src irq; > >>>>>>> unsigned num_enc_rings; > >>>>>>> + enum amd_powergating_state cur_state; > >>>>>> Does the default value (0) at init time properly reflect the > >>>>>> default powergating state? If so, > >>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> > >>>>> Yes, the below code shows it will be set to 0 during driver load > >>>>> stage. > >>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as > >>>> the default though? The first time the code runs are we going to > >>>> do the right thing or is the code going to return early? IIRC, the > >>>> hw default is ungated. > >>> cur_state is used for tracking driver SW PG state, not HW PG state. > >>> I though no matter what HW PG state is after device powers up, when > >>> first vcn ring is scheduled to run, > >>> begin_use->set_powergating_state->vcn_v1_0_start->ungate > >>> power/clock->Boot_VCPU will be tried. > >>> > >>> For DPG mode, the ungate power/clock , boot VCPU will not actually > >>> be activated during start setup stage, and only be activated during > >>> ring run stage. > >> > >> Mhm, I wonder if it wouldn't be better to have that functionality one > >> layer up. > >> > >> E.g. in amdgpu_device_ip_set_powergating_state so that it applies to > >> all IP blocks in the same way. > > If necessary, I can add support for IP blocks later. > > Yeah, agree. In general I'm not doing much with power management, so > can't judge if that is a good idea or not. > > Anyway feel free to add my Acked-by to the patch. > > Regards, > Christian. > > > James > >> > >> But on the other hand the correct solution looks good to me as well, > >> so feel free to add my Acked-by as well. > >> > >> Christian. > >> > >>> > >>> James > >>>> Alex > >>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long > >>>>> flags) > >>>>> .... > >>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); > >>>>> > >>>>> struct amdgpu_device { > >>>>> .... > >>>>> struct amdgpu_vcn vcn; > >>>>> > >>>>> Best Regards! > >>>>> James zhu > >>>>>>> }; > >>>>>>> > >>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff > >>>>>>> --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>> index 2664bb2..2cde0b4 100644 > >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>> @@ -1633,12 +1633,20 @@ static int > >>>>>>> vcn_v1_0_set_powergating_state(void *handle, > >>>>>>> * revisit this when there is a cleaner line between > >>>>>>> * the smc and the hw blocks > >>>>>>> */ > >>>>>>> + int ret; > >>>>>>> struct amdgpu_device *adev = (struct amdgpu_device > >>>>>>> *)handle; > >>>>>>> > >>>>>>> + if(state == adev->vcn.cur_state) > >>>>>>> + return 0; > >>>>>>> + > >>>>>>> if (state == AMD_PG_STATE_GATE) > >>>>>>> - return vcn_v1_0_stop(adev); > >>>>>>> + ret = vcn_v1_0_stop(adev); > >>>>>>> else > >>>>>>> - return vcn_v1_0_start(adev); > >>>>>>> + ret = vcn_v1_0_start(adev); > >>>>>>> + > >>>>>>> + if(!ret) > >>>>>>> + adev->vcn.cur_state = state; > >>>>>>> + return ret; > >>>>>>> } > >>>>>>> > >>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > >>>>>>> -- > >>>>>>> 2.7.4 > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> amd-gfx mailing list > >>>>>>> amd-gfx@lists.freedesktop.org > >>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>> > >>> _______________________________________________ > >>> amd-gfx mailing list > >>> amd-gfx@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >> > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <BYAPR12MB277586C5BB434DF23EC5B3C0FB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <BYAPR12MB277586C5BB434DF23EC5B3C0FB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2018-09-21 14:22 ` James Zhu [not found] ` <d690bf76-e1d0-6edb-c11b-3db58eddd512-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-21 14:22 UTC (permalink / raw) To: Zhu, Rex, Koenig, Christian, Zhu, James, Alex Deucher Cc: James Zhu, amd-gfx list On 2018-09-21 10:05 AM, Zhu, Rex wrote: > In my understand, when dpg mode enabled, we don't need to power up/down VCN via SMU and > Also don't need to set vcn power gate/ungate state. > > Just need to enable the dpg mode. > > If not true, please correct me. You are right. Under DPG mode, vcn_v1_0_start is used to setup DPG mode instead of ungating power. For code reuse purpose, the function/variable name may have different connotation under different mode. James > > Best Regards > Rex > > > > > >> -----Original Message----- >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >> Christian König >> Sent: Friday, September 21, 2018 8:53 PM >> To: Zhu, James <James.Zhu@amd.com>; Alex Deucher >> <alexdeucher@gmail.com> >> Cc: James Zhu <jzhums@gmail.com>; Zhu, James <James.Zhu@amd.com>; >> amd-gfx list <amd-gfx@lists.freedesktop.org> >> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is >> unchanged >> >> Am 21.09.2018 um 14:28 schrieb James Zhu: >>> >>> On 2018-09-20 01:54 PM, Christian König wrote: >>>> Am 20.09.2018 um 18:24 schrieb James Zhu: >>>>> >>>>> On 2018-09-20 11:49 AM, Alex Deucher wrote: >>>>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> >> wrote: >>>>>>> >>>>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>>>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> >> wrote: >>>>>>>>> When VCN PG state is unchanged, it is unnecessary to reset power >>>>>>>>> gate state >>>>>>>>> >>>>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>> index 0b0b863..d2219ab 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>>>>>>> struct amdgpu_ring ring_jpeg; >>>>>>>>> struct amdgpu_irq_src irq; >>>>>>>>> unsigned num_enc_rings; >>>>>>>>> + enum amd_powergating_state cur_state; >>>>>>>> Does the default value (0) at init time properly reflect the >>>>>>>> default powergating state? If so, >>>>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >>>>>>> Yes, the below code shows it will be set to 0 during driver load >>>>>>> stage. >>>>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as >>>>>> the default though? The first time the code runs are we going to >>>>>> do the right thing or is the code going to return early? IIRC, the >>>>>> hw default is ungated. >>>>> cur_state is used for tracking driver SW PG state, not HW PG state. >>>>> I though no matter what HW PG state is after device powers up, when >>>>> first vcn ring is scheduled to run, >>>>> begin_use->set_powergating_state->vcn_v1_0_start->ungate >>>>> power/clock->Boot_VCPU will be tried. >>>>> >>>>> For DPG mode, the ungate power/clock , boot VCPU will not actually >>>>> be activated during start setup stage, and only be activated during >>>>> ring run stage. >>>> Mhm, I wonder if it wouldn't be better to have that functionality one >>>> layer up. >>>> >>>> E.g. in amdgpu_device_ip_set_powergating_state so that it applies to >>>> all IP blocks in the same way. >>> If necessary, I can add support for IP blocks later. >> Yeah, agree. In general I'm not doing much with power management, so >> can't judge if that is a good idea or not. >> >> Anyway feel free to add my Acked-by to the patch. >> >> Regards, >> Christian. >> >>> James >>>> But on the other hand the correct solution looks good to me as well, >>>> so feel free to add my Acked-by as well. >>>> >>>> Christian. >>>> >>>>> James >>>>>> Alex >>>>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long >>>>>>> flags) >>>>>>> .... >>>>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >>>>>>> >>>>>>> struct amdgpu_device { >>>>>>> .... >>>>>>> struct amdgpu_vcn vcn; >>>>>>> >>>>>>> Best Regards! >>>>>>> James zhu >>>>>>>>> }; >>>>>>>>> >>>>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff >>>>>>>>> --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>> index 2664bb2..2cde0b4 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>> @@ -1633,12 +1633,20 @@ static int >>>>>>>>> vcn_v1_0_set_powergating_state(void *handle, >>>>>>>>> * revisit this when there is a cleaner line between >>>>>>>>> * the smc and the hw blocks >>>>>>>>> */ >>>>>>>>> + int ret; >>>>>>>>> struct amdgpu_device *adev = (struct amdgpu_device >>>>>>>>> *)handle; >>>>>>>>> >>>>>>>>> + if(state == adev->vcn.cur_state) >>>>>>>>> + return 0; >>>>>>>>> + >>>>>>>>> if (state == AMD_PG_STATE_GATE) >>>>>>>>> - return vcn_v1_0_stop(adev); >>>>>>>>> + ret = vcn_v1_0_stop(adev); >>>>>>>>> else >>>>>>>>> - return vcn_v1_0_start(adev); >>>>>>>>> + ret = vcn_v1_0_start(adev); >>>>>>>>> + >>>>>>>>> + if(!ret) >>>>>>>>> + adev->vcn.cur_state = state; >>>>>>>>> + return ret; >>>>>>>>> } >>>>>>>>> >>>>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>>>>>> -- >>>>>>>>> 2.7.4 >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> amd-gfx mailing list >>>>>>>>> amd-gfx@lists.freedesktop.org >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>>> _______________________________________________ >>>>> amd-gfx mailing list >>>>> amd-gfx@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <d690bf76-e1d0-6edb-c11b-3db58eddd512-5C7GfCeVMHo@public.gmane.org>]
* RE: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <d690bf76-e1d0-6edb-c11b-3db58eddd512-5C7GfCeVMHo@public.gmane.org> @ 2018-09-21 14:48 ` Zhu, Rex [not found] ` <BYAPR12MB27752338D93DA89D250C53BAFB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Zhu, Rex @ 2018-09-21 14:48 UTC (permalink / raw) To: Zhu, James, Koenig, Christian; +Cc: James Zhu, amd-gfx list > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of > James Zhu > Sent: Friday, September 21, 2018 10:22 PM > To: Zhu, Rex <Rex.Zhu@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Zhu, James <James.Zhu@amd.com>; Alex > Deucher <alexdeucher@gmail.com> > Cc: James Zhu <jzhums@gmail.com>; amd-gfx list <amd- > gfx@lists.freedesktop.org> > Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is > unchanged > > > > On 2018-09-21 10:05 AM, Zhu, Rex wrote: > > In my understand, when dpg mode enabled, we don't need to power > > up/down VCN via SMU and Also don't need to set vcn power gate/ungate > state. > > > > Just need to enable the dpg mode. > > > > If not true, please correct me. > You are right. Under DPG mode, vcn_v1_0_start is used to setup DPG mode > instead of ungating power. > For code reuse purpose, the function/variable name may have different > connotation under different mode. Did your patch base on tip drm-next code? Rex > James > > > > Best Regards > > Rex > > > > > > > > > > > >> -----Original Message----- > >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of > >> Christian König > >> Sent: Friday, September 21, 2018 8:53 PM > >> To: Zhu, James <James.Zhu@amd.com>; Alex Deucher > >> <alexdeucher@gmail.com> > >> Cc: James Zhu <jzhums@gmail.com>; Zhu, James <James.Zhu@amd.com>; > >> amd-gfx list <amd-gfx@lists.freedesktop.org> > >> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is > >> unchanged > >> > >> Am 21.09.2018 um 14:28 schrieb James Zhu: > >>> > >>> On 2018-09-20 01:54 PM, Christian König wrote: > >>>> Am 20.09.2018 um 18:24 schrieb James Zhu: > >>>>> > >>>>> On 2018-09-20 11:49 AM, Alex Deucher wrote: > >>>>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> > >> wrote: > >>>>>>> > >>>>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: > >>>>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> > >> wrote: > >>>>>>>>> When VCN PG state is unchanged, it is unnecessary to reset > >>>>>>>>> power gate state > >>>>>>>>> > >>>>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> > >>>>>>>>> --- > >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + > >>>>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- > >>>>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>> index 0b0b863..d2219ab 100644 > >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { > >>>>>>>>> struct amdgpu_ring ring_jpeg; > >>>>>>>>> struct amdgpu_irq_src irq; > >>>>>>>>> unsigned num_enc_rings; > >>>>>>>>> + enum amd_powergating_state cur_state; > >>>>>>>> Does the default value (0) at init time properly reflect the > >>>>>>>> default powergating state? If so, > >>>>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> > >>>>>>> Yes, the below code shows it will be set to 0 during driver load > >>>>>>> stage. > >>>>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want > as > >>>>>> the default though? The first time the code runs are we going to > >>>>>> do the right thing or is the code going to return early? IIRC, > >>>>>> the hw default is ungated. > >>>>> cur_state is used for tracking driver SW PG state, not HW PG state. > >>>>> I though no matter what HW PG state is after device powers up, > >>>>> when first vcn ring is scheduled to run, > >>>>> begin_use->set_powergating_state->vcn_v1_0_start->ungate > >>>>> power/clock->Boot_VCPU will be tried. > >>>>> > >>>>> For DPG mode, the ungate power/clock , boot VCPU will not actually > >>>>> be activated during start setup stage, and only be activated > >>>>> during ring run stage. > >>>> Mhm, I wonder if it wouldn't be better to have that functionality > >>>> one layer up. > >>>> > >>>> E.g. in amdgpu_device_ip_set_powergating_state so that it applies > >>>> to all IP blocks in the same way. > >>> If necessary, I can add support for IP blocks later. > >> Yeah, agree. In general I'm not doing much with power management, so > >> can't judge if that is a good idea or not. > >> > >> Anyway feel free to add my Acked-by to the patch. > >> > >> Regards, > >> Christian. > >> > >>> James > >>>> But on the other hand the correct solution looks good to me as > >>>> well, so feel free to add my Acked-by as well. > >>>> > >>>> Christian. > >>>> > >>>>> James > >>>>>> Alex > >>>>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned > long > >>>>>>> flags) > >>>>>>> .... > >>>>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); > >>>>>>> > >>>>>>> struct amdgpu_device { > >>>>>>> .... > >>>>>>> struct amdgpu_vcn vcn; > >>>>>>> > >>>>>>> Best Regards! > >>>>>>> James zhu > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff > >>>>>>>>> --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>> index 2664bb2..2cde0b4 100644 > >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>> @@ -1633,12 +1633,20 @@ static int > >>>>>>>>> vcn_v1_0_set_powergating_state(void *handle, > >>>>>>>>> * revisit this when there is a cleaner line > >>>>>>>>> between > >>>>>>>>> * the smc and the hw blocks > >>>>>>>>> */ > >>>>>>>>> + int ret; > >>>>>>>>> struct amdgpu_device *adev = (struct amdgpu_device > >>>>>>>>> *)handle; > >>>>>>>>> > >>>>>>>>> + if(state == adev->vcn.cur_state) > >>>>>>>>> + return 0; > >>>>>>>>> + > >>>>>>>>> if (state == AMD_PG_STATE_GATE) > >>>>>>>>> - return vcn_v1_0_stop(adev); > >>>>>>>>> + ret = vcn_v1_0_stop(adev); > >>>>>>>>> else > >>>>>>>>> - return vcn_v1_0_start(adev); > >>>>>>>>> + ret = vcn_v1_0_start(adev); > >>>>>>>>> + > >>>>>>>>> + if(!ret) > >>>>>>>>> + adev->vcn.cur_state = state; > >>>>>>>>> + return ret; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > >>>>>>>>> -- > >>>>>>>>> 2.7.4 > >>>>>>>>> > >>>>>>>>> _______________________________________________ > >>>>>>>>> amd-gfx mailing list > >>>>>>>>> amd-gfx@lists.freedesktop.org > >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>>>> _______________________________________________ > >>>>> amd-gfx mailing list > >>>>> amd-gfx@lists.freedesktop.org > >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <BYAPR12MB27752338D93DA89D250C53BAFB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <BYAPR12MB27752338D93DA89D250C53BAFB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2018-09-21 14:51 ` James Zhu [not found] ` <de330d77-efb3-935d-7c7f-8ac99b86bf62-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: James Zhu @ 2018-09-21 14:51 UTC (permalink / raw) To: Zhu, Rex, Zhu, James, Koenig, Christian, Alex Deucher Cc: James Zhu, amd-gfx list On 2018-09-21 10:48 AM, Zhu, Rex wrote: > >> -----Original Message----- >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >> James Zhu >> Sent: Friday, September 21, 2018 10:22 PM >> To: Zhu, Rex <Rex.Zhu@amd.com>; Koenig, Christian >> <Christian.Koenig@amd.com>; Zhu, James <James.Zhu@amd.com>; Alex >> Deucher <alexdeucher@gmail.com> >> Cc: James Zhu <jzhums@gmail.com>; amd-gfx list <amd- >> gfx@lists.freedesktop.org> >> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is >> unchanged >> >> >> >> On 2018-09-21 10:05 AM, Zhu, Rex wrote: >>> In my understand, when dpg mode enabled, we don't need to power >>> up/down VCN via SMU and Also don't need to set vcn power gate/ungate >> state. >>> Just need to enable the dpg mode. >>> >>> If not true, please correct me. >> You are right. Under DPG mode, vcn_v1_0_start is used to setup DPG mode >> instead of ungating power. >> For code reuse purpose, the function/variable name may have different >> connotation under different mode. > Did your patch base on tip drm-next code? No based on amd-temp-pco. Since DPG mode is enabled only on PCO at this moment. James > > Rex > >> James >>> Best Regards >>> Rex >>> >>> >>> >>> >>> >>>> -----Original Message----- >>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >>>> Christian König >>>> Sent: Friday, September 21, 2018 8:53 PM >>>> To: Zhu, James <James.Zhu@amd.com>; Alex Deucher >>>> <alexdeucher@gmail.com> >>>> Cc: James Zhu <jzhums@gmail.com>; Zhu, James <James.Zhu@amd.com>; >>>> amd-gfx list <amd-gfx@lists.freedesktop.org> >>>> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is >>>> unchanged >>>> >>>> Am 21.09.2018 um 14:28 schrieb James Zhu: >>>>> On 2018-09-20 01:54 PM, Christian König wrote: >>>>>> Am 20.09.2018 um 18:24 schrieb James Zhu: >>>>>>> On 2018-09-20 11:49 AM, Alex Deucher wrote: >>>>>>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> >>>> wrote: >>>>>>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>>>>>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> >>>> wrote: >>>>>>>>>>> When VCN PG state is unchanged, it is unnecessary to reset >>>>>>>>>>> power gate state >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>>>>>>>>> --- >>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>>>>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>> index 0b0b863..d2219ab 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>>>>>>>>> struct amdgpu_ring ring_jpeg; >>>>>>>>>>> struct amdgpu_irq_src irq; >>>>>>>>>>> unsigned num_enc_rings; >>>>>>>>>>> + enum amd_powergating_state cur_state; >>>>>>>>>> Does the default value (0) at init time properly reflect the >>>>>>>>>> default powergating state? If so, >>>>>>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >>>>>>>>> Yes, the below code shows it will be set to 0 during driver load >>>>>>>>> stage. >>>>>>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want >> as >>>>>>>> the default though? The first time the code runs are we going to >>>>>>>> do the right thing or is the code going to return early? IIRC, >>>>>>>> the hw default is ungated. >>>>>>> cur_state is used for tracking driver SW PG state, not HW PG state. >>>>>>> I though no matter what HW PG state is after device powers up, >>>>>>> when first vcn ring is scheduled to run, >>>>>>> begin_use->set_powergating_state->vcn_v1_0_start->ungate >>>>>>> power/clock->Boot_VCPU will be tried. >>>>>>> >>>>>>> For DPG mode, the ungate power/clock , boot VCPU will not actually >>>>>>> be activated during start setup stage, and only be activated >>>>>>> during ring run stage. >>>>>> Mhm, I wonder if it wouldn't be better to have that functionality >>>>>> one layer up. >>>>>> >>>>>> E.g. in amdgpu_device_ip_set_powergating_state so that it applies >>>>>> to all IP blocks in the same way. >>>>> If necessary, I can add support for IP blocks later. >>>> Yeah, agree. In general I'm not doing much with power management, so >>>> can't judge if that is a good idea or not. >>>> >>>> Anyway feel free to add my Acked-by to the patch. >>>> >>>> Regards, >>>> Christian. >>>> >>>>> James >>>>>> But on the other hand the correct solution looks good to me as >>>>>> well, so feel free to add my Acked-by as well. >>>>>> >>>>>> Christian. >>>>>> >>>>>>> James >>>>>>>> Alex >>>>>>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned >> long >>>>>>>>> flags) >>>>>>>>> .... >>>>>>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >>>>>>>>> >>>>>>>>> struct amdgpu_device { >>>>>>>>> .... >>>>>>>>> struct amdgpu_vcn vcn; >>>>>>>>> >>>>>>>>> Best Regards! >>>>>>>>> James zhu >>>>>>>>>>> }; >>>>>>>>>>> >>>>>>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff >>>>>>>>>>> --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>> index 2664bb2..2cde0b4 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>> @@ -1633,12 +1633,20 @@ static int >>>>>>>>>>> vcn_v1_0_set_powergating_state(void *handle, >>>>>>>>>>> * revisit this when there is a cleaner line >>>>>>>>>>> between >>>>>>>>>>> * the smc and the hw blocks >>>>>>>>>>> */ >>>>>>>>>>> + int ret; >>>>>>>>>>> struct amdgpu_device *adev = (struct amdgpu_device >>>>>>>>>>> *)handle; >>>>>>>>>>> >>>>>>>>>>> + if(state == adev->vcn.cur_state) >>>>>>>>>>> + return 0; >>>>>>>>>>> + >>>>>>>>>>> if (state == AMD_PG_STATE_GATE) >>>>>>>>>>> - return vcn_v1_0_stop(adev); >>>>>>>>>>> + ret = vcn_v1_0_stop(adev); >>>>>>>>>>> else >>>>>>>>>>> - return vcn_v1_0_start(adev); >>>>>>>>>>> + ret = vcn_v1_0_start(adev); >>>>>>>>>>> + >>>>>>>>>>> + if(!ret) >>>>>>>>>>> + adev->vcn.cur_state = state; >>>>>>>>>>> + return ret; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>>>>>>>> -- >>>>>>>>>>> 2.7.4 >>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> amd-gfx mailing list >>>>>>>>>>> amd-gfx@lists.freedesktop.org >>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>>>>> _______________________________________________ >>>>>>> amd-gfx mailing list >>>>>>> amd-gfx@lists.freedesktop.org >>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <de330d77-efb3-935d-7c7f-8ac99b86bf62-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <de330d77-efb3-935d-7c7f-8ac99b86bf62-5C7GfCeVMHo@public.gmane.org> @ 2018-09-21 14:52 ` Alex Deucher [not found] ` <CADnq5_Ouq4_9BngfKEYH42anGL3oGKgf0791JQynGo_GA=8qaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Alex Deucher @ 2018-09-21 14:52 UTC (permalink / raw) To: James Zhu; +Cc: James Zhu, James Zhu, Rex Zhu, Christian Koenig, amd-gfx list On Fri, Sep 21, 2018 at 10:51 AM James Zhu <jamesz@amd.com> wrote: > > > > On 2018-09-21 10:48 AM, Zhu, Rex wrote: > > > >> -----Original Message----- > >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of > >> James Zhu > >> Sent: Friday, September 21, 2018 10:22 PM > >> To: Zhu, Rex <Rex.Zhu@amd.com>; Koenig, Christian > >> <Christian.Koenig@amd.com>; Zhu, James <James.Zhu@amd.com>; Alex > >> Deucher <alexdeucher@gmail.com> > >> Cc: James Zhu <jzhums@gmail.com>; amd-gfx list <amd- > >> gfx@lists.freedesktop.org> > >> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is > >> unchanged > >> > >> > >> > >> On 2018-09-21 10:05 AM, Zhu, Rex wrote: > >>> In my understand, when dpg mode enabled, we don't need to power > >>> up/down VCN via SMU and Also don't need to set vcn power gate/ungate > >> state. > >>> Just need to enable the dpg mode. > >>> > >>> If not true, please correct me. > >> You are right. Under DPG mode, vcn_v1_0_start is used to setup DPG mode > >> instead of ungating power. > >> For code reuse purpose, the function/variable name may have different > >> connotation under different mode. > > Did your patch base on tip drm-next code? > No based on amd-temp-pco. Since DPG mode is enabled only on PCO at this > moment. Picasso support has been merged into drm-next as of last week. Alex > James > > > > Rex > > > >> James > >>> Best Regards > >>> Rex > >>> > >>> > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of > >>>> Christian König > >>>> Sent: Friday, September 21, 2018 8:53 PM > >>>> To: Zhu, James <James.Zhu@amd.com>; Alex Deucher > >>>> <alexdeucher@gmail.com> > >>>> Cc: James Zhu <jzhums@gmail.com>; Zhu, James <James.Zhu@amd.com>; > >>>> amd-gfx list <amd-gfx@lists.freedesktop.org> > >>>> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is > >>>> unchanged > >>>> > >>>> Am 21.09.2018 um 14:28 schrieb James Zhu: > >>>>> On 2018-09-20 01:54 PM, Christian König wrote: > >>>>>> Am 20.09.2018 um 18:24 schrieb James Zhu: > >>>>>>> On 2018-09-20 11:49 AM, Alex Deucher wrote: > >>>>>>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> > >>>> wrote: > >>>>>>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: > >>>>>>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> > >>>> wrote: > >>>>>>>>>>> When VCN PG state is unchanged, it is unnecessary to reset > >>>>>>>>>>> power gate state > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> > >>>>>>>>>>> --- > >>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + > >>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- > >>>>>>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) > >>>>>>>>>>> > >>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>>>> index 0b0b863..d2219ab 100644 > >>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h > >>>>>>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { > >>>>>>>>>>> struct amdgpu_ring ring_jpeg; > >>>>>>>>>>> struct amdgpu_irq_src irq; > >>>>>>>>>>> unsigned num_enc_rings; > >>>>>>>>>>> + enum amd_powergating_state cur_state; > >>>>>>>>>> Does the default value (0) at init time properly reflect the > >>>>>>>>>> default powergating state? If so, > >>>>>>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> > >>>>>>>>> Yes, the below code shows it will be set to 0 during driver load > >>>>>>>>> stage. > >>>>>>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want > >> as > >>>>>>>> the default though? The first time the code runs are we going to > >>>>>>>> do the right thing or is the code going to return early? IIRC, > >>>>>>>> the hw default is ungated. > >>>>>>> cur_state is used for tracking driver SW PG state, not HW PG state. > >>>>>>> I though no matter what HW PG state is after device powers up, > >>>>>>> when first vcn ring is scheduled to run, > >>>>>>> begin_use->set_powergating_state->vcn_v1_0_start->ungate > >>>>>>> power/clock->Boot_VCPU will be tried. > >>>>>>> > >>>>>>> For DPG mode, the ungate power/clock , boot VCPU will not actually > >>>>>>> be activated during start setup stage, and only be activated > >>>>>>> during ring run stage. > >>>>>> Mhm, I wonder if it wouldn't be better to have that functionality > >>>>>> one layer up. > >>>>>> > >>>>>> E.g. in amdgpu_device_ip_set_powergating_state so that it applies > >>>>>> to all IP blocks in the same way. > >>>>> If necessary, I can add support for IP blocks later. > >>>> Yeah, agree. In general I'm not doing much with power management, so > >>>> can't judge if that is a good idea or not. > >>>> > >>>> Anyway feel free to add my Acked-by to the patch. > >>>> > >>>> Regards, > >>>> Christian. > >>>> > >>>>> James > >>>>>> But on the other hand the correct solution looks good to me as > >>>>>> well, so feel free to add my Acked-by as well. > >>>>>> > >>>>>> Christian. > >>>>>> > >>>>>>> James > >>>>>>>> Alex > >>>>>>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned > >> long > >>>>>>>>> flags) > >>>>>>>>> .... > >>>>>>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); > >>>>>>>>> > >>>>>>>>> struct amdgpu_device { > >>>>>>>>> .... > >>>>>>>>> struct amdgpu_vcn vcn; > >>>>>>>>> > >>>>>>>>> Best Regards! > >>>>>>>>> James zhu > >>>>>>>>>>> }; > >>>>>>>>>>> > >>>>>>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff > >>>>>>>>>>> --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>>>> index 2664bb2..2cde0b4 100644 > >>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > >>>>>>>>>>> @@ -1633,12 +1633,20 @@ static int > >>>>>>>>>>> vcn_v1_0_set_powergating_state(void *handle, > >>>>>>>>>>> * revisit this when there is a cleaner line > >>>>>>>>>>> between > >>>>>>>>>>> * the smc and the hw blocks > >>>>>>>>>>> */ > >>>>>>>>>>> + int ret; > >>>>>>>>>>> struct amdgpu_device *adev = (struct amdgpu_device > >>>>>>>>>>> *)handle; > >>>>>>>>>>> > >>>>>>>>>>> + if(state == adev->vcn.cur_state) > >>>>>>>>>>> + return 0; > >>>>>>>>>>> + > >>>>>>>>>>> if (state == AMD_PG_STATE_GATE) > >>>>>>>>>>> - return vcn_v1_0_stop(adev); > >>>>>>>>>>> + ret = vcn_v1_0_stop(adev); > >>>>>>>>>>> else > >>>>>>>>>>> - return vcn_v1_0_start(adev); > >>>>>>>>>>> + ret = vcn_v1_0_start(adev); > >>>>>>>>>>> + > >>>>>>>>>>> + if(!ret) > >>>>>>>>>>> + adev->vcn.cur_state = state; > >>>>>>>>>>> + return ret; > >>>>>>>>>>> } > >>>>>>>>>>> > >>>>>>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > >>>>>>>>>>> -- > >>>>>>>>>>> 2.7.4 > >>>>>>>>>>> > >>>>>>>>>>> _______________________________________________ > >>>>>>>>>>> amd-gfx mailing list > >>>>>>>>>>> amd-gfx@lists.freedesktop.org > >>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>>>>>> _______________________________________________ > >>>>>>> amd-gfx mailing list > >>>>>>> amd-gfx@lists.freedesktop.org > >>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>>> _______________________________________________ > >>>> amd-gfx mailing list > >>>> amd-gfx@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CADnq5_Ouq4_9BngfKEYH42anGL3oGKgf0791JQynGo_GA=8qaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged [not found] ` <CADnq5_Ouq4_9BngfKEYH42anGL3oGKgf0791JQynGo_GA=8qaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2018-09-21 14:56 ` James Zhu 0 siblings, 0 replies; 22+ messages in thread From: James Zhu @ 2018-09-21 14:56 UTC (permalink / raw) To: Alex Deucher Cc: James Zhu, James Zhu, Rex Zhu, Christian Koenig, amd-gfx list On 2018-09-21 10:52 AM, Alex Deucher wrote: > On Fri, Sep 21, 2018 at 10:51 AM James Zhu <jamesz@amd.com> wrote: >> >> >> On 2018-09-21 10:48 AM, Zhu, Rex wrote: >>>> -----Original Message----- >>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >>>> James Zhu >>>> Sent: Friday, September 21, 2018 10:22 PM >>>> To: Zhu, Rex <Rex.Zhu@amd.com>; Koenig, Christian >>>> <Christian.Koenig@amd.com>; Zhu, James <James.Zhu@amd.com>; Alex >>>> Deucher <alexdeucher@gmail.com> >>>> Cc: James Zhu <jzhums@gmail.com>; amd-gfx list <amd- >>>> gfx@lists.freedesktop.org> >>>> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is >>>> unchanged >>>> >>>> >>>> >>>> On 2018-09-21 10:05 AM, Zhu, Rex wrote: >>>>> In my understand, when dpg mode enabled, we don't need to power >>>>> up/down VCN via SMU and Also don't need to set vcn power gate/ungate >>>> state. >>>>> Just need to enable the dpg mode. >>>>> >>>>> If not true, please correct me. >>>> You are right. Under DPG mode, vcn_v1_0_start is used to setup DPG mode >>>> instead of ungating power. >>>> For code reuse purpose, the function/variable name may have different >>>> connotation under different mode. >>> Did your patch base on tip drm-next code? >> No based on amd-temp-pco. Since DPG mode is enabled only on PCO at this >> moment. > Picasso support has been merged into drm-next as of last week. I see, then I will rebase it to drm-next also. James. > > Alex > >> James >>> Rex >>> >>>> James >>>>> Best Regards >>>>> Rex >>>>> >>>>> >>>>> >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >>>>>> Christian König >>>>>> Sent: Friday, September 21, 2018 8:53 PM >>>>>> To: Zhu, James <James.Zhu@amd.com>; Alex Deucher >>>>>> <alexdeucher@gmail.com> >>>>>> Cc: James Zhu <jzhums@gmail.com>; Zhu, James <James.Zhu@amd.com>; >>>>>> amd-gfx list <amd-gfx@lists.freedesktop.org> >>>>>> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is >>>>>> unchanged >>>>>> >>>>>> Am 21.09.2018 um 14:28 schrieb James Zhu: >>>>>>> On 2018-09-20 01:54 PM, Christian König wrote: >>>>>>>> Am 20.09.2018 um 18:24 schrieb James Zhu: >>>>>>>>> On 2018-09-20 11:49 AM, Alex Deucher wrote: >>>>>>>>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz@amd.com> >>>>>> wrote: >>>>>>>>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote: >>>>>>>>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums@gmail.com> >>>>>> wrote: >>>>>>>>>>>>> When VCN PG state is unchanged, it is unnecessary to reset >>>>>>>>>>>>> power gate state >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com> >>>>>>>>>>>>> --- >>>>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 + >>>>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 12 ++++++++++-- >>>>>>>>>>>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>>>> index 0b0b863..d2219ab 100644 >>>>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h >>>>>>>>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn { >>>>>>>>>>>>> struct amdgpu_ring ring_jpeg; >>>>>>>>>>>>> struct amdgpu_irq_src irq; >>>>>>>>>>>>> unsigned num_enc_rings; >>>>>>>>>>>>> + enum amd_powergating_state cur_state; >>>>>>>>>>>> Does the default value (0) at init time properly reflect the >>>>>>>>>>>> default powergating state? If so, >>>>>>>>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com> >>>>>>>>>>> Yes, the below code shows it will be set to 0 during driver load >>>>>>>>>>> stage. >>>>>>>>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want >>>> as >>>>>>>>>> the default though? The first time the code runs are we going to >>>>>>>>>> do the right thing or is the code going to return early? IIRC, >>>>>>>>>> the hw default is ungated. >>>>>>>>> cur_state is used for tracking driver SW PG state, not HW PG state. >>>>>>>>> I though no matter what HW PG state is after device powers up, >>>>>>>>> when first vcn ring is scheduled to run, >>>>>>>>> begin_use->set_powergating_state->vcn_v1_0_start->ungate >>>>>>>>> power/clock->Boot_VCPU will be tried. >>>>>>>>> >>>>>>>>> For DPG mode, the ungate power/clock , boot VCPU will not actually >>>>>>>>> be activated during start setup stage, and only be activated >>>>>>>>> during ring run stage. >>>>>>>> Mhm, I wonder if it wouldn't be better to have that functionality >>>>>>>> one layer up. >>>>>>>> >>>>>>>> E.g. in amdgpu_device_ip_set_powergating_state so that it applies >>>>>>>> to all IP blocks in the same way. >>>>>>> If necessary, I can add support for IP blocks later. >>>>>> Yeah, agree. In general I'm not doing much with power management, so >>>>>> can't judge if that is a good idea or not. >>>>>> >>>>>> Anyway feel free to add my Acked-by to the patch. >>>>>> >>>>>> Regards, >>>>>> Christian. >>>>>> >>>>>>> James >>>>>>>> But on the other hand the correct solution looks good to me as >>>>>>>> well, so feel free to add my Acked-by as well. >>>>>>>> >>>>>>>> Christian. >>>>>>>> >>>>>>>>> James >>>>>>>>>> Alex >>>>>>>>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned >>>> long >>>>>>>>>>> flags) >>>>>>>>>>> .... >>>>>>>>>>> adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); >>>>>>>>>>> >>>>>>>>>>> struct amdgpu_device { >>>>>>>>>>> .... >>>>>>>>>>> struct amdgpu_vcn vcn; >>>>>>>>>>> >>>>>>>>>>> Best Regards! >>>>>>>>>>> James zhu >>>>>>>>>>>>> }; >>>>>>>>>>>>> >>>>>>>>>>>>> int amdgpu_vcn_sw_init(struct amdgpu_device *adev); diff >>>>>>>>>>>>> --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>>>> index 2664bb2..2cde0b4 100644 >>>>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>>>>>>>>> @@ -1633,12 +1633,20 @@ static int >>>>>>>>>>>>> vcn_v1_0_set_powergating_state(void *handle, >>>>>>>>>>>>> * revisit this when there is a cleaner line >>>>>>>>>>>>> between >>>>>>>>>>>>> * the smc and the hw blocks >>>>>>>>>>>>> */ >>>>>>>>>>>>> + int ret; >>>>>>>>>>>>> struct amdgpu_device *adev = (struct amdgpu_device >>>>>>>>>>>>> *)handle; >>>>>>>>>>>>> >>>>>>>>>>>>> + if(state == adev->vcn.cur_state) >>>>>>>>>>>>> + return 0; >>>>>>>>>>>>> + >>>>>>>>>>>>> if (state == AMD_PG_STATE_GATE) >>>>>>>>>>>>> - return vcn_v1_0_stop(adev); >>>>>>>>>>>>> + ret = vcn_v1_0_stop(adev); >>>>>>>>>>>>> else >>>>>>>>>>>>> - return vcn_v1_0_start(adev); >>>>>>>>>>>>> + ret = vcn_v1_0_start(adev); >>>>>>>>>>>>> + >>>>>>>>>>>>> + if(!ret) >>>>>>>>>>>>> + adev->vcn.cur_state = state; >>>>>>>>>>>>> + return ret; >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>>>>>>>>>> -- >>>>>>>>>>>>> 2.7.4 >>>>>>>>>>>>> >>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>> amd-gfx mailing list >>>>>>>>>>>>> amd-gfx@lists.freedesktop.org >>>>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>>>>>>> _______________________________________________ >>>>>>>>> amd-gfx mailing list >>>>>>>>> amd-gfx@lists.freedesktop.org >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>>>> _______________________________________________ >>>>>> amd-gfx mailing list >>>>>> amd-gfx@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-09-21 14:56 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-10 16:49 [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged James Zhu
[not found] ` <1536598142-19689-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org>
2018-09-11 14:44 ` Alex Deucher
[not found] ` <CADnq5_P5Oj_Fdxhv2n8ZxDTm1mV_NQo6M1kmgGzLV0fHhhR8sA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-09-11 15:06 ` James Zhu
[not found] ` <d358b2be-26d1-627a-7105-5d90685d0f3b-5C7GfCeVMHo@public.gmane.org>
2018-09-11 15:17 ` Christian König
[not found] ` <68b30f71-86eb-89a0-0f8d-fcfb18e89cc2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-09-11 15:29 ` James Zhu
[not found] ` <62baa235-e113-3635-6060-f5ffcc14e967-5C7GfCeVMHo@public.gmane.org>
2018-09-11 15:36 ` Christian König
[not found] ` <cbd39fc8-bd5e-cd11-b9f2-4b91185429f7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-09-11 15:44 ` James Zhu
2018-09-13 20:55 ` [PATCH v2] drm/amdgpu:No action " James Zhu
[not found] ` <1536872144-18347-1-git-send-email-James.Zhu-5C7GfCeVMHo@public.gmane.org>
2018-09-20 14:19 ` Zhu, James
2018-09-20 15:14 ` Alex Deucher
[not found] ` <CADnq5_Nb-QPFox25ggaaOVO12P2_BUN7gmVWoJYpim1mYS9r3Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-09-20 15:27 ` James Zhu
[not found] ` <f7433b95-909a-8a6b-6659-2f9d04c108e5-5C7GfCeVMHo@public.gmane.org>
2018-09-20 15:49 ` Alex Deucher
[not found] ` <CADnq5_P2T2R+AyDquvEDWPN09iE1ZUMfMxoMSiJtuUWTKxkarQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-09-20 16:24 ` James Zhu
[not found] ` <f0c2d5b3-a273-406f-16c8-8e419ae26dd8-5C7GfCeVMHo@public.gmane.org>
2018-09-20 17:54 ` Christian König
[not found] ` <1d95a0b3-3b35-53f3-4ef3-0a0bbcc0456e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-09-21 12:28 ` James Zhu
[not found] ` <89710c26-e399-5f1f-d27c-d038bf275a23-5C7GfCeVMHo@public.gmane.org>
2018-09-21 12:52 ` Christian König
[not found] ` <1faf5014-572d-0260-0f50-ab63aedc274e-5C7GfCeVMHo@public.gmane.org>
2018-09-21 14:05 ` Zhu, Rex
[not found] ` <BYAPR12MB277586C5BB434DF23EC5B3C0FB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2018-09-21 14:22 ` James Zhu
[not found] ` <d690bf76-e1d0-6edb-c11b-3db58eddd512-5C7GfCeVMHo@public.gmane.org>
2018-09-21 14:48 ` Zhu, Rex
[not found] ` <BYAPR12MB27752338D93DA89D250C53BAFB120-ZGDeBxoHBPmJeBUhB162ZQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2018-09-21 14:51 ` James Zhu
[not found] ` <de330d77-efb3-935d-7c7f-8ac99b86bf62-5C7GfCeVMHo@public.gmane.org>
2018-09-21 14:52 ` Alex Deucher
[not found] ` <CADnq5_Ouq4_9BngfKEYH42anGL3oGKgf0791JQynGo_GA=8qaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-09-21 14:56 ` James Zhu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.