[PATCH 1/2] drm/amdgpu/UAPI: add new CTX OP to get/set stable pstates
Alex Deucher
alexdeucher at gmail.com
Thu Dec 16 15:13:02 UTC 2021
On Thu, Dec 16, 2021 at 9:24 AM Christian König
<ckoenig.leichtzumerken at gmail.com> 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 at amd.com>
> > ---
> > 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 {
>
More information about the amd-gfx
mailing list