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

[Lazar, Lijo]

On 10/17/2024 6:37 PM, Liu, Leo wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: October 16, 2024 11:18 PM
>> To: Liu, Leo <Leo.Liu at amd.com>; Koenig, Christian
>> <Christian.Koenig at amd.com>; amd-gfx at lists.freedesktop.org
>> 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
>>
>>
>>
>> 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?
>>
> You need to fix the function name as I commented from last email.
> 

Any suggestions - amdgpu_vcn_save_vcpu_buffer/bo?

Thanks,
Lijo

> Regards,
> Leo
> 
> 
>> 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
>>>
>>>  > +}
>>>  > +