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@xxxxxxx>
>> Sent: October 16, 2024 11:18 PM
>> To: Liu, Leo <Leo.Liu@xxxxxxx>; Koenig, Christian
>> <Christian.Koenig@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Deucher, Alexander
>> <Alexander.Deucher@xxxxxxx>; Sundararaju, Sathishkumar
>> <Sathishkumar.Sundararaju@xxxxxxx>; Jiang, Sonny
>> <Sonny.Jiang@xxxxxxx>; Zhou, Hao (Claire) <Hao.Zhou@xxxxxxx>
>> 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@xxxxxxx>
>>>> Sent: October 16, 2024 9:16 AM
>>>> To: Lazar, Lijo <Lijo.Lazar@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
>>>> Liu, Leo <Leo.Liu@xxxxxxx>
>>>> Cc: Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Deucher, Alexander
>>>> <Alexander.Deucher@xxxxxxx>; Sundararaju, Sathishkumar
>>>> <Sathishkumar.Sundararaju@xxxxxxx>; Jiang, Sonny
>>>> <Sonny.Jiang@xxxxxxx>; Zhou, Hao (Claire) <Hao.Zhou@xxxxxxx>
>>>> 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
>>>
>>> > +}
>>> > +
