<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, "EmojiFont", "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> Sounds good to me. Alex <div style="color: rgb(0, 0, 0);"> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhu, Rex <Rex.Zhu@amd.com> Sent: Tuesday, October 9, 2018 9:44 PM To: Alex Deucher Cc: amd-gfx list Subject: RE: [PATCH 3/5] drm/amdgpu: Extract the function of fw loading out of powerplay <div> </div> </div> <div class="BodyFragment"> <div class="PlainText"> > -----Original Message----- > From: Alex Deucher <alexdeucher@gmail.com> > Sent: Wednesday, October 10, 2018 3:28 AM > To: Zhu, Rex <Rex.Zhu@amd.com> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org> > Subject: Re: [PATCH 3/5] drm/amdgpu: Extract the function of fw loading out > of powerplay > > On Tue, Oct 9, 2018 at 8:45 AM Rex Zhu <Rex.Zhu@amd.com> wrote: > > > > So there is no dependence between gfx/sdma/smu. > > and for Vi, after IH hw_init, driver load all the smu/gfx/sdma fw. for > > AI, fw loading is controlled by PSP, after psp hw init, we call the > > function to check smu fw version. > > > > Signed-off-by: Rex Zhu <Rex.Zhu@amd.com> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 30 > ++++++++++++++++++++++ > > drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 11 -------- > > drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 8 ------ > > drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 20 --------------- > > drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 - > > drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 8 ++---- > > drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c | 5 ---- > > 7 files changed, 32 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > index 4787571..a6766b3 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > @@ -1525,6 +1525,24 @@ static int amdgpu_device_ip_early_init(struct > amdgpu_device *adev) > > return 0; > > } > > > > +static int amdgpu_device_fw_loading(struct amdgpu_device *adev, > > +uint32_t index) { > > + int r = 0; > > + > > + if ((adev->asic_type < CHIP_VEGA10 > > + && (adev->ip_blocks[index].version->type == > AMD_IP_BLOCK_TYPE_IH)) > > + || (adev->asic_type >= CHIP_VEGA10 > > + && (adev->ip_blocks[index].version->type == > > + AMD_IP_BLOCK_TYPE_PSP))) { > > This seems kind of fragile. If we change the order again at some point, it will > break. How about we check whether hw_init/resume is done or not on the > blocks we care about or move the checks into the callers and only call when > we need it? Hi Alex, How about split hw_init to hw_init_phase1 and hw_init_phase2 as resume? We loaded fw(call psp_hw_init and start_smu) between phase1 and phase2. Regards Rex > > + if (adev->powerplay.pp_funcs->load_firmware) { > > + r = adev->powerplay.pp_funcs->load_firmware(adev- > >powerplay.pp_handle); > > + if (r) { > > + pr_err("firmware loading failed\n"); > > + return r; > > + } > > + } > > + } > > + return 0; > > +} > > /** > > * amdgpu_device_ip_init - run init for hardware IPs > > * > > @@ -1595,6 +1613,9 @@ static int amdgpu_device_ip_init(struct > amdgpu_device *adev) > > return r; > > } > > adev->ip_blocks[i].status.hw = true; > > + r = amdgpu_device_fw_loading(adev, i); > > + if (r) > > + return r; > > } > > > > amdgpu_xgmi_add_device(adev); > > @@ -2030,6 +2051,9 @@ static int > amdgpu_device_ip_reinit_early_sriov(struct amdgpu_device *adev) > > DRM_INFO("RE-INIT: %s %s\n", block->version->funcs->name, > r?"failed":"succeeded"); > > if (r) > > return r; > > + r = amdgpu_device_fw_loading(adev, i); > > + if (r) > > + return r; > > } > > } > > > > @@ -2098,6 +2122,9 @@ static int > amdgpu_device_ip_resume_phase1(struct amdgpu_device *adev) > > adev->ip_blocks[i].version->funcs->name, r); > > return r; > > } > > + r = amdgpu_device_fw_loading(adev, i); > > + if (r) > > + return r; > > } > > } > > > > @@ -2134,6 +2161,9 @@ static int > amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev) > > adev->ip_blocks[i].version->funcs->name, r); > > return r; > > } > > + r = amdgpu_device_fw_loading(adev, i); > > + if (r) > > + return r; > > } > > > > return 0; > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > index 8439f9a..3d0f277 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c > > @@ -4175,20 +4175,9 @@ static void gfx_v8_0_rlc_start(struct > > amdgpu_device *adev) > > > > static int gfx_v8_0_rlc_resume(struct amdgpu_device *adev) { > > - int r; > > - > > gfx_v8_0_rlc_stop(adev); > > gfx_v8_0_rlc_reset(adev); > > gfx_v8_0_init_pg(adev); > > - > > - if (adev->powerplay.pp_funcs->load_firmware) { > > - r = adev->powerplay.pp_funcs->load_firmware(adev- > >powerplay.pp_handle); > > - if (r) { > > - pr_err("firmware loading failed\n"); > > - return r; > > - } > > - } > > - > > gfx_v8_0_rlc_start(adev); > > > > return 0; > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > > b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > > index 0bdde7f..6fb3eda 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > > @@ -788,14 +788,6 @@ static int sdma_v3_0_start(struct amdgpu_device > > *adev) { > > int r; > > > > - if (adev->powerplay.pp_funcs->load_firmware) { > > - r = adev->powerplay.pp_funcs->load_firmware(adev- > >powerplay.pp_handle); > > - if (r) { > > - pr_err("firmware loading failed\n"); > > - return r; > > - } > > - } > > - > > /* disable sdma engine before programing it */ > > sdma_v3_0_ctx_switch_enable(adev, false); > > sdma_v3_0_enable(adev, false); diff --git > > a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c > > b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c > > index d552af2..47ac923 100644 > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c > > @@ -89,7 +89,6 @@ int hwmgr_early_init(struct pp_hwmgr *hwmgr) > > hwmgr_init_default_caps(hwmgr); > > hwmgr_set_user_specify_caps(hwmgr); > > hwmgr->fan_ctrl_is_in_default_mode = true; > > - hwmgr->reload_fw = 1; > > hwmgr_init_workload_prority(hwmgr); > > > > switch (hwmgr->chip_family) { > > @@ -209,17 +208,6 @@ int hwmgr_hw_init(struct pp_hwmgr *hwmgr) { > > int ret = 0; > > > > - if (!hwmgr || !hwmgr->smumgr_funcs) > > - return -EINVAL; > > - > > - if (hwmgr->smumgr_funcs->start_smu) { > > - ret = hwmgr->smumgr_funcs->start_smu(hwmgr); > > - if (ret) { > > - pr_err("smc start failed\n"); > > - return -EINVAL; > > - } > > - } > > - > > if (!hwmgr->pm_en) > > return 0; > > > > @@ -301,7 +289,6 @@ int hwmgr_suspend(struct pp_hwmgr *hwmgr) > > if (!hwmgr || !hwmgr->pm_en) > > return 0; > > > > - hwmgr->reload_fw = true; > > phm_disable_smc_firmware_ctf(hwmgr); > > ret = psm_set_boot_states(hwmgr); > > if (ret) > > @@ -321,13 +308,6 @@ int hwmgr_resume(struct pp_hwmgr *hwmgr) > > if (!hwmgr) > > return -EINVAL; > > > > - if (hwmgr->smumgr_funcs && hwmgr->smumgr_funcs->start_smu) { > > - if (hwmgr->smumgr_funcs->start_smu(hwmgr)) { > > - pr_err("smc start failed\n"); > > - return -EINVAL; > > - } > > - } > > - > > if (!hwmgr->pm_en) > > return 0; > > > > diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h > > b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h > > index 35f2272..e5a60aa 100644 > > --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h > > +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h > > @@ -734,7 +734,6 @@ struct pp_hwmgr { > > void *smu_backend; > > const struct pp_smumgr_func *smumgr_funcs; > > bool is_kicker; > > - bool reload_fw; > > > > enum PP_DAL_POWERLEVEL dal_power_level; > > struct phm_dynamic_state_info dyn_state; diff --git > > a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c > > b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c > > index 99b4e4f..3f51d54 100644 > > --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c > > +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c > > @@ -343,9 +343,6 @@ int smu7_request_smu_load_fw(struct pp_hwmgr > *hwmgr) > > uint32_t fw_to_load; > > int r = 0; > > > > - if (!hwmgr->reload_fw) > > - return 0; > > - > > amdgpu_ucode_init_bo(hwmgr->adev); > > > > if (smu_data->soft_regs_start) @@ -432,10 +429,9 @@ int > > smu7_request_smu_load_fw(struct pp_hwmgr *hwmgr) > > smu7_send_msg_to_smc_with_parameter(hwmgr, > > PPSMC_MSG_LoadUcodes, fw_to_load); > > > > r = smu7_check_fw_load_finish(hwmgr, fw_to_load); > > - if (!r) { > > - hwmgr->reload_fw = 0; > > + if (!r) > > return 0; > > - } > > + > > pr_err("SMU load firmware failed\n"); > > > > failed: > > diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c > > b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c > > index abbf2f2..f836d30 100644 > > --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c > > +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c > > @@ -661,9 +661,6 @@ static int smu8_request_smu_load_fw(struct > pp_hwmgr *hwmgr) > > uint32_t fw_to_check = 0; > > int ret; > > > > - if (!hwmgr->reload_fw) > > - return 0; > > - > > amdgpu_ucode_init_bo(hwmgr->adev); > > > > smu8_smu_populate_firmware_entries(hwmgr); > > @@ -719,8 +716,6 @@ static int smu8_request_smu_load_fw(struct > pp_hwmgr *hwmgr) > > return ret; > > } > > > > - hwmgr->reload_fw = 0; > > - > > return 0; > > } > > > > -- > > 1.9.1 > > > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk554620" class="OWAAutoLink" previewremoved="true"> https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk111319" class="OWAAutoLink" previewremoved="true">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </div> </div> </div> </div> </body> </html>