[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.
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
> >
> > > +}
> > > +
