[PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[Alex Deucher]

No need to make the workload profile setup dependent
on the results of cancelling the delayed work thread.
We have all of the necessary checking in place for the
workload profile reference counting, so separate the
two.  As it is now, we can theoretically end up with
the call from begin_use happening while the worker
thread is executing which would result in the profile
not getting set for that submission.  It should not
affect the reference counting.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 099329d15b9ff..20424f8c4925f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -2188,18 +2188,18 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
 
 	atomic_inc(&adev->gfx.total_submission_cnt);
 
-	if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
-		mutex_lock(&adev->gfx.workload_profile_mutex);
-		if (!adev->gfx.workload_profile_active) {
-			r = amdgpu_dpm_switch_power_profile(adev, profile, true);
-			if (r)
-				dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
-					 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
-					 "fullscreen 3D" : "compute");
-			adev->gfx.workload_profile_active = true;
-		}
-		mutex_unlock(&adev->gfx.workload_profile_mutex);
+	cancel_delayed_work_sync(&adev->gfx.idle_work);
+
+	mutex_lock(&adev->gfx.workload_profile_mutex);
+	if (!adev->gfx.workload_profile_active) {
+		r = amdgpu_dpm_switch_power_profile(adev, profile, true);
+		if (r)
+			dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
+				 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
+				 "fullscreen 3D" : "compute");
+		adev->gfx.workload_profile_active = true;
 	}
+	mutex_unlock(&adev->gfx.workload_profile_mutex);
 }
 
 void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)
-- 
2.48.1

[PATCH 2/2] drm/amdgpu/vcn: adjust workload profile handling

[Alex Deucher]

No need to make the workload profile setup dependent
on the results of cancelling the delayed work thread.
We have all of the necessary checking in place for the
workload profile reference counting, so separate the
two.  As it is now, we can theoretically end up with
the call from begin_use happening while the worker
thread is executing which would result in the profile
not getting set for that submission.  It should not
affect the reference counting.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 20 ++++++++++----------
 drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c   | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 6b410e601bb65..0ab74b46bcb0a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -460,17 +460,17 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 
 	atomic_inc(&vcn_inst->total_submission_cnt);
 
-	if (!cancel_delayed_work_sync(&vcn_inst->idle_work)) {
-		mutex_lock(&adev->vcn.workload_profile_mutex);
-		if (!adev->vcn.workload_profile_active) {
-			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
-							    true);
-			if (r)
-				dev_warn(adev->dev, "(%d) failed to switch to video power profile mode\n", r);
-			adev->vcn.workload_profile_active = true;
-		}
-		mutex_unlock(&adev->vcn.workload_profile_mutex);
+	cancel_delayed_work_sync(&vcn_inst->idle_work);
+
+	mutex_lock(&adev->vcn.workload_profile_mutex);
+	if (!adev->vcn.workload_profile_active) {
+		r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
+						    true);
+		if (r)
+			dev_warn(adev->dev, "(%d) failed to switch to video power profile mode\n", r);
+		adev->vcn.workload_profile_active = true;
 	}
+	mutex_unlock(&adev->vcn.workload_profile_mutex);
 
 	mutex_lock(&vcn_inst->vcn_pg_lock);
 	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
index b4b8091980ad5..8c7c50f34fce0 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -169,17 +169,17 @@ static void vcn_v2_5_ring_begin_use(struct amdgpu_ring *ring)
 
 	atomic_inc(&adev->vcn.inst[0].total_submission_cnt);
 
-	if (!cancel_delayed_work_sync(&adev->vcn.inst[0].idle_work)) {
-		mutex_lock(&adev->vcn.workload_profile_mutex);
-		if (!adev->vcn.workload_profile_active) {
-			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
-							    true);
-			if (r)
-				dev_warn(adev->dev, "(%d) failed to switch to video power profile mode\n", r);
-			adev->vcn.workload_profile_active = true;
-		}
-		mutex_unlock(&adev->vcn.workload_profile_mutex);
+	cancel_delayed_work_sync(&adev->vcn.inst[0].idle_work);
+
+	mutex_lock(&adev->vcn.workload_profile_mutex);
+	if (!adev->vcn.workload_profile_active) {
+		r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
+						    true);
+		if (r)
+			dev_warn(adev->dev, "(%d) failed to switch to video power profile mode\n", r);
+		adev->vcn.workload_profile_active = true;
 	}
+	mutex_unlock(&adev->vcn.workload_profile_mutex);
 
 	mutex_lock(&adev->vcn.inst[0].vcn_pg_lock);
 	amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCN,
-- 
2.48.1

Re: [PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[Lazar, Lijo]

On 3/14/2025 7:17 PM, Alex Deucher wrote:
> No need to make the workload profile setup dependent
> on the results of cancelling the delayed work thread.
> We have all of the necessary checking in place for the
> workload profile reference counting, so separate the
> two.  As it is now, we can theoretically end up with
> the call from begin_use happening while the worker
> thread is executing which would result in the profile
> not getting set for that submission.  It should not
> affect the reference counting.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 099329d15b9ff..20424f8c4925f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -2188,18 +2188,18 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
>  
>  	atomic_inc(&adev->gfx.total_submission_cnt);
>  
> -	if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
> -		mutex_lock(&adev->gfx.workload_profile_mutex);
> -		if (!adev->gfx.workload_profile_active) {
> -			r = amdgpu_dpm_switch_power_profile(adev, profile, true);
> -			if (r)
> -				dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
> -					 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
> -					 "fullscreen 3D" : "compute");
> -			adev->gfx.workload_profile_active = true;
> -		}
> -		mutex_unlock(&adev->gfx.workload_profile_mutex);
> +	cancel_delayed_work_sync(&adev->gfx.idle_work);
> +

To avoid locking/unlocking mutex for each begin_use, I think here we
could do like

if (adev->gfx.workload_profile_active)
	return;

Thanks,
Lijo

> +	mutex_lock(&adev->gfx.workload_profile_mutex);
> +	if (!adev->gfx.workload_profile_active) {
> +		r = amdgpu_dpm_switch_power_profile(adev, profile, true);
> +		if (r)
> +			dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
> +				 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
> +				 "fullscreen 3D" : "compute");
> +		adev->gfx.workload_profile_active = true;
>  	}
> +	mutex_unlock(&adev->gfx.workload_profile_mutex);
>  }
>  
>  void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)

Re: [PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[Alex Deucher]

On Fri, Mar 14, 2025 at 10:53 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 3/14/2025 7:17 PM, Alex Deucher wrote:
> > No need to make the workload profile setup dependent
> > on the results of cancelling the delayed work thread.
> > We have all of the necessary checking in place for the
> > workload profile reference counting, so separate the
> > two.  As it is now, we can theoretically end up with
> > the call from begin_use happening while the worker
> > thread is executing which would result in the profile
> > not getting set for that submission.  It should not
> > affect the reference counting.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > index 099329d15b9ff..20424f8c4925f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > @@ -2188,18 +2188,18 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
> >
> >       atomic_inc(&adev->gfx.total_submission_cnt);
> >
> > -     if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
> > -             mutex_lock(&adev->gfx.workload_profile_mutex);
> > -             if (!adev->gfx.workload_profile_active) {
> > -                     r = amdgpu_dpm_switch_power_profile(adev, profile, true);
> > -                     if (r)
> > -                             dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
> > -                                      profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
> > -                                      "fullscreen 3D" : "compute");
> > -                     adev->gfx.workload_profile_active = true;
> > -             }
> > -             mutex_unlock(&adev->gfx.workload_profile_mutex);
> > +     cancel_delayed_work_sync(&adev->gfx.idle_work);
> > +
>
> To avoid locking/unlocking mutex for each begin_use, I think here we
> could do like
>
> if (adev->gfx.workload_profile_active)
>         return;

But that is what the mutex is protecting.

Alex

>
> Thanks,
> Lijo
>
> > +     mutex_lock(&adev->gfx.workload_profile_mutex);
> > +     if (!adev->gfx.workload_profile_active) {
> > +             r = amdgpu_dpm_switch_power_profile(adev, profile, true);
> > +             if (r)
> > +                     dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
> > +                              profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
> > +                              "fullscreen 3D" : "compute");
> > +             adev->gfx.workload_profile_active = true;
> >       }
> > +     mutex_unlock(&adev->gfx.workload_profile_mutex);
> >  }
> >
> >  void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)
>

Re: [PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[Lazar, Lijo]

On 3/14/2025 8:28 PM, Alex Deucher wrote:
> On Fri, Mar 14, 2025 at 10:53 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>>
>>
>>
>> On 3/14/2025 7:17 PM, Alex Deucher wrote:
>>> No need to make the workload profile setup dependent
>>> on the results of cancelling the delayed work thread.
>>> We have all of the necessary checking in place for the
>>> workload profile reference counting, so separate the
>>> two.  As it is now, we can theoretically end up with
>>> the call from begin_use happening while the worker
>>> thread is executing which would result in the profile
>>> not getting set for that submission.  It should not
>>> affect the reference counting.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> index 099329d15b9ff..20424f8c4925f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> @@ -2188,18 +2188,18 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
>>>
>>>       atomic_inc(&adev->gfx.total_submission_cnt);
>>>
>>> -     if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
>>> -             mutex_lock(&adev->gfx.workload_profile_mutex);
>>> -             if (!adev->gfx.workload_profile_active) {
>>> -                     r = amdgpu_dpm_switch_power_profile(adev, profile, true);
>>> -                     if (r)
>>> -                             dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
>>> -                                      profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
>>> -                                      "fullscreen 3D" : "compute");
>>> -                     adev->gfx.workload_profile_active = true;
>>> -             }
>>> -             mutex_unlock(&adev->gfx.workload_profile_mutex);
>>> +     cancel_delayed_work_sync(&adev->gfx.idle_work);
>>> +
>>
>> To avoid locking/unlocking mutex for each begin_use, I think here we
>> could do like
>>
>> if (adev->gfx.workload_profile_active)
>>         return;
> 
> But that is what the mutex is protecting.
> 

I think once we cancelled the work, there is no one to turn it to false.
We don't mind if somebody else changed to true already.

Thanks,
Lijo

> Alex
> 
>>
>> Thanks,
>> Lijo
>>
>>> +     mutex_lock(&adev->gfx.workload_profile_mutex);
>>> +     if (!adev->gfx.workload_profile_active) {
>>> +             r = amdgpu_dpm_switch_power_profile(adev, profile, true);
>>> +             if (r)
>>> +                     dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
>>> +                              profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
>>> +                              "fullscreen 3D" : "compute");
>>> +             adev->gfx.workload_profile_active = true;
>>>       }
>>> +     mutex_unlock(&adev->gfx.workload_profile_mutex);
>>>  }
>>>
>>>  void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)
>>

[PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[Alex Deucher]

No need to make the workload profile setup dependent
on the results of cancelling the delayed work thread.
We have all of the necessary checking in place for the
workload profile reference counting, so separate the
two.  As it is now, we can theoretically end up with
the call from begin_use happening while the worker
thread is executing which would result in the profile
not getting set for that submission.  It should not
affect the reference counting.

v2: bail early if the the profile is already active (Lijo)

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 29 +++++++++++++++----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 099329d15b9ff..4beb0609e7034 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -2188,18 +2188,25 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
 
 	atomic_inc(&adev->gfx.total_submission_cnt);
 
-	if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
-		mutex_lock(&adev->gfx.workload_profile_mutex);
-		if (!adev->gfx.workload_profile_active) {
-			r = amdgpu_dpm_switch_power_profile(adev, profile, true);
-			if (r)
-				dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
-					 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
-					 "fullscreen 3D" : "compute");
-			adev->gfx.workload_profile_active = true;
-		}
-		mutex_unlock(&adev->gfx.workload_profile_mutex);
+	cancel_delayed_work_sync(&adev->gfx.idle_work);
+
+	/* We can safely return early here because we've cancelled the
+	 * the delayed work so there is no one else to set it to false
+	 * and we don't care if someone else sets it to true.
+	 */
+	if (adev->gfx.workload_profile_active)
+		return;
+
+	mutex_lock(&adev->gfx.workload_profile_mutex);
+	if (!adev->gfx.workload_profile_active) {
+		r = amdgpu_dpm_switch_power_profile(adev, profile, true);
+		if (r)
+			dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
+				 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
+				 "fullscreen 3D" : "compute");
+		adev->gfx.workload_profile_active = true;
 	}
+	mutex_unlock(&adev->gfx.workload_profile_mutex);
 }
 
 void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)
-- 
2.48.1

Re: [PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

[Lazar, Lijo]

On 3/15/2025 12:15 AM, Alex Deucher wrote:
> No need to make the workload profile setup dependent
> on the results of cancelling the delayed work thread.
> We have all of the necessary checking in place for the
> workload profile reference counting, so separate the
> two.  As it is now, we can theoretically end up with
> the call from begin_use happening while the worker
> thread is executing which would result in the profile
> not getting set for that submission.  It should not
> affect the reference counting.
> 
> v2: bail early if the the profile is already active (Lijo)
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 29 +++++++++++++++----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 099329d15b9ff..4beb0609e7034 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -2188,18 +2188,25 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
>  
>  	atomic_inc(&adev->gfx.total_submission_cnt);
>  
> -	if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
> -		mutex_lock(&adev->gfx.workload_profile_mutex);
> -		if (!adev->gfx.workload_profile_active) {
> -			r = amdgpu_dpm_switch_power_profile(adev, profile, true);
> -			if (r)
> -				dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
> -					 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
> -					 "fullscreen 3D" : "compute");
> -			adev->gfx.workload_profile_active = true;
> -		}
> -		mutex_unlock(&adev->gfx.workload_profile_mutex);
> +	cancel_delayed_work_sync(&adev->gfx.idle_work);
> +
> +	/* We can safely return early here because we've cancelled the
> +	 * the delayed work so there is no one else to set it to false
> +	 * and we don't care if someone else sets it to true.
> +	 */

The same may be extended to vcn also. Anyway, series is

	Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> +	if (adev->gfx.workload_profile_active)
> +		return;
> +
> +	mutex_lock(&adev->gfx.workload_profile_mutex);
> +	if (!adev->gfx.workload_profile_active) {
> +		r = amdgpu_dpm_switch_power_profile(adev, profile, true);
> +		if (r)
> +			dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
> +				 profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
> +				 "fullscreen 3D" : "compute");
> +		adev->gfx.workload_profile_active = true;
>  	}
> +	mutex_unlock(&adev->gfx.workload_profile_mutex);
>  }
>  
>  void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)