[PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[James Zhu]

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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[Alex Deucher]

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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[James Zhu]

[-- 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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[Christian König]

[-- 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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[James Zhu]

[-- 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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[Christian König]

[-- 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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[James Zhu]

[-- 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

[PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[James Zhu]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Zhu, James]

[-- 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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Alex Deucher]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[James Zhu]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Alex Deucher]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[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.

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Christian König]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[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.
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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Christian König]

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

RE: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Zhu, Rex]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[James Zhu]

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

RE: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Zhu, Rex]

> -----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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[James Zhu]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Alex Deucher]

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

Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[James Zhu]

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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[Alex Deucher]

On Tue, Sep 11, 2018 at 12:20 PM James Zhu <jamesz@amd.com> wrote:
>
> Hi Christian,
>
> Thanks!
>
> I add this check for VCN DPG/DPG PAUSE mode implementation.
>
> Do you think it is necessary to add for other IP blocks if they need or just for VCN only?

Well, it will may save some unnecessary programming in some cases, but
I think we track it pretty well already for manual PG.

Alex

>
> Best Regards!
>
> James Zhu
>
>
> On 2018-09-11 12:14 PM, Koenig, Christian wrote:
>
> Hi James,
>
> Am 11.09.2018 17:44 schrieb "Zhu, James" <James.Zhu@amd.com>:
>
>
>
> 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@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
>
> 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?
>
>
> No that is not correct.
>
> Static variables both on function as well as module level are shared between all amdgpu devices.
>
> Regards,
> Christian.
>
>
> 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@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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[James Zhu]

[-- Attachment #1.1: Type: text/plain, Size: 6227 bytes --]

Hi Christian,

Thanks!

I add this check for VCN DPG/DPG PAUSE mode implementation.

Do you think it is necessary to add for other IP blocks if they need or 
just for VCN only?

Best Regards!

James Zhu


On 2018-09-11 12:14 PM, Koenig, Christian wrote:
> Hi James,
>
> Am 11.09.2018 17:44 schrieb "Zhu, James" <James.Zhu-5C7GfCeVMHo@public.gmane.org>:
>
>
>
>     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> <mailto:jzhums-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>
>                             Signed-off-by: James Zhu<James.Zhu-5C7GfCeVMHo@public.gmane.org> <mailto: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?
>
>
> No that is not correct.
>
> Static variables both on function as well as module level are shared 
> between all amdgpu devices.
>
> Regards,
> Christian.
>
>
>     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
>                             <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>                             https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
>                     _______________________________________________
>                     amd-gfx mailing list
>                     amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>                     <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>                     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
>
>             _______________________________________________
>             amd-gfx mailing list
>             amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>             <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>             https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>


[-- Attachment #1.2: Type: text/html, Size: 9420 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

Re: [PATCH] drm/amdgpu:No action needs when VCN PG state is unchanged

[Koenig, Christian]

[-- Attachment #1.1: Type: text/plain, Size: 3701 bytes --]

Hi James,

Am 11.09.2018 17:44 schrieb "Zhu, James" <James.Zhu@amd.com>:


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@gmail.com><mailto:jzhums@gmail.com> wrote:


Signed-off-by: James Zhu <James.Zhu@amd.com><mailto: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



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?

No that is not correct.

Static variables both on function as well as module level are shared between all amdgpu devices.

Regards,
Christian.


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@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx





_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx






_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx





[-- Attachment #1.2: Type: text/html, Size: 5649 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