On Thu, May 23, 2019 at 10:29 PM Deng, Emily <Emily.Deng@xxxxxxx> wrote:
>
>
>
> >-----Original Message-----
> >From: Alex Deucher <alexdeucher@xxxxxxxxx>
> >Sent: Friday, May 24, 2019 12:09 AM
> >To: Deng, Emily <Emily.Deng@xxxxxxx>
> >Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
> >Subject: Re: [PATCH] drm/amdgpu: Need to set the baco cap before baco
> >reset
> >
> >[CAUTION: External Email]
> >
> >On Thu, May 23, 2019 at 6:22 AM Emily Deng <Emily.Deng@xxxxxxx> wrote:
> >>
> >> For passthrough, after rebooted the VM, driver will do a baco reset
> >> before doing other driver initialization during loading driver. For
> >> doing the baco reset, it will first check the baco reset capability.
> >> So first need to set the cap from the vbios information or baco reset
> >> won't be enabled.
> >>
> >> Signed-off-by: Emily Deng <Emily.Deng@xxxxxxx>
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++++++
> >> drivers/gpu/drm/amd/amdgpu/soc15.c | 3 ++-
> >> drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
> >> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
> >+++++++++++++++
> >> drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
> >> .../amd/powerplay/hwmgr/vega10_processpptables.c | 24
> >++++++++++++++++++++++
> >> .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
> >> drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
> >> 8 files changed, 54 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index bdd1fe73..2dde672 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -2611,6 +2611,14 @@ int amdgpu_device_init(struct amdgpu_device
> >*adev,
> >> * E.g., driver was not cleanly unloaded previously, etc.
> >> */
> >> if (!amdgpu_sriov_vf(adev) &&
> >> amdgpu_asic_need_reset_on_init(adev)) {
> >> + if (amdgpu_passthrough(adev) && adev->powerplay.pp_funcs &&
> >adev->powerplay.pp_funcs->set_asic_baco_cap) {
> >> + r = adev->powerplay.pp_funcs->set_asic_baco_cap(adev-
> >>powerplay.pp_handle);
> >> + if (r) {
> >> + dev_err(adev->dev, "set baco capability failed\n");
> >> + goto failed;
> >> + }
> >> + }
> >> +
> >
> >I think it would be cleaner to add this to hwmgr_early_init() or something
> >called from early init for powerplay.
> I also preferred to put it in the hwmgr_early_init, but as the function set_asic_baco_cap need to get the vbios info, so need to put the amdgpu_get_bios before early init. If so the code changes too big.
I think this change is all you need:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index bdd1fe73f14b..952f61e28d42 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2564,6 +2564,12 @@ int amdgpu_device_init(struct amdgpu_device *adev,
amdgpu_device_get_pcie_info(adev);
+ /* Read BIOS */
+ if (!amdgpu_get_bios(adev)) {
+ r = -EINVAL;
+ goto failed;
+ }
+
/* early init functions */
r = amdgpu_device_ip_early_init(adev);
if (r)
@@ -2591,12 +2597,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
goto fence_driver_init;
}
- /* Read BIOS */
- if (!amdgpu_get_bios(adev)) {
- r = -EINVAL;
- goto failed;
- }
-
r = amdgpu_atombios_init(adev);
if (r) {
dev_err(adev->dev, "amdgpu_atombios_init failed\n");
I guess that could be a follow up change if you want.
Alex
> >
> >Alex
> >
> >> r = amdgpu_asic_reset(adev);
> >> if (r) {
> >> dev_err(adev->dev, "asic reset on init
> >> failed\n"); diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
> >> b/drivers/gpu/drm/amd/amdgpu/soc15.c
> >> index 78bd4fc..d9fdd95 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> >> @@ -764,7 +764,8 @@ static bool soc15_need_reset_on_init(struct
> >amdgpu_device *adev)
> >> /* Just return false for soc15 GPUs. Reset does not seem to
> >> * be necessary.
> >> */
> >> - return false;
> >> + if (!amdgpu_passthrough(adev))
> >> + return false;
> >>
> >> if (adev->flags & AMD_IS_APU)
> >> return false;
> >> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> >> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> >> index 9f661bf..c6e2a51 100644
> >> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> >> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> >> @@ -296,6 +296,7 @@ struct amd_pm_funcs {
> >> int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
> >> int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
> >> int (*get_asic_baco_capability)(void *handle, bool *cap);
> >> + int (*set_asic_baco_cap)(void *handle);
> >> int (*get_asic_baco_state)(void *handle, int *state);
> >> int (*set_asic_baco_state)(void *handle, int state);
> >> int (*get_ppfeature_status)(void *handle, char *buf); diff
> >> --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> >> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> >> index bea1587..9856760 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> >> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> >> @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void
> >*handle, uint32_t count)
> >> return ret;
> >> }
> >>
> >> +static int pp_set_asic_baco_cap(void *handle) {
> >> + struct pp_hwmgr *hwmgr = handle;
> >> +
> >> + if (!hwmgr)
> >> + return -EINVAL;
> >> +
> >> + if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
> >> + return 0;
> >> +
> >> + hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int pp_get_asic_baco_capability(void *handle, bool *cap) {
> >> struct pp_hwmgr *hwmgr = handle; @@ -1546,6 +1561,7 @@ static
> >> const struct amd_pm_funcs pp_dpm_funcs = {
> >> .set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
> >> .set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
> >> .get_asic_baco_capability = pp_get_asic_baco_capability,
> >> + .set_asic_baco_cap = pp_set_asic_baco_cap,
> >> .get_asic_baco_state = pp_get_asic_baco_state,
> >> .set_asic_baco_state = pp_set_asic_baco_state,
> >> .get_ppfeature_status = pp_get_ppfeature_status, diff --git
> >> a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> >> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> >> index ce6aeb5..e5bcbc8 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> >> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> >> @@ -5302,6 +5302,7 @@ static const struct pp_hwmgr_func
> >vega10_hwmgr_funcs = {
> >> .odn_edit_dpm_table = vega10_odn_edit_dpm_table,
> >> .get_performance_level = vega10_get_performance_level,
> >> .get_asic_baco_capability = smu9_baco_get_capability,
> >> + .set_asic_baco_cap = vega10_baco_set_cap,
> >> .get_asic_baco_state = smu9_baco_get_state,
> >> .set_asic_baco_state = vega10_baco_set_state,
> >> .enable_mgpu_fan_boost = vega10_enable_mgpu_fan_boost, diff
> >> --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
> >> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
> >> index b6767d7..83d22cd 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
> >> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
> >> @@ -1371,3 +1371,27 @@ int vega10_get_powerplay_table_entry(struct
> >> pp_hwmgr *hwmgr,
> >>
> >> return result;
> >> }
> >> +
> >> +int vega10_baco_set_cap(struct pp_hwmgr *hwmgr) {
> >> + int result = 0;
> >> +
> >> + const ATOM_Vega10_POWERPLAYTABLE *powerplay_table;
> >> +
> >> + powerplay_table = get_powerplay_table(hwmgr);
> >> +
> >> + PP_ASSERT_WITH_CODE((powerplay_table != NULL),
> >> + "Missing PowerPlay Table!", return -1);
> >> +
> >> + result = check_powerplay_tables(hwmgr, powerplay_table);
> >> +
> >> + PP_ASSERT_WITH_CODE((result == 0),
> >> + "check_powerplay_tables failed", return
> >> + result);
> >> +
> >> + set_hw_cap(
> >> + hwmgr,
> >> + 0 != (le32_to_cpu(powerplay_table->ulPlatformCaps) &
> >ATOM_VEGA10_PP_PLATFORM_CAP_BACO),
> >> + PHM_PlatformCaps_BACO);
> >> + return result;
> >> +}
> >> +
> >> diff --git
> >> a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.h
> >> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.h
> >> index d83ed2a..da5fbec 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.h
> >> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.h
> >> @@ -59,4 +59,5 @@ extern int
> >> vega10_get_number_of_powerplay_table_entries(struct pp_hwmgr
> >*hwmgr); extern int vega10_get_powerplay_table_entry(struct pp_hwmgr
> >*hwmgr, uint32_t entry_index,
> >> struct pp_power_state *power_state, int (*call_back_func)(struct
> >pp_hwmgr *, void *,
> >> struct pp_power_state *, void *,
> >> uint32_t));
> >> +extern int vega10_baco_set_cap(struct pp_hwmgr *hwmgr);
> >> #endif
> >> diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> >> b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> >> index bac3d85..14480ae 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> >> +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> >> @@ -339,6 +339,7 @@ struct pp_hwmgr_func {
> >> int (*set_hard_min_dcefclk_by_freq)(struct pp_hwmgr *hwmgr,
> >uint32_t clock);
> >> int (*set_hard_min_fclk_by_freq)(struct pp_hwmgr *hwmgr, uint32_t
> >clock);
> >> int (*get_asic_baco_capability)(struct pp_hwmgr *hwmgr, bool
> >> *cap);
> >> + int (*set_asic_baco_cap)(struct pp_hwmgr *hwmgr);
> >> int (*get_asic_baco_state)(struct pp_hwmgr *hwmgr, enum
> >BACO_STATE *state);
> >> int (*set_asic_baco_state)(struct pp_hwmgr *hwmgr, enum
> >BACO_STATE state);
> >> int (*get_ppfeature_status)(struct pp_hwmgr *hwmgr, char
> >> *buf);
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> 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
