On Thu, Jul 25, 2019 at 10:20 PM Quan, Evan <Evan.Quan@xxxxxxx> wrote:
>
> Patch1 - patch11: Reviewed-by: Evan Quan <evan.quan@xxxxxxx>
>
> For patch12, comment inline
>
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Alex
> > Deucher
> > Sent: Friday, July 26, 2019 12:58 AM
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> > Subject: [PATCH 12/12] drm/amdgpu: put the SMC into the proper state on
> > suspend
> >
> > Suspend is used for S3/S4, GPU reset, and PCI shutdown. In each case, we
> > need to put the SMC into the proper state in order to resume or reload
> > correctly.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33
> > ++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 4425ff06ecc4..bb4260648a97 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2174,6 +2174,39 @@ static int
> > amdgpu_device_ip_suspend_phase2(struct amdgpu_device *adev)
> > DRM_ERROR("suspend of IP block <%s> failed %d\n",
> > adev->ip_blocks[i].version->funcs->name,
> > r);
> > }
> > + /* handle putting the SMC in the appropriate state */
> > + if (adev->ip_blocks[i].version->type ==
> > AMD_IP_BLOCK_TYPE_SMC) {
> > + enum pp_mp1_state mp1_state =
> > PP_MP1_STATE_NONE;
> > +
> > + if (adev->in_gpu_reset) {
> > + switch (amdgpu_asic_reset_method(adev)) {
> > + case AMD_RESET_METHOD_MODE1:
> > + case AMD_RESET_METHOD_BACO:
> > + mp1_state =
> > PP_MP1_STATE_SHUTDOWN;
> [Quan, Evan] For AMD_RESET_METHOD_BACO, it should be PP_MP1_STATE_UNLOAD.
Ok. I thought you had said shutdown before, but see my comment below
about BACO.
> > + break;
> > + case AMD_RESET_METHOD_MODE2:
> > + mp1_state = PP_MP1_STATE_RESET;
> > + break;
> > + default:
> > + mp1_state = PP_MP1_STATE_NONE;
> > + break;
> > + }
> > + } else if (adev->in_gpu_shutdown) {
> > + mp1_state = PP_MP1_STATE_UNLOAD;
> > + }
> [Quan, Evan] Handling for suspend only case seems missing.
Do you know what state we should use for suspend? Do we even need to
a case for suspend? Putting the chip into D3 might be enough as is.
> > + if (is_support_sw_smu(adev)) {
> > + /* todo */
> > + } else if (adev->powerplay.pp_funcs &&
> > + adev->powerplay.pp_funcs-
> > >set_mp1_state) {
> > + r = adev->powerplay.pp_funcs-
> > >set_mp1_state(
> > + adev->powerplay.pp_handle,
> > + mp1_state);
> > + if (r) {
> > + DRM_ERROR("SMC failed to set mp1
> > state %d, %d\n",
> > + mp1_state, r);
> > + }
> > + }
> > + }
> [Quan, Evan] Baco reset will be triggered in soc15_asic_reset. And there will be SMU message issued then and needs the SMU prepared.
> If we stall the SMU engine here(before soc15_asic_reset), we may fail to issue BACO messages.
Good point. Although thinking a bit more about how BACO works, I'm
wondering if we even need to do anything for BACO. We use the SMU to
bring the chip in and out of BACO so I'm not sure if it makes sense to
do anything for the BACO case.
Alex
> > }
> >
> > return 0;
> > --
> > 2.20.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
