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

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

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