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

[James Zhu]

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 at 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 at gmail.com> <mailto:jzhums at gmail.com>  wrote:
>
>                             Signed-off-by: James Zhu<James.Zhu at amd.com> <mailto:James.Zhu at 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 at lists.freedesktop.org
>                             <mailto:amd-gfx at lists.freedesktop.org>
>                             https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
>                     _______________________________________________
>                     amd-gfx mailing list
>                     amd-gfx at lists.freedesktop.org
>                     <mailto:amd-gfx at lists.freedesktop.org>
>                     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
>
>             _______________________________________________
>             amd-gfx mailing list
>             amd-gfx at lists.freedesktop.org
>             <mailto:amd-gfx at lists.freedesktop.org>
>             https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180911/eccdb540/attachment.html>