[PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Sathishkumar S]

There is a race condition which leads to dpm video power
profile switch (disable and enable) during active video
decode on multi-instance VCN hardware.

This patch aims to fix/skip step 3 in the below sequence:

 - inst_1       power_on
 - inst_0(idle) power_off
 - inst_0(idle) video_power_profile OFF (step 3)
 - inst_1       video_power_profile ON during next begin_use

Add flags to track ON/OFF vcn instances and check if all
instances are off before disabling video power profile.

Protect workload_profile_active also within pg_lock and ON it
during first use and OFF it when last VCN instance is powered
OFF. VCN workload_profile_mutex can be removed after similar
clean up is done for vcn2_5.

Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 9a76e11d1c18..da372dd7b761 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
 	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
 		mutex_lock(&vcn_inst->vcn_pg_lock);
 		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
-		mutex_unlock(&vcn_inst->vcn_pg_lock);
-		mutex_lock(&adev->vcn.workload_profile_mutex);
-		if (adev->vcn.workload_profile_active) {
+		adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
+		if (adev->vcn.workload_profile_active &&
+		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
 			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
 							    false);
 			if (r)
 				dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
 			adev->vcn.workload_profile_active = false;
 		}
-		mutex_unlock(&adev->vcn.workload_profile_mutex);
+		mutex_unlock(&vcn_inst->vcn_pg_lock);
 	} else {
 		schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
 	}
@@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 
 	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
-		goto pg_lock;
+	mutex_lock(&vcn_inst->vcn_pg_lock);
 
-	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);
@@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 			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);
 
-pg_lock:
-	mutex_lock(&vcn_inst->vcn_pg_lock);
-	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
+	if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
+		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
+		adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
+	}
 
 	/* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
 	if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index b3fb1d0e43fc..a876a182ff88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -366,6 +366,10 @@ struct amdgpu_vcn {
 	struct mutex            workload_profile_mutex;
 	u32 reg_count;
 	const struct amdgpu_hwip_reg_entry *reg_list;
+#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
+#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
+#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
+	u32 flags;
 };
 
 struct amdgpu_fw_shared_rb_ptrs_struct {
-- 
2.48.1

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Alex Deucher]

[-- Attachment #1: Type: text/plain, Size: 5177 bytes --]

On Tue, Aug 12, 2025 at 10:56 AM Sathishkumar S
<sathishkumar.sundararaju@amd.com> wrote:
>
> There is a race condition which leads to dpm video power
> profile switch (disable and enable) during active video
> decode on multi-instance VCN hardware.
>
> This patch aims to fix/skip step 3 in the below sequence:
>
>  - inst_1       power_on
>  - inst_0(idle) power_off
>  - inst_0(idle) video_power_profile OFF (step 3)
>  - inst_1       video_power_profile ON during next begin_use
>
> Add flags to track ON/OFF vcn instances and check if all
> instances are off before disabling video power profile.

I think you could also just look at the outstanding fences on the
other instances.  Something like the attached patch.  Either way works
for me.

Alex

>
> Protect workload_profile_active also within pg_lock and ON it
> during first use and OFF it when last VCN instance is powered
> OFF. VCN workload_profile_mutex can be removed after similar
> clean up is done for vcn2_5.
>
> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 9a76e11d1c18..da372dd7b761 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>         if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
>                 mutex_lock(&vcn_inst->vcn_pg_lock);
>                 vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
> -               mutex_unlock(&vcn_inst->vcn_pg_lock);
> -               mutex_lock(&adev->vcn.workload_profile_mutex);
> -               if (adev->vcn.workload_profile_active) {
> +               adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
> +               if (adev->vcn.workload_profile_active &&
> +                   !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
>                         r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>                                                             false);
>                         if (r)
>                                 dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
>                         adev->vcn.workload_profile_active = false;
>                 }
> -               mutex_unlock(&adev->vcn.workload_profile_mutex);
> +               mutex_unlock(&vcn_inst->vcn_pg_lock);
>         } else {
>                 schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
>         }
> @@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>
>         cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
> -               goto pg_lock;
> +       mutex_lock(&vcn_inst->vcn_pg_lock);
>
> -       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);
> @@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>                         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);
>
> -pg_lock:
> -       mutex_lock(&vcn_inst->vcn_pg_lock);
> -       vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
> +       if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
> +               vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
> +               adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
> +       }
>
>         /* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
>         if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index b3fb1d0e43fc..a876a182ff88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>         struct mutex            workload_profile_mutex;
>         u32 reg_count;
>         const struct amdgpu_hwip_reg_entry *reg_list;
> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
> +       u32 flags;
>  };
>
>  struct amdgpu_fw_shared_rb_ptrs_struct {
> --
> 2.48.1
>

[-- Attachment #2: 0001-drm-amdgpu-vcn-fix-video-profile-race-condition.patch --]
[-- Type: text/x-patch, Size: 3153 bytes --]

From 6db18acfa8aae239c36e6736308af88709979671 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
Date: Tue, 12 Aug 2025 11:38:09 -0400
Subject: [PATCH] drm/amdgpu/vcn: fix video profile race condition

If there are multiple instances of the VCN running,
we may end up switching the video profile while another
instance is active because we only take into account
the current instance's submissions.  Look at all
outstanding fences for the video profile.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 9a76e11d1c184..dcec86da6fd23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -415,19 +415,25 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
 	struct amdgpu_vcn_inst *vcn_inst =
 		container_of(work, struct amdgpu_vcn_inst, idle_work.work);
 	struct amdgpu_device *adev = vcn_inst->adev;
-	unsigned int fences = 0, fence[AMDGPU_MAX_VCN_INSTANCES] = {0};
-	unsigned int i = vcn_inst->inst, j;
+	unsigned int total_fences = 0, fence[AMDGPU_MAX_VCN_INSTANCES] = {0};
+	unsigned int i, j;
 	int r = 0;
 
-	if (adev->vcn.harvest_config & (1 << i))
+	if (adev->vcn.harvest_config & (1 << vcn_inst->inst))
 		return;
 
-	for (j = 0; j < adev->vcn.inst[i].num_enc_rings; ++j)
-		fence[i] += amdgpu_fence_count_emitted(&vcn_inst->ring_enc[j]);
+	for (i = 0; i < adev->vcn.num_vcn_inst; ++i) {
+		struct amdgpu_vcn_inst *v = &adev->vcn.inst[i];
+
+		for (j = 0; j < v->num_enc_rings; ++j)
+			fence[i] += amdgpu_fence_count_emitted(&v->ring_enc[j]);
+		fence[i] += amdgpu_fence_count_emitted(&v->ring_dec);
+		total_fences += fence[i];
+	}
 
 	/* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
 	if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
-	    !adev->vcn.inst[i].using_unified_queue) {
+	    !vcn_inst->using_unified_queue) {
 		struct dpg_pause_state new_state;
 
 		if (fence[i] ||
@@ -436,18 +442,17 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
 		else
 			new_state.fw_based = VCN_DPG_STATE__UNPAUSE;
 
-		adev->vcn.inst[i].pause_dpg_mode(vcn_inst, &new_state);
+		vcn_inst->pause_dpg_mode(vcn_inst, &new_state);
 	}
 
-	fence[i] += amdgpu_fence_count_emitted(&vcn_inst->ring_dec);
-	fences += fence[i];
-
-	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
+	if (!fence[vcn_inst->inst] && !atomic_read(&vcn_inst->total_submission_cnt)) {
+		/* This is specific to this instance */
 		mutex_lock(&vcn_inst->vcn_pg_lock);
 		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
 		mutex_unlock(&vcn_inst->vcn_pg_lock);
 		mutex_lock(&adev->vcn.workload_profile_mutex);
-		if (adev->vcn.workload_profile_active) {
+		/* this is global and depends on all VCN instances */
+		if (adev->vcn.workload_profile_active && !total_fences) {
 			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
 							    false);
 			if (r)
-- 
2.50.1

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[David Wu]

[-- Attachment #1: Type: text/plain, Size: 4633 bytes --]


On 2025-08-12 10:56, Sathishkumar S wrote:
> There is a race condition which leads to dpm video power
> profile switch (disable and enable) during active video
> decode on multi-instance VCN hardware.
>
> This patch aims to fix/skip step 3 in the below sequence:
>
>   - inst_1       power_on
>   - inst_0(idle) power_off
>   - inst_0(idle) video_power_profile OFF (step 3)
>   - inst_1       video_power_profile ON during next begin_use
>
> Add flags to track ON/OFF vcn instances and check if all
> instances are off before disabling video power profile.
>
> Protect workload_profile_active also within pg_lock and ON it
> during first use and OFF it when last VCN instance is powered
> OFF. VCN workload_profile_mutex can be removed after similar
> clean up is done for vcn2_5.
>
> Signed-off-by: Sathishkumar S<sathishkumar.sundararaju@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>   2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 9a76e11d1c18..da372dd7b761 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>   	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
>   		mutex_lock(&vcn_inst->vcn_pg_lock);
>   		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
> -		mutex_unlock(&vcn_inst->vcn_pg_lock);
> -		mutex_lock(&adev->vcn.workload_profile_mutex);
> -		if (adev->vcn.workload_profile_active) {
> +		adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
> +		if (adev->vcn.workload_profile_active &&
> +		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
>   			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>   							    false);
>   			if (r)
>   				dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
what if there is a context switch here? since the vcn_pg_lock is per 
instance - if another instance starts to
call amdgpu_vcn_ring_begin_use() the amdgpu_dpm_switch_power_profile() 
will not be called due to workload_profile_active is per device.
I think you still have a race condition.

David

>   			adev->vcn.workload_profile_active = false;
>   		}
> -		mutex_unlock(&adev->vcn.workload_profile_mutex);
> +		mutex_unlock(&vcn_inst->vcn_pg_lock);
>   	} else {
>   		schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
>   	}
> @@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>   
>   	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
> -		goto pg_lock;
> +	mutex_lock(&vcn_inst->vcn_pg_lock);
>   
> -	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);
> @@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>   			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);
>   
> -pg_lock:
> -	mutex_lock(&vcn_inst->vcn_pg_lock);
> -	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
> +	if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
> +		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
> +		adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
> +	}
>   
>   	/* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
>   	if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index b3fb1d0e43fc..a876a182ff88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>   	struct mutex            workload_profile_mutex;
>   	u32 reg_count;
>   	const struct amdgpu_hwip_reg_entry *reg_list;
> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
> +	u32 flags;
>   };
>   
>   struct amdgpu_fw_shared_rb_ptrs_struct {

[-- Attachment #2: Type: text/html, Size: 5533 bytes --]

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Sundararaju, Sathishkumar]

Hi Alex,


On 8/12/2025 9:38 PM, Alex Deucher wrote:
> On Tue, Aug 12, 2025 at 10:56 AM Sathishkumar S
> <sathishkumar.sundararaju@amd.com> wrote:
>> There is a race condition which leads to dpm video power
>> profile switch (disable and enable) during active video
>> decode on multi-instance VCN hardware.
>>
>> This patch aims to fix/skip step 3 in the below sequence:
>>
>>   - inst_1       power_on
>>   - inst_0(idle) power_off
>>   - inst_0(idle) video_power_profile OFF (step 3)
>>   - inst_1       video_power_profile ON during next begin_use
>>
>> Add flags to track ON/OFF vcn instances and check if all
>> instances are off before disabling video power profile.
> I think you could also just look at the outstanding fences on the
> other instances.  Something like the attached patch.  Either way works
> for me.

Yes, checking other instance fences is simpler, your patch is :-

Reviewed-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>

Regards,
Sathish
>
> Alex
>
>> Protect workload_profile_active also within pg_lock and ON it
>> during first use and OFF it when last VCN instance is powered
>> OFF. VCN workload_profile_mutex can be removed after similar
>> clean up is done for vcn2_5.
>>
>> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>>   2 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> index 9a76e11d1c18..da372dd7b761 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> @@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>>          if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
>>                  mutex_lock(&vcn_inst->vcn_pg_lock);
>>                  vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
>> -               mutex_unlock(&vcn_inst->vcn_pg_lock);
>> -               mutex_lock(&adev->vcn.workload_profile_mutex);
>> -               if (adev->vcn.workload_profile_active) {
>> +               adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
>> +               if (adev->vcn.workload_profile_active &&
>> +                   !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
>>                          r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>>                                                              false);
>>                          if (r)
>>                                  dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
>>                          adev->vcn.workload_profile_active = false;
>>                  }
>> -               mutex_unlock(&adev->vcn.workload_profile_mutex);
>> +               mutex_unlock(&vcn_inst->vcn_pg_lock);
>>          } else {
>>                  schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
>>          }
>> @@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>
>>          cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
>> -               goto pg_lock;
>> +       mutex_lock(&vcn_inst->vcn_pg_lock);
>>
>> -       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);
>> @@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>                          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);
>>
>> -pg_lock:
>> -       mutex_lock(&vcn_inst->vcn_pg_lock);
>> -       vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>> +       if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
>> +               vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>> +               adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
>> +       }
>>
>>          /* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
>>          if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> index b3fb1d0e43fc..a876a182ff88 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>>          struct mutex            workload_profile_mutex;
>>          u32 reg_count;
>>          const struct amdgpu_hwip_reg_entry *reg_list;
>> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
>> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
>> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
>> +       u32 flags;
>>   };
>>
>>   struct amdgpu_fw_shared_rb_ptrs_struct {
>> --
>> 2.48.1
>>

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Sundararaju, Sathishkumar]

[-- Attachment #1: Type: text/plain, Size: 5274 bytes --]

Hi David,

On 8/12/2025 10:21 PM, David Wu wrote:
>
>
> On 2025-08-12 10:56, Sathishkumar S wrote:
>> There is a race condition which leads to dpm video power
>> profile switch (disable and enable) during active video
>> decode on multi-instance VCN hardware.
>>
>> This patch aims to fix/skip step 3 in the below sequence:
>>
>>   - inst_1       power_on
>>   - inst_0(idle) power_off
>>   - inst_0(idle) video_power_profile OFF (step 3)
>>   - inst_1       video_power_profile ON during next begin_use
>>
>> Add flags to track ON/OFF vcn instances and check if all
>> instances are off before disabling video power profile.
>>
>> Protect workload_profile_active also within pg_lock and ON it
>> during first use and OFF it when last VCN instance is powered
>> OFF. VCN workload_profile_mutex can be removed after similar
>> clean up is done for vcn2_5.
>>
>> Signed-off-by: Sathishkumar S<sathishkumar.sundararaju@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>>   2 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> index 9a76e11d1c18..da372dd7b761 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> @@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>>   	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
>>   		mutex_lock(&vcn_inst->vcn_pg_lock);
>>   		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
>> -		mutex_unlock(&vcn_inst->vcn_pg_lock);
>> -		mutex_lock(&adev->vcn.workload_profile_mutex);
>> -		if (adev->vcn.workload_profile_active) {
>> +		adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
>> +		if (adev->vcn.workload_profile_active &&
>> +		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
>>   			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>>   							    false);
>>   			if (r)
>>   				dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
> what if there is a context switch here? since the vcn_pg_lock is per 
> instance - if another instance starts to
> call amdgpu_vcn_ring_begin_use() the amdgpu_dpm_switch_power_profile() 
> will not be called due to workload_profile_active is per device.
> I think you still have a race condition.

The situation you are explaining is bound to happen even in the current 
form of locks without this patch as well, in both cases, processes will 
run mutually exclusively at different times

with the one holding lock finishing first and then the other continues 
after, without defined ordering between them. workload_profile_active is 
common for all vcn instances, it is ON before powering ON

first inst and OFF after all the instances are powered off.

Regards,
Sathish
>
> David
>
>>   			adev->vcn.workload_profile_active = false;
>>   		}
>> -		mutex_unlock(&adev->vcn.workload_profile_mutex);
>> +		mutex_unlock(&vcn_inst->vcn_pg_lock);
>>   	} else {
>>   		schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
>>   	}
>> @@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>   
>>   	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
>> -		goto pg_lock;
>> +	mutex_lock(&vcn_inst->vcn_pg_lock);
>>   
>> -	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);
>> @@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>   			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);
>>   
>> -pg_lock:
>> -	mutex_lock(&vcn_inst->vcn_pg_lock);
>> -	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>> +	if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
>> +		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>> +		adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
>> +	}
>>   
>>   	/* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
>>   	if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> index b3fb1d0e43fc..a876a182ff88 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>>   	struct mutex            workload_profile_mutex;
>>   	u32 reg_count;
>>   	const struct amdgpu_hwip_reg_entry *reg_list;
>> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
>> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
>> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
>> +	u32 flags;
>>   };
>>   
>>   struct amdgpu_fw_shared_rb_ptrs_struct {

[-- Attachment #2: Type: text/html, Size: 6601 bytes --]

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[David Wu]

[-- Attachment #1: Type: text/plain, Size: 6298 bytes --]

Hi Sathis,

I think the issue is the /workload_profile_active/ is not protected by 
the lock /workload_profile_mutex/in idle and begin_use.
for multi instance case - all instances could be idle so the last one is 
trying to power off vcn as the fences is 0.
since it does not hold a global lock when counting the fences then when 
another instance has a new job, its /workload_profile_active/
is unknown as it could be ON as the last instance (idle handler) has not 
set it to OFF yet in its idle work handler. A context switch from idle 
to begin_use
will end up vcn power in OFF state. Also to make sure there isn't any 
fence miss - the /workload_profile_mutex/should be used for the
entire begin_use function and idle work handler.
I think Alex's patch just tightens it up to make race condition less 
likely happen.

David

On 2025-08-12 16:36, Sundararaju, Sathishkumar wrote:
>
> Hi David,
>
> On 8/12/2025 10:21 PM, David Wu wrote:
>>
>>
>> On 2025-08-12 10:56, Sathishkumar S wrote:
>>> There is a race condition which leads to dpm video power
>>> profile switch (disable and enable) during active video
>>> decode on multi-instance VCN hardware.
>>>
>>> This patch aims to fix/skip step 3 in the below sequence:
>>>
>>>   - inst_1       power_on
>>>   - inst_0(idle) power_off
>>>   - inst_0(idle) video_power_profile OFF (step 3)
>>>   - inst_1       video_power_profile ON during next begin_use
>>>
>>> Add flags to track ON/OFF vcn instances and check if all
>>> instances are off before disabling video power profile.
>>>
>>> Protect workload_profile_active also within pg_lock and ON it
>>> during first use and OFF it when last VCN instance is powered
>>> OFF. VCN workload_profile_mutex can be removed after similar
>>> clean up is done for vcn2_5.
>>>
>>> Signed-off-by: Sathishkumar S<sathishkumar.sundararaju@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>>>   2 files changed, 13 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> index 9a76e11d1c18..da372dd7b761 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> @@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>>>   	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
>>>   		mutex_lock(&vcn_inst->vcn_pg_lock);
>>>   		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
>>> -		mutex_unlock(&vcn_inst->vcn_pg_lock);
>>> -		mutex_lock(&adev->vcn.workload_profile_mutex);
>>> -		if (adev->vcn.workload_profile_active) {
>>> +		adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
>>> +		if (adev->vcn.workload_profile_active &&
>>> +		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
>>>   			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>>>   							    false);
>>>   			if (r)
>>>   				dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
>> what if there is a context switch here? since the vcn_pg_lock is per 
>> instance - if another instance starts to
>> call amdgpu_vcn_ring_begin_use() the 
>> amdgpu_dpm_switch_power_profile() will not be called due to 
>> workload_profile_active is per device.
>> I think you still have a race condition.
>
> The situation you are explaining is bound to happen even in the 
> current form of locks without this patch as well, in both cases, 
> processes will run mutually exclusively at different times
>
> with the one holding lock finishing first and then the other continues 
> after, without defined ordering between them. workload_profile_active 
> is common for all vcn instances, it is ON before powering ON
>
> first inst and OFF after all the instances are powered off.
>
> Regards,
> Sathish
>>
>> David
>>
>>>   			adev->vcn.workload_profile_active = false;
>>>   		}
>>> -		mutex_unlock(&adev->vcn.workload_profile_mutex);
>>> +		mutex_unlock(&vcn_inst->vcn_pg_lock);
>>>   	} else {
>>>   		schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
>>>   	}
>>> @@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>>   
>>>   	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
>>> -		goto pg_lock;
>>> +	mutex_lock(&vcn_inst->vcn_pg_lock);
>>>   
>>> -	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);
>>> @@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>>   			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);
>>>   
>>> -pg_lock:
>>> -	mutex_lock(&vcn_inst->vcn_pg_lock);
>>> -	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>>> +	if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
>>> +		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>>> +		adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
>>> +	}
>>>   
>>>   	/* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
>>>   	if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>> index b3fb1d0e43fc..a876a182ff88 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>>>   	struct mutex            workload_profile_mutex;
>>>   	u32 reg_count;
>>>   	const struct amdgpu_hwip_reg_entry *reg_list;
>>> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
>>> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
>>> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
>>> +	u32 flags;
>>>   };
>>>   
>>>   struct amdgpu_fw_shared_rb_ptrs_struct {

[-- Attachment #2: Type: text/html, Size: 8313 bytes --]

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Wu, David]

[-- Attachment #1: Type: text/plain, Size: 4661 bytes --]

Hi Sathish,
       Please see inline.
David
On 8/12/2025 10:56 AM, Sathishkumar S wrote:
> There is a race condition which leads to dpm video power
> profile switch (disable and enable) during active video
> decode on multi-instance VCN hardware.
>
> This patch aims to fix/skip step 3 in the below sequence:
>
>   - inst_1       power_on
>   - inst_0(idle) power_off
>   - inst_0(idle) video_power_profile OFF (step 3)
>   - inst_1       video_power_profile ON during next begin_use
>
> Add flags to track ON/OFF vcn instances and check if all
> instances are off before disabling video power profile.
>
> Protect workload_profile_active also within pg_lock and ON it
> during first use and OFF it when last VCN instance is powered
> OFF. VCN workload_profile_mutex can be removed after similar
> clean up is done for vcn2_5.
>
> Signed-off-by: Sathishkumar S<sathishkumar.sundararaju@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++---------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>   2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 9a76e11d1c18..da372dd7b761 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -445,16 +445,16 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>   	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
>   		mutex_lock(&vcn_inst->vcn_pg_lock);
>   		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
> -		mutex_unlock(&vcn_inst->vcn_pg_lock);
> -		mutex_lock(&adev->vcn.workload_profile_mutex);
keep these 2 lines
> -		if (adev->vcn.workload_profile_active) {
> +		adev->vcn.flags &= AMDGPU_VCN_FLAG_VINST_OFF(vcn_inst->inst);
> +		if (adev->vcn.workload_profile_active &&
> +		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
>   			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>   							    false);
>   			if (r)
>   				dev_warn(adev->dev, "(%d) failed to disable video power profile mode\n", r);
>   			adev->vcn.workload_profile_active = false;
>   		}
> -		mutex_unlock(&adev->vcn.workload_profile_mutex);
keep it
> +		mutex_unlock(&vcn_inst->vcn_pg_lock);
remove it
>   	} else {
>   		schedule_delayed_work(&vcn_inst->idle_work, VCN_IDLE_TIMEOUT);
>   	}
> @@ -470,14 +470,8 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>   
>   	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
> -		goto pg_lock;
> +	mutex_lock(&vcn_inst->vcn_pg_lock);
remove this mutex_lock
> -	mutex_lock(&adev->vcn.workload_profile_mutex);
keep it
>   	if (!adev->vcn.workload_profile_active) {
>   		r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>   						    true);
> @@ -485,11 +479,11 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>   			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);
keep it
> -pg_lock:
> -	mutex_lock(&vcn_inst->vcn_pg_lock);
> -	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
keep 2 lines above
> +	if (!(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst))) {
> +		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
> +		adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
> +	}
remove them and add the following before 
mutex_lock(&adev->vcn.workload_profile_mutex);

        adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);

there is no need to test this flag.
>   	/* Only set DPG pause for VCN3 or below, VCN4 and above will be handled by FW */
>   	if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG &&
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index b3fb1d0e43fc..a876a182ff88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>   	struct mutex            workload_profile_mutex;
>   	u32 reg_count;
>   	const struct amdgpu_hwip_reg_entry *reg_list;
> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
> +	u32 flags;
>   };
>   
>   struct amdgpu_fw_shared_rb_ptrs_struct {

[-- Attachment #2: Type: text/html, Size: 6759 bytes --]

[PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Sathishkumar S]

There is a race condition which leads to dpm video power
profile switch (disable and enable) during active video
decode on multi-instance VCN hardware.

This patch aims to fix/skip step 3 in the below sequence:

 - inst_1       power_on
 - inst_0(idle) power_off
 - inst_0(idle) video_power_profile OFF (step 3)
 - inst_1       video_power_profile ON during next begin_use

Add flags to track ON/OFF vcn instances and check if all
instances are off before disabling video power profile.

v2: (David Wu)
 - pg_lock is per instance it doesn't help solve the issue.
 - protect flags also with global workload_profile_mutex.

Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 9a76e11d1c18..b677b287dd49 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -447,7 +447,14 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
 		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
 		mutex_unlock(&vcn_inst->vcn_pg_lock);
 		mutex_lock(&adev->vcn.workload_profile_mutex);
-		if (adev->vcn.workload_profile_active) {
+		if (!adev->vcn.workload_profile_active &&
+		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
+			/* video profile is active , we are holding global workload_profile_mutex.
+			 * it is safe to check if flags are 0 here and be assured that all instances
+			 * are off, since no other begin_use paths can be holding this lock now.
+			 * so off video_power_profile and update workload_profile_active = false
+			 * since all vcn instances are inactive here.
+			 */
 			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
 							    false);
 			if (r)
@@ -470,24 +477,23 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 
 	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
-		goto pg_lock;
-
 	mutex_lock(&adev->vcn.workload_profile_mutex);
 	if (!adev->vcn.workload_profile_active) {
+	/* If inactive proceed to ON video_power_profile and update 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;
 	}
+	/* Holding global workload_profile_mutex, so none of the idle handlers can access flags.
+	 * and cannot OFF video_power_profile at this point. Can safely update vcn.flags to
+	 * indicate active vcn instances, which is visible to any idle handlers who later grab
+	 * this lock and check flags for any active vcn instances.
+	 */
+	adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
 	mutex_unlock(&adev->vcn.workload_profile_mutex);
 
-pg_lock:
 	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/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index b3fb1d0e43fc..a876a182ff88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -366,6 +366,10 @@ struct amdgpu_vcn {
 	struct mutex            workload_profile_mutex;
 	u32 reg_count;
 	const struct amdgpu_hwip_reg_entry *reg_list;
+#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
+#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
+#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
+	u32 flags;
 };
 
 struct amdgpu_fw_shared_rb_ptrs_struct {
-- 
2.48.1

Re: [PATCH] drm/amdgpu/vcn: Fix video_profile switch race condition

[Sundararaju, Sathishkumar]

Please ignore this.

Regards,

Sathish

On 8/14/2025 2:40 PM, Sathishkumar S wrote:
> There is a race condition which leads to dpm video power
> profile switch (disable and enable) during active video
> decode on multi-instance VCN hardware.
>
> This patch aims to fix/skip step 3 in the below sequence:
>
>   - inst_1       power_on
>   - inst_0(idle) power_off
>   - inst_0(idle) video_power_profile OFF (step 3)
>   - inst_1       video_power_profile ON during next begin_use
>
> Add flags to track ON/OFF vcn instances and check if all
> instances are off before disabling video power profile.
>
> v2: (David Wu)
>   - pg_lock is per instance it doesn't help solve the issue.
>   - protect flags also with global workload_profile_mutex.
>
> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 24 +++++++++++++++---------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  4 ++++
>   2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 9a76e11d1c18..b677b287dd49 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -447,7 +447,14 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>   		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
>   		mutex_unlock(&vcn_inst->vcn_pg_lock);
>   		mutex_lock(&adev->vcn.workload_profile_mutex);
> -		if (adev->vcn.workload_profile_active) {
> +		if (!adev->vcn.workload_profile_active &&
> +		    !(adev->vcn.flags & AMDGPU_VCN_FLAG_VINST_MASK(adev->vcn.num_vcn_inst))) {
> +			/* video profile is active , we are holding global workload_profile_mutex.
> +			 * it is safe to check if flags are 0 here and be assured that all instances
> +			 * are off, since no other begin_use paths can be holding this lock now.
> +			 * so off video_power_profile and update workload_profile_active = false
> +			 * since all vcn instances are inactive here.
> +			 */
>   			r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>   							    false);
>   			if (r)
> @@ -470,24 +477,23 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>   
>   	cancel_delayed_work_sync(&vcn_inst->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->vcn.workload_profile_active)
> -		goto pg_lock;
> -
>   	mutex_lock(&adev->vcn.workload_profile_mutex);
>   	if (!adev->vcn.workload_profile_active) {
> +	/* If inactive proceed to ON video_power_profile and update 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;
>   	}
> +	/* Holding global workload_profile_mutex, so none of the idle handlers can access flags.
> +	 * and cannot OFF video_power_profile at this point. Can safely update vcn.flags to
> +	 * indicate active vcn instances, which is visible to any idle handlers who later grab
> +	 * this lock and check flags for any active vcn instances.
> +	 */
> +	adev->vcn.flags |= AMDGPU_VCN_FLAG_VINST_ON(vcn_inst->inst);
>   	mutex_unlock(&adev->vcn.workload_profile_mutex);
>   
> -pg_lock:
>   	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/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index b3fb1d0e43fc..a876a182ff88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -366,6 +366,10 @@ struct amdgpu_vcn {
>   	struct mutex            workload_profile_mutex;
>   	u32 reg_count;
>   	const struct amdgpu_hwip_reg_entry *reg_list;
> +#define AMDGPU_VCN_FLAG_VINST_MASK(n)  (BIT(n+1) - 1)
> +#define AMDGPU_VCN_FLAG_VINST_ON(n)    (BIT(n))
> +#define AMDGPU_VCN_FLAG_VINST_OFF(n)   (~BIT(n))
> +	u32 flags;
>   };
>   
>   struct amdgpu_fw_shared_rb_ptrs_struct {