RE: [PATCH] drm/amdgpu: Need to set the baco cap before baco reset

["Deng, Emily" <Emily.Deng-5C7GfCeVMHo@public.gmane.org>]

>-----Original Message-----
>From: Alex Deucher <alexdeucher@gmail.com>
>Sent: Saturday, May 25, 2019 12:59 AM
>To: Deng, Emily <Emily.Deng@amd.com>
>Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>Subject: Re: [PATCH] drm/amdgpu: Need to set the baco cap before baco
>reset
>
>[CAUTION: External Email]
>
>On Thu, May 23, 2019 at 10:29 PM Deng, Emily <Emily.Deng@amd.com> wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Alex Deucher <alexdeucher@gmail.com>
>> >Sent: Friday, May 24, 2019 12:09 AM
>> >To: Deng, Emily <Emily.Deng@amd.com>
>> >Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>> >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@amd.com>
>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@amd.com>
>> >> ---
>> >>  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
Thanks, will modify the change as your suggestion.

Best wishes
Emily Deng


>
>> >
>> >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@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx