[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: October 17, 2024 9:17 AM
> 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/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?
>
"amdgpu_vcn_save_vcpu_bo" should be good.
Leo
> 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
> >>>
> >>> > +}
> >>> > +
