> This means we should either separate filling the BO from allocating it (e.g. > split amdgpu_ucode_init_bo into two functions) and then only call the filling > function during GPU reset and resume. Got it. Thanks. I will refine this function. Regards Rex > -----Original Message----- > From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx> > Sent: Tuesday, October 9, 2018 2:22 AM > To: Zhu, Rex <Rex.Zhu@xxxxxxx>; Koenig, Christian > <Christian.Koenig@xxxxxxx>; Deucher, Alexander > <Alexander.Deucher@xxxxxxx>; Alex Deucher <alexdeucher@xxxxxxxxx> > Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when > suspend > > Am 08.10.2018 um 20:15 schrieb Zhu, Rex: > > > >> -----Original Message----- > >> From: Koenig, Christian > >> Sent: Tuesday, October 9, 2018 2:03 AM > >> To: Zhu, Rex <Rex.Zhu@xxxxxxx>; Deucher, Alexander > >> <Alexander.Deucher@xxxxxxx>; Alex Deucher <alexdeucher@xxxxxxxxx> > >> Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > >> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when > >> suspend > >> > >> Am 08.10.2018 um 19:58 schrieb Zhu, Rex: > >>>> -----Original Message----- > >>>> From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx> > >>>> Sent: Tuesday, October 9, 2018 1:32 AM > >>>> To: Zhu, Rex <Rex.Zhu@xxxxxxx>; Deucher, Alexander > >>>> <Alexander.Deucher@xxxxxxx>; Alex Deucher > <alexdeucher@xxxxxxxxx> > >>>> Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > >>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when > >>>> suspend > >>>> > >>>> Am 08.10.2018 um 18:30 schrieb Zhu, Rex: > >>>>>> -----Original Message----- > >>>>>> From: Deucher, Alexander > >>>>>> Sent: Tuesday, October 9, 2018 12:21 AM > >>>>>> To: Zhu, Rex <Rex.Zhu@xxxxxxx>; Alex Deucher > >>>> <alexdeucher@xxxxxxxxx> > >>>>>> Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > >>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo > >>>>>> when suspend > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf > >> Of > >>>>>>> Zhu, Rex > >>>>>>> Sent: Monday, October 8, 2018 11:57 AM > >>>>>>> To: Alex Deucher <alexdeucher@xxxxxxxxx> > >>>>>>> Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > >>>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo > >>>>>>> when suspend > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: Alex Deucher <alexdeucher@xxxxxxxxx> > >>>>>>>> Sent: Thursday, October 4, 2018 11:35 AM > >>>>>>>> To: Zhu, Rex <Rex.Zhu@xxxxxxx> > >>>>>>>> Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > >>>>>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo > >>>>>>>> when suspend > >>>>>>>> > >>>>>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu@xxxxxxx> > wrote: > >>>>>>>>> driver don't release the ucode memory when suspend. so don't > >>>>>>>>> need to allocate bo when resume back. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Rex Zhu <Rex.Zhu@xxxxxxx> > >>>>>>>>> --- > >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +- > >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c > >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c > >>>>>>>>> index 9878212..adfeb93 100644 > >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c > >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c > >>>>>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct > >>>>>> amdgpu_device > >>>>>>>> *adev) > >>>>>>>>> return 0; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> - if (!adev->in_gpu_reset) { > >>>>>>>>> + if (!adev->in_gpu_reset && !adev->in_suspend) { > >>>>>>>>> err = amdgpu_bo_create_kernel(adev, > >>>>>>>>> adev->firmware.fw_size, > >>>>>>>> PAGE_SIZE, > >>>>>>>>> amdgpu_sriov_vf(adev) ? > >>>>>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT, > >>>>>>>>> &adev->firmware.fw_buf, > >>>>>>>> Not sure if we support S3 in SR-IOV, but I think this will > >>>>>>>> break it because we'll lose vram contents and not re-init it. > >>>>>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV. > >>>>>>> > >>>>>>> But I still confused why this patch will break the suspend > >>>>>>> if in SRIOV > >>>> case? > >>>>>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu > >>>>>> reset or S3, the data is lost. GTT is retained since the OS manages > that. > >>>>> The gart table was unpinned when suspend.so don't need to create > >>>>> the bo > >>>> again. we still copy the ucode to the bo. > >>>>> And in baremetal, this function can return directly for S3. > >>>> That's irrelevant. > >>>> > >>>> The whole code is buggy since amdgpu_ucode_fini_bo() will drop the > >>>> BO independent if we are in reset or in suspend. > >>> We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset. > >> Yeah and exactly that's the bug which should be fixed instead. > > Sorry, I don't understand. > > Why we need to release the bo when suspend? > > In amdgpu, we create a bunch of bo(vram_scratch.robj, e.g), we only free > them when driver unload. > > You should not release the BO while suspending, but the > amdgpu_ucode_fini_bo() and amdgpu_ucode_init_bo() should be called > under the same conditions. > > This means we should either separate filling the BO from allocating it (e.g. > split amdgpu_ucode_init_bo into two functions) and then only call the filling > function during GPU reset and resume. > > Or we add the same "if (!adev->in_gpu_reset && !adev->in_suspend)" into > the amdgpu_ucode_fini_bo() function as well. > > I consider the first one more cleaner. > > Christian. > > > > > Rex > > > >> Christian. > >> > >>> Rex > >>> > >>>> The correct handling here is to remove the if all together and make > >>>> sure > >>>> amdgpu_bo_create_kernel() is ALWAYS called. > >>>> > >>>> Cause then it is always re-created if it isn't there already. > >>>> > >>>> Alternatively we could fix up the callers of amdgpu_ucode_init_bo() > >>>> and > >>>> amdgpu_ucode_fini_bo() to be correctly balanced. > >>>> > >>>> Christian. > >>>> > >>>>> Rex > >>>>> > >>>>> > >>>>>> Alex > >>>>>> > >>>>>>> Rex > >>>>>>> > >>>>>>>> Alex > >>>>>>>> > >>>>>>>>> -- > >>>>>>>>> 1.9.1 > >>>>>>>>> > >>>>>>>>> _______________________________________________ > >>>>>>>>> amd-gfx mailing list > >>>>>>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx > >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>>>>>> _______________________________________________ > >>>>>>> amd-gfx mailing list > >>>>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx > >>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>>>> _______________________________________________ > >>>>> amd-gfx mailing list > >>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx > >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx