Re: [PATCH] drm/amdgpu: Save VCN shared memory with init reset

[Christian König <christian.koenig@xxxxxxx>]

Am 15.10.24 um 08:23 schrieb Lijo Lazar:
> VCN shared memory is in framebuffer and there are some flags initialized
> during sw_init. Ideally, such programming should be during hw_init.

IIRC that was intentionally not done during hw_init for some reason. 
@Leo do you remember why?

Regards,
Christian.

> Make sure the flags are saved during reset on initialization since that
> reset will affect frame buffer region. For clarity, separate it out to
> another function.
>
> Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx>
> Reported-by: Hao Zhou <hao.zhou@xxxxxxx>
>
> Fixes: 1b665567fd6d ("drm/amdgpu: Add reset on init handler for XGMI")
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c |  6 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c   | 26 ++++++++++++++---------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h   |  1 +
>   3 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> index 3e2724590dbf..ea1e128362a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> @@ -49,6 +49,12 @@ static int amdgpu_reset_xgmi_reset_on_init_suspend(struct amdgpu_device *adev)
>   		adev->ip_blocks[i].status.hw = false;
>   	}
>   
> +	/* VCN FW shared region is in frambuffer, there are some flags
> +	 * initialized in that region during sw_init. Make sure the region is
> +	 * backed up.
> +	 */
> +	amdgpu_vcn_save_fw_shared_region(adev);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 43f44cc201cb..3a1e83fa3d1f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -294,21 +294,12 @@ bool amdgpu_vcn_is_disabled_vcn(struct amdgpu_device *adev, enum vcn_ring_type t
>   	return ret;
>   }
>   
> -int amdgpu_vcn_suspend(struct amdgpu_device *adev)
> +int amdgpu_vcn_save_fw_shared_region(struct amdgpu_device *adev)
>   {
>   	unsigned int size;
>   	void *ptr;
>   	int i, idx;
>   
> -	bool in_ras_intr = amdgpu_ras_intr_triggered();
> -
> -	cancel_delayed_work_sync(&adev->vcn.idle_work);
> -
> -	/* err_event_athub will corrupt VCPU buffer, so we need to
> -	 * restore fw data and clear buffer in amdgpu_vcn_resume() */
> -	if (in_ras_intr)
> -		return 0;
> -
>   	for (i = 0; i < adev->vcn.num_vcn_inst; ++i) {
>   		if (adev->vcn.harvest_config & (1 << i))
>   			continue;
> @@ -327,9 +318,24 @@ int amdgpu_vcn_suspend(struct amdgpu_device *adev)
>   			drm_dev_exit(idx);
>   		}
>   	}
> +
>   	return 0;
>   }
>   
> +int amdgpu_vcn_suspend(struct amdgpu_device *adev)
> +{
> +	bool in_ras_intr = amdgpu_ras_intr_triggered();
> +
> +	cancel_delayed_work_sync(&adev->vcn.idle_work);
> +
> +	/* err_event_athub will corrupt VCPU buffer, so we need to
> +	 * restore fw data and clear buffer in amdgpu_vcn_resume() */
> +	if (in_ras_intr)
> +		return 0;
> +
> +	return amdgpu_vcn_save_fw_shared_region(adev);
> +}
> +
>   int amdgpu_vcn_resume(struct amdgpu_device *adev)
>   {
>   	unsigned int size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> index 2a1f3dbb14d3..14713a5c6a28 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> @@ -518,5 +518,6 @@ int amdgpu_vcn_ras_sw_init(struct amdgpu_device *adev);
>   
>   int amdgpu_vcn_psp_update_sram(struct amdgpu_device *adev, int inst_idx,
>   			       enum AMDGPU_UCODE_ID ucode_id);
> +int amdgpu_vcn_save_fw_shared_region(struct amdgpu_device *adev);
>   
>   #endif