[PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

Zhu, Rex Rex.Zhu at amd.com
Mon Oct 8 18:37:43 UTC 2018 

> 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 at gmail.com>
> Sent: Tuesday, October 9, 2018 2:22 AM
> To: Zhu, Rex <Rex.Zhu at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; Alex Deucher <alexdeucher at gmail.com>
> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> 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 at amd.com>; Deucher, Alexander
> >> <Alexander.Deucher at amd.com>; Alex Deucher <alexdeucher at gmail.com>
> >> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> >> 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 at gmail.com>
> >>>> Sent: Tuesday, October 9, 2018 1:32 AM
> >>>> To: Zhu, Rex <Rex.Zhu at amd.com>; Deucher, Alexander
> >>>> <Alexander.Deucher at amd.com>; Alex Deucher
> <alexdeucher at gmail.com>
> >>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> >>>> 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 at amd.com>; Alex Deucher
> >>>> <alexdeucher at gmail.com>
> >>>>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> >>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>>> when suspend
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf
> >> Of
> >>>>>>> Zhu, Rex
> >>>>>>> Sent: Monday, October 8, 2018 11:57 AM
> >>>>>>> To: Alex Deucher <alexdeucher at gmail.com>
> >>>>>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> >>>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo
> >>>>>>> when suspend
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Alex Deucher <alexdeucher at gmail.com>
> >>>>>>>> Sent: Thursday, October 4, 2018 11:35 AM
> >>>>>>>> To: Zhu, Rex <Rex.Zhu at amd.com>
> >>>>>>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> >>>>>>>> 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 at amd.com>
> 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 at amd.com>
> >>>>>>>>> ---
> >>>>>>>>>     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 at lists.freedesktop.org
> >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>>>>>> _______________________________________________
> >>>>>>> amd-gfx mailing list
> >>>>>>> amd-gfx at lists.freedesktop.org
> >>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>>>> _______________________________________________
> >>>>> amd-gfx mailing list
> >>>>> amd-gfx at lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

More information about the amd-gfx mailing list