Re: [PATCH 1/2] drm/amdgpu/UAPI: add new CTX OP to get/set stable pstates

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 16, 2021 at 9:24 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
>
>
> Am 16.12.21 um 15:14 schrieb Alex Deucher:
> > Add a new CTX ioctl operation to set stable pstates for profiling.
> > When creating traces for tools like RGP or using SPM or doing
> > performance profiling, it's required to enable a special
> > stable profiling power state on the GPU.  These profiling
> > states set fixed clocks and disable certain other power
> > features like powergating which may impact the results.
> >
> > Historically, these profiling pstates were enabled via sysfs,
> > but this adds an interface to enable it via the CTX ioctl
> > from the application.  Since the power state is global
> > only one application can set it at a time, so if multiple
> > applications try and use it only the first will get it,
> > the ioctl will return -EBUSY for others.  The sysfs interface
> > will override whatever has been set by this interface.
> >
> > Mesa MR: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/207
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c    | 145 ++++++++++++++++++++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h    |   1 +
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |   1 +
> >   drivers/gpu/drm/amd/pm/amdgpu_pm.c         |   7 +
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |   3 +
> >   include/uapi/drm/amdgpu_drm.h              |  17 ++-
> >   6 files changed, 171 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > index 468003583b2a..327cf308c2ab 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > @@ -237,6 +237,7 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev,
> >       ctx->vram_lost_counter = atomic_read(&adev->vram_lost_counter);
> >       ctx->init_priority = priority;
> >       ctx->override_priority = AMDGPU_CTX_PRIORITY_UNSET;
> > +     ctx->stable_pstate = AMDGPU_CTX_STABLE_PSTATE_NONE;
> >
> >       return 0;
> >   }
> > @@ -255,6 +256,102 @@ static void amdgpu_ctx_fini_entity(struct amdgpu_ctx_entity *entity)
> >       kfree(entity);
> >   }
> >
> > +static int amdgpu_ctx_get_stable_pstate(struct amdgpu_ctx *ctx,
> > +                                     u32 *stable_pstate)
> > +{
> > +     struct amdgpu_device *adev = ctx->adev;
> > +     const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> > +     enum amd_dpm_forced_level current_level;
> > +
> > +     if (!ctx)
> > +             return -EINVAL;
> > +
> > +     if (pp_funcs->get_performance_level)
> > +             current_level = amdgpu_dpm_get_performance_level(adev);
> > +     else
> > +             current_level = adev->pm.dpm.forced_level;
> > +
> > +     switch (current_level) {
> > +     case AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD:
> > +             *stable_pstate = AMDGPU_CTX_STABLE_PSTATE_STANDARD;
> > +             break;
> > +     case AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK:
> > +             *stable_pstate = AMDGPU_CTX_STABLE_PSTATE_MIN_SCLK;
> > +             break;
> > +     case AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK:
> > +             *stable_pstate = AMDGPU_CTX_STABLE_PSTATE_MIN_MCLK;
> > +             break;
> > +     case AMD_DPM_FORCED_LEVEL_PROFILE_PEAK:
> > +             *stable_pstate = AMDGPU_CTX_STABLE_PSTATE_PEAK;
> > +             break;
> > +     default:
> > +             *stable_pstate = AMDGPU_CTX_STABLE_PSTATE_NONE;
> > +             break;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int amdgpu_ctx_set_stable_pstate(struct amdgpu_ctx *ctx,
> > +                                     u32 stable_pstate)
> > +{
> > +     struct amdgpu_device *adev = ctx->adev;
> > +     const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> > +     enum amd_dpm_forced_level level, current_level;
> > +     int r = 0;
>
> I would avoid initializing the return value and rather set it below
> after everything worked out.

Will fix.

>
> > +
> > +     if (!ctx)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&adev->pm.stable_pstate_ctx_lock);
> > +     if (adev->pm.stable_pstate_ctx && adev->pm.stable_pstate_ctx != ctx) {
> > +             r = -EBUSY;
> > +             goto done;
> > +     }
> > +
> > +     switch (stable_pstate) {
> > +     case AMDGPU_CTX_STABLE_PSTATE_NONE:
> > +             level = AMD_DPM_FORCED_LEVEL_AUTO;
> > +             break;
> > +     case AMDGPU_CTX_STABLE_PSTATE_STANDARD:
> > +             level = AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD;
> > +             break;
> > +     case AMDGPU_CTX_STABLE_PSTATE_MIN_SCLK:
> > +             level = AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK;
> > +             break;
> > +     case AMDGPU_CTX_STABLE_PSTATE_MIN_MCLK:
> > +             level = AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK;
> > +             break;
> > +     case AMDGPU_CTX_STABLE_PSTATE_PEAK:
> > +             level = AMD_DPM_FORCED_LEVEL_PROFILE_PEAK;
> > +             break;
> > +     default:
> > +             r = -EINVAL;
> > +             goto done;
> > +     }
> > +
> > +     if (pp_funcs->get_performance_level)
> > +             current_level = amdgpu_dpm_get_performance_level(adev);
> > +     else
> > +             current_level = adev->pm.dpm.forced_level;
>
> Mhm, that looks strongly like it doesn't belong into the ctx code.
>
> Didn't Evan had some patches to clean that up?

yeah, will rework this once Evan's patches land.

Alex

>
> Apart from those two nit picks looks good to me,
> Christian.
>
> > +
> > +     if ((current_level != level) && pp_funcs->force_performance_level) {
> > +             mutex_lock(&adev->pm.mutex);
> > +             r = amdgpu_dpm_force_performance_level(adev, level);
> > +             if (!r)
> > +                     adev->pm.dpm.forced_level = level;
> > +             mutex_unlock(&adev->pm.mutex);
> > +     }
> > +
> > +     if (level == AMD_DPM_FORCED_LEVEL_AUTO)
> > +             adev->pm.stable_pstate_ctx = NULL;
> > +     else
> > +             adev->pm.stable_pstate_ctx = ctx;
> > +done:
> > +     mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
> > +
> > +     return r;
> > +}
> > +
> >   static void amdgpu_ctx_fini(struct kref *ref)
> >   {
> >       struct amdgpu_ctx *ctx = container_of(ref, struct amdgpu_ctx, refcount);
> > @@ -270,7 +367,7 @@ static void amdgpu_ctx_fini(struct kref *ref)
> >                       ctx->entities[i][j] = NULL;
> >               }
> >       }
> > -
> > +     amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);
> >       mutex_destroy(&ctx->lock);
> >       kfree(ctx);
> >   }
> > @@ -467,11 +564,41 @@ static int amdgpu_ctx_query2(struct amdgpu_device *adev,
> >       return 0;
> >   }
> >
> > +
> > +
> > +static int amdgpu_ctx_stable_pstate(struct amdgpu_device *adev,
> > +                                 struct amdgpu_fpriv *fpriv, uint32_t id,
> > +                                 bool set, u32 *stable_pstate)
> > +{
> > +     struct amdgpu_ctx *ctx;
> > +     struct amdgpu_ctx_mgr *mgr;
> > +     int r;
> > +
> > +     if (!fpriv)
> > +             return -EINVAL;
> > +
> > +     mgr = &fpriv->ctx_mgr;
> > +     mutex_lock(&mgr->lock);
> > +     ctx = idr_find(&mgr->ctx_handles, id);
> > +     if (!ctx) {
> > +             mutex_unlock(&mgr->lock);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (set)
> > +             r = amdgpu_ctx_set_stable_pstate(ctx, *stable_pstate);
> > +     else
> > +             r = amdgpu_ctx_get_stable_pstate(ctx, stable_pstate);
> > +
> > +     mutex_unlock(&mgr->lock);
> > +     return r;
> > +}
> > +
> >   int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
> >                    struct drm_file *filp)
> >   {
> >       int r;
> > -     uint32_t id;
> > +     uint32_t id, stable_pstate;
> >       int32_t priority;
> >
> >       union drm_amdgpu_ctx *args = data;
> > @@ -500,6 +627,20 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
> >       case AMDGPU_CTX_OP_QUERY_STATE2:
> >               r = amdgpu_ctx_query2(adev, fpriv, id, &args->out);
> >               break;
> > +     case AMDGPU_CTX_OP_GET_STABLE_PSTATE:
> > +             if (args->in.flags)
> > +                     return -EINVAL;
> > +             r = amdgpu_ctx_stable_pstate(adev, fpriv, id, false, &stable_pstate);
> > +             args->out.pstate.flags = stable_pstate;
> > +             break;
> > +     case AMDGPU_CTX_OP_SET_STABLE_PSTATE:
> > +             if (args->in.flags & ~AMDGPU_CTX_STABLE_PSTATE_FLAGS_MASK)
> > +                     return -EINVAL;
> > +             stable_pstate = args->in.flags & AMDGPU_CTX_STABLE_PSTATE_FLAGS_MASK;
> > +             if (stable_pstate > AMDGPU_CTX_STABLE_PSTATE_PEAK)
> > +                     return -EINVAL;
> > +             r = amdgpu_ctx_stable_pstate(adev, fpriv, id, true, &stable_pstate);
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> > index a44b8b8ed39c..142f2f87d44c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> > @@ -53,6 +53,7 @@ struct amdgpu_ctx {
> >       atomic_t                        guilty;
> >       unsigned long                   ras_counter_ce;
> >       unsigned long                   ras_counter_ue;
> > +     uint32_t                        stable_pstate;
> >   };
> >
> >   struct amdgpu_ctx_mgr {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index a24b4c374188..e0fc943e9f9b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3486,6 +3486,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >       init_rwsem(&adev->reset_sem);
> >       mutex_init(&adev->psp.mutex);
> >       mutex_init(&adev->notifier_lock);
> > +     mutex_init(&adev->pm.stable_pstate_ctx_lock);
> >
> >       r = amdgpu_device_init_apu_flags(adev);
> >       if (r)
> > diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > index 082539c70fd4..bcbc3190ed47 100644
> > --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > @@ -390,12 +390,14 @@ static ssize_t amdgpu_set_power_dpm_force_performance_level(struct device *dev,
> >               return -EINVAL;
> >       }
> >
> > +     mutex_lock(&adev->pm.stable_pstate_ctx_lock);
> >       if (pp_funcs->force_performance_level) {
> >               mutex_lock(&adev->pm.mutex);
> >               if (adev->pm.dpm.thermal_active) {
> >                       mutex_unlock(&adev->pm.mutex);
> >                       pm_runtime_mark_last_busy(ddev->dev);
> >                       pm_runtime_put_autosuspend(ddev->dev);
> > +                     mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
> >                       return -EINVAL;
> >               }
> >               ret = amdgpu_dpm_force_performance_level(adev, level);
> > @@ -403,6 +405,7 @@ static ssize_t amdgpu_set_power_dpm_force_performance_level(struct device *dev,
> >                       mutex_unlock(&adev->pm.mutex);
> >                       pm_runtime_mark_last_busy(ddev->dev);
> >                       pm_runtime_put_autosuspend(ddev->dev);
> > +                     mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
> >                       return -EINVAL;
> >               } else {
> >                       adev->pm.dpm.forced_level = level;
> > @@ -412,6 +415,10 @@ static ssize_t amdgpu_set_power_dpm_force_performance_level(struct device *dev,
> >       pm_runtime_mark_last_busy(ddev->dev);
> >       pm_runtime_put_autosuspend(ddev->dev);
> >
> > +     /* override whatever a user ctx may have set */
> > +     adev->pm.stable_pstate_ctx = NULL;
> > +     mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
> > +
> >       return count;
> >   }
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > index c464a045000d..629cb1ec6c03 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > @@ -468,6 +468,9 @@ struct amdgpu_pm {
> >        * 0 = disabled (default), otherwise enable corresponding debug mode
> >        */
> >       uint32_t                smu_debug_mask;
> > +
> > +     struct mutex            stable_pstate_ctx_lock;
> > +     struct amdgpu_ctx       *stable_pstate_ctx;
> >   };
> >
> >   #define R600_SSTU_DFLT                               0
> > diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> > index 0b94ec7b73e7..7f01f9830bf8 100644
> > --- a/include/uapi/drm/amdgpu_drm.h
> > +++ b/include/uapi/drm/amdgpu_drm.h
> > @@ -206,6 +206,8 @@ union drm_amdgpu_bo_list {
> >   #define AMDGPU_CTX_OP_FREE_CTX      2
> >   #define AMDGPU_CTX_OP_QUERY_STATE   3
> >   #define AMDGPU_CTX_OP_QUERY_STATE2  4
> > +#define AMDGPU_CTX_OP_GET_STABLE_PSTATE      5
> > +#define AMDGPU_CTX_OP_SET_STABLE_PSTATE      6
> >
> >   /* GPU reset status */
> >   #define AMDGPU_CTX_NO_RESET         0
> > @@ -238,10 +240,18 @@ union drm_amdgpu_bo_list {
> >   #define AMDGPU_CTX_PRIORITY_HIGH        512
> >   #define AMDGPU_CTX_PRIORITY_VERY_HIGH   1023
> >
> > +/* select a stable profiling pstate for perfmon tools */
> > +#define AMDGPU_CTX_STABLE_PSTATE_FLAGS_MASK  0xf
> > +#define AMDGPU_CTX_STABLE_PSTATE_NONE  0
> > +#define AMDGPU_CTX_STABLE_PSTATE_STANDARD  1
> > +#define AMDGPU_CTX_STABLE_PSTATE_MIN_SCLK  2
> > +#define AMDGPU_CTX_STABLE_PSTATE_MIN_MCLK  3
> > +#define AMDGPU_CTX_STABLE_PSTATE_PEAK  4
> > +
> >   struct drm_amdgpu_ctx_in {
> >       /** AMDGPU_CTX_OP_* */
> >       __u32   op;
> > -     /** For future use, no flags defined so far */
> > +     /** Flags */
> >       __u32   flags;
> >       __u32   ctx_id;
> >       /** AMDGPU_CTX_PRIORITY_* */
> > @@ -262,6 +272,11 @@ union drm_amdgpu_ctx_out {
> >                       /** Reset status since the last call of the ioctl. */
> >                       __u32   reset_status;
> >               } state;
> > +
> > +             struct {
> > +                     __u32   flags;
> > +                     __u32   _pad;
> > +             } pstate;
> >   };
> >
> >   union drm_amdgpu_ctx {
>




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux