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

Submitted by Christian König on Sept. 11, 2018, 4:14 p.m.

Details

Message ID 82dc897f-8405-41b2-9482-43d567ec4fcc@email.android.com
State New
Headers show
Series "drm/amdgpu:No action needs when VCN PG state is unchanged" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Christian König Sept. 11, 2018, 4:14 p.m.
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(-)

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

Patch hide | download patch | download mbox

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

Comments

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