Re: [PATCH 3/5] drm/amdgpu: Extract the function of fw loading out of powerplay
Sounds good to me.
Alex
From: amd-gfx on behalf of Zhu, Rex
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
> Sent: Wednesday, October 10, 2018 3:28 AM
> To: Zhu, Rex
> Cc: amd-gfx list
> 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 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
> > ---
> > 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)
> > +
RE: [PATCH 3/5] drm/amdgpu: Extract the function of fw loading out of powerplay
> -Original Message-
> From: Alex Deucher
> Sent: Wednesday, October 10, 2018 3:28 AM
> To: Zhu, Rex
> Cc: amd-gfx list
> 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 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
> > ---
> > 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)
> > a
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 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
> ---
> 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?
> + 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
