[PATCH] drm/amdgpu/ctx: only reset stable pstate if the user changed it

[PATCH] drm/amdgpu/ctx: only reset stable pstate if the user changed it

[Alex Deucher]

Track whether the user requested a stable pstate via the ctx
IOCTL and only reset the pstate on context destroy if the user
actually changed it.  This avoids changing the pstate on contex
destroy if the user never changed it in the first place via the
IOCTL.

Fixes: 8cda7a4f96e435 ("drm/amdgpu/UAPI: add new CTX OP to get/set stable pstates")
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 5 ++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 5981c7d9bd48..e4b0c6ec227c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -330,6 +330,8 @@ static int amdgpu_ctx_set_stable_pstate(struct amdgpu_ctx *ctx,
 		adev->pm.stable_pstate_ctx = NULL;
 	else
 		adev->pm.stable_pstate_ctx = ctx;
+
+	ctx->stable_pstate_requested = true;
 done:
 	mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
 
@@ -353,7 +355,8 @@ static void amdgpu_ctx_fini(struct kref *ref)
 	}
 
 	if (drm_dev_enter(&adev->ddev, &idx)) {
-		amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);
+		if (ctx->stable_pstate_requested)
+			amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);
 		drm_dev_exit(idx);
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index d0cbfcea90f7..f03e842209b8 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 {
 	unsigned long			ras_counter_ce;
 	unsigned long			ras_counter_ue;
 	uint32_t			stable_pstate;
+	bool				stable_pstate_requested;
 };
 
 struct amdgpu_ctx_mgr {
-- 
2.35.1

RE: [PATCH] drm/amdgpu/ctx: only reset stable pstate if the user changed it

[Quan, Evan]

[AMD Official Use Only - General]

Reviewed-by: Evan Quan <evan.quan@amd.com>

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex
> Deucher
> Sent: Tuesday, May 10, 2022 4:04 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: [PATCH] drm/amdgpu/ctx: only reset stable pstate if the user
> changed it
> 
> Track whether the user requested a stable pstate via the ctx
> IOCTL and only reset the pstate on context destroy if the user
> actually changed it.  This avoids changing the pstate on contex
> destroy if the user never changed it in the first place via the
> IOCTL.
> 
> Fixes: 8cda7a4f96e435 ("drm/amdgpu/UAPI: add new CTX OP to get/set
> stable pstates")
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 5 ++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 5981c7d9bd48..e4b0c6ec227c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -330,6 +330,8 @@ static int amdgpu_ctx_set_stable_pstate(struct
> amdgpu_ctx *ctx,
>  		adev->pm.stable_pstate_ctx = NULL;
>  	else
>  		adev->pm.stable_pstate_ctx = ctx;
> +
> +	ctx->stable_pstate_requested = true;
>  done:
>  	mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
> 
> @@ -353,7 +355,8 @@ static void amdgpu_ctx_fini(struct kref *ref)
>  	}
> 
>  	if (drm_dev_enter(&adev->ddev, &idx)) {
> -		amdgpu_ctx_set_stable_pstate(ctx,
> AMDGPU_CTX_STABLE_PSTATE_NONE);
> +		if (ctx->stable_pstate_requested)
> +			amdgpu_ctx_set_stable_pstate(ctx,
> AMDGPU_CTX_STABLE_PSTATE_NONE);
>  		drm_dev_exit(idx);
>  	}
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> index d0cbfcea90f7..f03e842209b8 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 {
>  	unsigned long			ras_counter_ce;
>  	unsigned long			ras_counter_ue;
>  	uint32_t			stable_pstate;
> +	bool				stable_pstate_requested;
>  };
> 
>  struct amdgpu_ctx_mgr {
> --
> 2.35.1

Re: [PATCH] drm/amdgpu/ctx: only reset stable pstate if the user changed it

[Lazar, Lijo]

On 5/10/2022 1:33 AM, Alex Deucher wrote:
> Track whether the user requested a stable pstate via the ctx
> IOCTL and only reset the pstate on context destroy if the user
> actually changed it.  This avoids changing the pstate on contex
> destroy if the user never changed it in the first place via the
> IOCTL.
> 
> Fixes: 8cda7a4f96e435 ("drm/amdgpu/UAPI: add new CTX OP to get/set stable pstates")
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 5 ++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 1 +
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 5981c7d9bd48..e4b0c6ec227c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -330,6 +330,8 @@ static int amdgpu_ctx_set_stable_pstate(struct amdgpu_ctx *ctx,
>   		adev->pm.stable_pstate_ctx = NULL;
>   	else
>   		adev->pm.stable_pstate_ctx = ctx;
> +
> +	ctx->stable_pstate_requested = true;
>   done:
>   	mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
>   
> @@ -353,7 +355,8 @@ static void amdgpu_ctx_fini(struct kref *ref)
>   	}
>   
>   	if (drm_dev_enter(&adev->ddev, &idx)) {
> -		amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);
> +		if (ctx->stable_pstate_requested)
> +			amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);

Why not have the check inside amdgpu_ctx_set_stable_pstate - if current 
stable pstate is not the same as the requested one, then only switch. If 
the user has not made a request, it will be NONE and the call doesn't 
have any effect.

Thanks,
Lijo

>   		drm_dev_exit(idx);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> index d0cbfcea90f7..f03e842209b8 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 {
>   	unsigned long			ras_counter_ce;
>   	unsigned long			ras_counter_ue;
>   	uint32_t			stable_pstate;
> +	bool				stable_pstate_requested;
>   };
>   
>   struct amdgpu_ctx_mgr {
> 

Re: [PATCH] drm/amdgpu/ctx: only reset stable pstate if the user changed it

[Alex Deucher]

On Tue, May 10, 2022 at 6:24 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 5/10/2022 1:33 AM, Alex Deucher wrote:
> > Track whether the user requested a stable pstate via the ctx
> > IOCTL and only reset the pstate on context destroy if the user
> > actually changed it.  This avoids changing the pstate on contex
> > destroy if the user never changed it in the first place via the
> > IOCTL.
> >
> > Fixes: 8cda7a4f96e435 ("drm/amdgpu/UAPI: add new CTX OP to get/set stable pstates")
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 5 ++++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 1 +
> >   2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > index 5981c7d9bd48..e4b0c6ec227c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > @@ -330,6 +330,8 @@ static int amdgpu_ctx_set_stable_pstate(struct amdgpu_ctx *ctx,
> >               adev->pm.stable_pstate_ctx = NULL;
> >       else
> >               adev->pm.stable_pstate_ctx = ctx;
> > +
> > +     ctx->stable_pstate_requested = true;
> >   done:
> >       mutex_unlock(&adev->pm.stable_pstate_ctx_lock);
> >
> > @@ -353,7 +355,8 @@ static void amdgpu_ctx_fini(struct kref *ref)
> >       }
> >
> >       if (drm_dev_enter(&adev->ddev, &idx)) {
> > -             amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);
> > +             if (ctx->stable_pstate_requested)
> > +                     amdgpu_ctx_set_stable_pstate(ctx, AMDGPU_CTX_STABLE_PSTATE_NONE);
>
> Why not have the check inside amdgpu_ctx_set_stable_pstate - if current
> stable pstate is not the same as the requested one, then only switch. If
> the user has not made a request, it will be NONE and the call doesn't
> have any effect.

Good idea.  New patch sent out earlier today.

Alex

>
> Thanks,
> Lijo
>
> >               drm_dev_exit(idx);
> >       }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> > index d0cbfcea90f7..f03e842209b8 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 {
> >       unsigned long                   ras_counter_ce;
> >       unsigned long                   ras_counter_ue;
> >       uint32_t                        stable_pstate;
> > +     bool                            stable_pstate_requested;
> >   };
> >
> >   struct amdgpu_ctx_mgr {
> >