[PATCH 3/5] drm/amdgpu: Extract the function of fw loading out of powerplay
Deucher, Alexander
Alexander.Deucher at amd.com
Wed Oct 10 14:44:38 UTC 2018
Sounds good to me.
Alex
________________________________
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of Zhu, Rex <Rex.Zhu at 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
> -----Original Message-----
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Wednesday, October 10, 2018 3:28 AM
> To: Zhu, Rex <Rex.Zhu at amd.com>
> Cc: amd-gfx list <amd-gfx at 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 at 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 at 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 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20181010/d52f37f4/attachment-0001.html>
More information about the amd-gfx
mailing list