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

[Lazar, Lijo]

On 10/16/2024 9:37 PM, Liu, Leo wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig at amd.com>
>> Sent: October 16, 2024 9:16 AM
>> To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org; Liu, Leo
>> <Leo.Liu at amd.com>
>> Cc: Zhang, Hawking <Hawking.Zhang at amd.com>; Deucher, Alexander
>> <Alexander.Deucher at amd.com>; Sundararaju, Sathishkumar
>> <Sathishkumar.Sundararaju at amd.com>; Jiang, Sonny
>> <Sonny.Jiang at amd.com>; Zhou, Hao (Claire) <Hao.Zhou at amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: Save VCN shared memory with init reset
>>
>> 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?
>>
> 
> We need to keep some of the status from share memory(driver and FW), since some of them are changed by FW, that is why the init cannot be in the hw_init stage with suspend/resume case.
> 

For vcn_v4_0_3, the flags that are initialized in sw_init are these

                fw_shared->present_flag_0 =
cpu_to_le32(AMDGPU_FW_SHARED_FLAG_0_UNIFIED_QUEUE);
                fw_shared->sq.is_enabled = true;

                if (amdgpu_vcnfw_log)
                        amdgpu_vcn_fwlog_init(&adev->vcn.inst[i]);

Is the flags initialized during sw_init required for FW during its
initialization stage? If not, it would be better to move this to hw_init.

Some part also get modified during vcn_v4_0_3_start (which is after
hw_init during runtime) -

                fw_shared->sq.queue_mode &=
                        cpu_to_le32(~(FW_QUEUE_RING_RESET |
FW_QUEUE_DPG_HOLD_OFF));


One reason probably is hw_init is also called during resume which
restores the saved bo during suspend. So this may be to avoid the double
work.

Anyway, is the patch okay to go?

Thanks,
Lijo

>  +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);
> The saved bo here is not only for fw shared memory, also for FW runtime stack/heap as well.
> > Regards,
> Leo
> 
>  > +}
>  > +