On 3/17/2025 8: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.
>
> v2: bail early if the the profile is already active (Lijo)
>
> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>
Thanks,
Lijo
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 28 ++++++++++++++++---------
> drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 28 ++++++++++++++++---------
> 2 files changed, 36 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..1991dd3d1056a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -460,18 +460,26 @@ 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);
> +
> + /* 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) {
> + 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);
>
> +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/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> index b4b8091980ad5..d716510b8dd68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> @@ -169,18 +169,26 @@ 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);
> +
> + /* 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) {
> + 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);
>
> +pg_lock:
> mutex_lock(&adev->vcn.inst[0].vcn_pg_lock);
> amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCN,
> AMD_PG_STATE_UNGATE);
