[PATCH 3/6] drm/amd/pp: Lock powerplay when reset pp table

[Alex Deucher]

On Thu, Mar 22, 2018 at 9:44 AM, Zhu, Rex <Rex.Zhu at amd.com> wrote:

> Please see inline.
>
> Best Regards
> Rex
> ------------------------------
> *From:* Alex Deucher <alexdeucher at gmail.com>
> *Sent:* Thursday, March 22, 2018 9:17 PM
> *To:* Zhu, Rex
> *Cc:* amd-gfx list
> *Subject:* Re: [PATCH 3/6] drm/amd/pp: Lock powerplay when reset pp table
>
> On Thu, Mar 22, 2018 at 7:40 AM, Rex Zhu <Rex.Zhu at amd.com> wrote:
> > Change-Id: Ib02cd71593bee9606822a466a56f2d01ac2e04cc
> > Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
>
> Please include a commit description.  It's not clear to me what's
> happening here.
>
> Alex
>
> > ---
> >  drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 28
> +++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > index 5d7d9ec..1ae4905 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > @@ -31,8 +31,6 @@
> >  #include "amdgpu.h"
> >  #include "hwmgr.h"
> >
> > -static int pp_dpm_dispatch_tasks(void *handle, enum amd_pp_task task_id,
> > -               enum amd_pm_state_type *user_state);
> >
> >  static const struct amd_pm_funcs pp_dpm_funcs;
> >
> > @@ -146,10 +144,12 @@ static int pp_late_init(void *handle)
> >         struct amdgpu_device *adev = handle;
> >         struct pp_hwmgr *hwmgr = adev->powerplay.pp_handle;
> >
> > -       if (hwmgr && hwmgr->pm_en)
> > -               pp_dpm_dispatch_tasks(hwmgr,
> > +       if (hwmgr && hwmgr->pm_en) {
> > +               mutex_lock(&hwmgr->smu_lock);
> > +               hwmgr_handle_task(hwmgr,
> >                                         AMD_PP_TASK_COMPLETE_INIT,
> NULL);
> > -
> > +               mutex_unlock(&hwmgr->smu_lock);
> > +       }
> >         return 0;
> >  }
>
> Rex : use hwmgr_handle_task instand of  pp_dpm_dispatch_tasks. because i
> think it is not make sense that call
> pp_function in ip_function.
>
> > @@ -620,7 +620,7 @@ static int amd_powerplay_reset(void *handle)
> >  static int pp_dpm_set_pp_table(void *handle, const char *buf, size_t
> size)
> >  {
> >         struct pp_hwmgr *hwmgr = handle;
> > -       int ret = 0;
> > +       int ret = -ENOMEM;
> >
> >         if (!hwmgr || !hwmgr->pm_en)
> >                 return -EINVAL;
> > @@ -630,28 +630,28 @@ static int pp_dpm_set_pp_table(void *handle, const
> char *buf, size_t size)
> >                 hwmgr->hardcode_pp_table = kmemdup(hwmgr->soft_pp_table,
> >
> hwmgr->soft_pp_table_size,
> >                                                    GFP_KERNEL);
> > -               if (!hwmgr->hardcode_pp_table) {
> > -                       mutex_unlock(&hwmgr->smu_lock);
> > -                       return -ENOMEM;
> > -               }
> > +               if (!hwmgr->hardcode_pp_table)
> > +                       goto err;
> >         }
> >
> >         memcpy(hwmgr->hardcode_pp_table, buf, size);
> >
> >         hwmgr->soft_pp_table = hwmgr->hardcode_pp_table;
> > -       mutex_unlock(&hwmgr->smu_lock);
> >
> >         ret = amd_powerplay_reset(handle);
> >         if (ret)
> > -               return ret;
> > +               goto err;
> >
> >         if (hwmgr->hwmgr_func->avfs_control) {
> >                 ret = hwmgr->hwmgr_func->avfs_control(hwmgr, false);
> >                 if (ret)
> > -                       return ret;
> > +                       goto err;
> >         }
> > -
> > +       mutex_unlock(&hwmgr->smu_lock);
> >         return 0;
> > +err:
> > +       mutex_unlock(&hwmgr->smu_lock);
> > +       return ret;
> >  }
>
>
> Rex: unlock mutex until  set pp table complete to avoid other pp functions
> were called simultaneously
> <http://www.baidu.com/link?url=8RpXVOm53HQyZI9h-ZIs8Ipio9NEDj98vxJiiM0Wx4hbUUpanLuj7jZHEnm0Javd7_rpySIGVAwaxX_ICV-y2nDz_TZqF8mzUGZT8RPrw2WCcNsGFsoAh9nirArCSm9s>
> .
>

Thanks.  Please include these comments in the patch description.  With that
fixed:
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>


>
>
> >  static int pp_dpm_force_clock_level(void *handle,
> > --
> > 1.9.1
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx at lists.freedesktop.org.
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> amd-gfx Info Page - freedesktop.org
> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
> lists.freedesktop.org
> Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the following
> form. Use of all freedesktop.org lists is subject to our Code of ...
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180322/6a788e39/attachment-0001.html>