[PATCH] drm/amd/pm: validate SMU feature enable message for getting feature enabled mask

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Friday, February 18, 2022 1:29 PM
> To: Liang, Prike <Prike.Liang at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan
> <Evan.Quan at amd.com>; Huang, Ray <Ray.Huang at amd.com>
> Subject: Re: [PATCH] drm/amd/pm: validate SMU feature enable message for
> getting feature enabled mask
>
>
>
> On 2/18/2022 9:57 AM, Liang, Prike wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> >> Sent: Friday, February 18, 2022 12:05 PM
> >> To: Liang, Prike <Prike.Liang at amd.com>; amd-gfx at lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan
> >> <Evan.Quan at amd.com>; Huang, Ray <Ray.Huang at amd.com>
> >> Subject: Re: [PATCH] drm/amd/pm: validate SMU feature enable message
> >> for getting feature enabled mask
> >>
> >>
> >>
> >> On 2/18/2022 9:25 AM, Prike Liang wrote:
> >>> There's always miss the SMU feature enabled checked in the NPI
> >>> phase, so let validate the SMU feature enable message directly
> >>> rather than add more and more MP1 version check.
> >>>
> >>> Signed-off-by: Prike Liang <Prike.Liang at amd.com>
> >>> ---
> >>>    drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 28
> >>> ++++++-------------------
> >> -
> >>>    1 file changed, 6 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> index f24111d28290..da1ac70ed455 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> @@ -552,10 +552,9 @@ bool smu_cmn_clk_dpm_is_enabled(struct
> >> smu_context *smu,
> >>>    int smu_cmn_get_enabled_mask(struct smu_context *smu,
> >>>                           uint64_t *feature_mask)
> >>>    {
> >>> -   struct amdgpu_device *adev = smu->adev;
> >>>      uint32_t *feature_mask_high;
> >>>      uint32_t *feature_mask_low;
> >>> -   int ret = 0;
> >>> +   int ret = 0, index = 0;
> >>>
> >>>      if (!feature_mask)
> >>>              return -EINVAL;
> >>> @@ -563,12 +562,10 @@ int smu_cmn_get_enabled_mask(struct
> >> smu_context *smu,
> >>>      feature_mask_low = &((uint32_t *)feature_mask)[0];
> >>>      feature_mask_high = &((uint32_t *)feature_mask)[1];
> >>>
> >>> -   switch (adev->ip_versions[MP1_HWIP][0]) {
> >>> -   /* For Vangogh and Yellow Carp */
> >>> -   case IP_VERSION(11, 5, 0):
> >>> -   case IP_VERSION(13, 0, 1):
> >>> -   case IP_VERSION(13, 0, 3):
> >>> -   case IP_VERSION(13, 0, 8):
> >>> +   index = smu_cmn_to_asic_specific_index(smu,
> >>> +                                           CMN2ASIC_MAPPING_MSG,
> >>> +
> >>        SMU_MSG_GetEnabledSmuFeatures);
> >>> +   if (index > 0) {
> >>>              ret = smu_cmn_send_smc_msg_with_param(smu,
> >>>
> >> SMU_MSG_GetEnabledSmuFeatures,
> >>>                                                    0, @@ -580,19
> >>> +577,7 @@ int smu_cmn_get_enabled_mask(struct
> >> smu_context *smu,
> >>>
> >> SMU_MSG_GetEnabledSmuFeatures,
> >>>                                                    1,
> >>>                                                    feature_mask_high);
> >>> -           break;
> >>> -   /*
> >>> -    * For Cyan Skillfish and Renoir, there is no interface provided by
> >> PMFW
> >>> -    * to retrieve the enabled features. So, we assume all features are
> >> enabled.
> >>> -    * TODO: add other APU ASICs which suffer from the same issue here
> >>> -    */
> >>> -   case IP_VERSION(11, 0, 8):
> >>> -   case IP_VERSION(12, 0, 0):
> >>> -   case IP_VERSION(12, 0, 1):
> >>> -           memset(feature_mask, 0xff, sizeof(*feature_mask));
> >>> -           break;
> >>
> >> This is broken now as these ASICs don't support any message. It is
> >> best to take out smu_cmn_get_enabled_mask altogether and move to
> >> smu_v*.c or *_ppt.c as this is a callback function.
> >>
> >> Thanks,
> >> Lijo
> >>
> > Before this solution I also consider put the  smu_cmn_get_enabled_mask
> implementation in each *_ppt directly, but seems need some effort for
> implementing on each *_ppt. How about we also check the
> SMU_MSG_GetEnabledSmuFeaturesHigh mapping index? As to the ASCI not
> support neither  SMU_MSG_GetEnabledSmuFeatures nor
> SMU_MSG_GetEnabledSmuFeaturesHigh will hard code the feature mask in
> this case.
> >
>
> Something like attached refactoring and then for the rest you could apply
> your patch.
>
> Thanks,
> Lijo
>
Well, this also looks good for handling the feature mask not support scenario case by case, before this I want to handle everything on the smu_cmn_get_enabled_mask(). Will update the patch base on your proposal, thanks!


> >>> -   /* other dGPU ASICs */
> >>> -   default:
> >>> +   } else {
> >>>              ret = smu_cmn_send_smc_msg(smu,
> >>>
> >> SMU_MSG_GetEnabledSmuFeaturesHigh,
> >>>                                         feature_mask_high); @@
> >>> -602,7 +587,6 @@ int smu_cmn_get_enabled_mask(struct
> >> smu_context *smu,
> >>>              ret = smu_cmn_send_smc_msg(smu,
> >>>
> >> SMU_MSG_GetEnabledSmuFeaturesLow,
> >>>                                         feature_mask_low);
> >>> -           break;
> >>>      }
> >>>
> >>>      return ret;
> >>>