Re: [PATCH] drm/amdgpu/vcn: fix idle work handler for VCN 2.5

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 3/4/2025 7:50 PM, Alex Deucher wrote:
> VCN 2.5 uses the PG callback to enable VCN DPM which is
> a global state.  As such, we need to make sure all instances
> are in the same state.  Use amdgpu_device_ip_set_powergating_state()
> rather than the per instance set_pg_state() callback.
> 
> Fixes: 4ce4fe27205c ("drm/amdgpu/vcn: use per instance callbacks for idle work handler")
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 8d8b39e6d197a..b93102e9fb43e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -437,7 +437,14 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>  	fences += fence[i];
>  
>  	if (!fences && !atomic_read(&vcn_inst->total_submission_cnt)) {
> -		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
> +		/* VCN 2.5 PG is actually DPM enablement which is global so
> +		 * update all instances
> +		 */
> +		if (amdgpu_ip_version(adev, UVD_HWIP, 0) == IP_VERSION(2, 5, 0))
> +			amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCN,
> +							       AMD_PG_STATE_GATE);

What about moving this logic to vcn_v2_5_set_pg_state? Apart from that,
the number of outstanding fences are counted only for the corresponding
instance. Won't this be problematic when multiple instances are active
and one of them gets to idle? May need a refcount as well.

Thanks,
Lijo


> +		else
> +			vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_GATE);
>  		r = amdgpu_dpm_switch_power_profile(adev, PP_SMC_POWER_PROFILE_VIDEO,
>  						    false);
>  		if (r)
> @@ -463,7 +470,14 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>  	}
>  
>  	mutex_lock(&vcn_inst->vcn_pg_lock);
> -	vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
> +	/* VCN 2.5 PG is actually DPM enablement which is global so
> +	 * update all instances
> +	 */
> +	if (amdgpu_ip_version(adev, UVD_HWIP, 0) == IP_VERSION(2, 5, 0))
> +		amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCN,
> +						       AMD_PG_STATE_UNGATE);
> +	else
> +		vcn_inst->set_pg_state(vcn_inst, AMD_PG_STATE_UNGATE);
>  
>  	/* 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 &&