[PATCH V2 4/5] drm/amd/pm: clear the cached dpm feature status
Lazar, Lijo
Lijo.Lazar at amd.com
Mon Jun 7 08:04:31 UTC 2021
[Public]
What about separating to smu_cmn_utils.c/smu_utils.c or similar which is meant for software based common/util functions? In general, it will have sw based common funcs (not ASIC specific) and may be used outside (for ex: in amdgpu_smu.c). smu_cmn continues to have the hw based common/ppt_func routines.
smu_cmn_to_asic_specific_index is also a good candidate for such a function as it fetches the sw lookup map (may be moved later).
Thanks,
Lijo
-----Original Message-----
From: Quan, Evan <Evan.Quan at amd.com>
Sent: Monday, June 7, 2021 1:04 PM
To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: RE: [PATCH V2 4/5] drm/amd/pm: clear the cached dpm feature status
[AMD Official Use Only]
Hi Lijo,
I got your concern. However, the problem is what amdgpu_smu.c can see is SMU_FEATURE_xxxxx_BIT(e.g. SMU_FEATURE_BACO_BIT) related.
While the bit mask stored in feature->enabled is FEATURE_xxxxx_BIT(e.g. FEATURE_BACO_BIT which is asic specific) related.
So, a SMU_FEATURE_BACO_BIT to FEATURE_BACO_BIT transfer is needed and for now this asic specific operation is performed in smu_cmn.c(smu_cmn_to_asic_specific_index).
The possible way to get this optimized I can see is either 1. try to fit this into smu_cmn_disable_all_features_with_exception()
2. make the bitmasks stored in feature->enabled are non asic specific ones. This will involve extra efforts and risks.
I personally prefer the approach 1 above.
Any thoughts?
BR
Evan
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Friday, June 4, 2021 7:56 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH V2 4/5] drm/amd/pm: clear the cached dpm feature
> status
>
>
>
> On 6/4/2021 3:28 PM, Evan Quan wrote:
> > For some ASICs, the real dpm feature disablement job is handled by
> > PMFW during baco reset and custom pptable loading. Cached dpm
> > feature status need to be cleared to pair that.
> >
> > Change-Id: I9e37d80e13599833301c04711b097fb37c2e41f9
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > ---
> > V1->V2:
> > - correct the setting for baco bit(pointed out by Lijo)
> > ---
> > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 6 +++++
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 19 ++++++++++++-
> -
> > .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 1 +
> > .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 1 +
> > drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 25
> +++++++++++++++++++
> > drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h | 3 +++
> > drivers/gpu/drm/amd/pm/swsmu/smu_internal.h | 1 +
> > 7 files changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index fb7438224872..3f9b2a839a37 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -937,6 +937,12 @@ struct pptable_funcs {
> > */
> > int (*disable_all_features_with_exception)(struct smu_context
> *smu,
> > enum smu_feature_mask mask);
> >
> > + /**
> > + * @clear_cached_dpm_feature_status_with_exception: Clear
> cached dpm feature status with
> > + * exception to those in &mask.
> > + */
> > + int (*clear_cached_dpm_feature_status_with_exception)(struct
> > +smu_context *smu, enum smu_feature_mask mask);
> > +
>
> Since this is a just a utility function which clears sw bitmask
> without touching hardware, do we need a ppt_func? In case that is
> needed, we already have this -
> smu_cmn_disable_all_features_with_exception. So, can't this remain as a standalone smu_cmn function which can be called from anywhere?
> Nowadays, I get a red alert when a ppt_func gets added :)
>
> Thanks,
> Lijo
>
> > /**
> > * @notify_display_change: Enable fast memory clock switching.
> > *
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index c9a31556e077..aba475d06507 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -1352,6 +1352,16 @@ static int smu_hw_init(void *handle)
> > return 0;
> > }
> >
> > +static void smu_reset_cached_dpm_feature_status(struct smu_context
> > +*smu) {
> > + struct smu_feature *feature = &smu->smu_feature;
> > +
> > + mutex_lock(&feature->mutex);
> > + bitmap_zero(feature->enabled, feature->feature_num);
> > + bitmap_zero(feature->supported, feature->feature_num);
> > + mutex_unlock(&feature->mutex);
> > +}
> > +
> > static int smu_disable_dpms(struct smu_context *smu)
> > {
> > struct amdgpu_device *adev = smu->adev; @@ -1374,16 +1384,21
> @@
> > static int smu_disable_dpms(struct smu_context *smu)
> > */
> > if (smu->uploading_custom_pp_table &&
> > (adev->asic_type >= CHIP_NAVI10) &&
> > - (adev->asic_type <= CHIP_DIMGREY_CAVEFISH))
> > + (adev->asic_type <= CHIP_DIMGREY_CAVEFISH)) {
> > + smu_reset_cached_dpm_feature_status(smu);
> > return 0;
> > + }
> >
> > /*
> > * For Sienna_Cichlid, PMFW will handle the features disablement
> properly
> > * on BACO in. Driver involvement is unnecessary.
> > */
> > if ((adev->asic_type == CHIP_SIENNA_CICHLID) &&
> > - use_baco)
> > + use_baco) {
> > +
> smu_clear_cached_dpm_feature_status_with_exception(smu,
> > +
> SMU_FEATURE_BACO_BIT);
> > return 0;
> > + }
> >
> > /*
> > * For gpu reset, runpm and hibernation through BACO, diff --git
> > a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > index afd4bd850c63..8c3ea20fb0f6 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > @@ -3083,6 +3083,7 @@ static const struct pptable_funcs
> navi10_ppt_funcs = {
> > .get_enabled_mask = smu_cmn_get_enabled_mask,
> > .feature_is_enabled = smu_cmn_feature_is_enabled,
> > .disable_all_features_with_exception =
> > smu_cmn_disable_all_features_with_exception,
> > + .clear_cached_dpm_feature_status_with_exception =
> > +smu_cmn_clear_cached_dpm_feature_status_with_exception,
> > .notify_display_change = smu_v11_0_notify_display_change,
> > .set_power_limit = smu_v11_0_set_power_limit,
> > .init_max_sustainable_clocks =
> > smu_v11_0_init_max_sustainable_clocks,
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> > index b4a2bb0f3f4c..14a14c30ee5e 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> > @@ -3886,6 +3886,7 @@ static const struct pptable_funcs
> sienna_cichlid_ppt_funcs = {
> > .get_enabled_mask = smu_cmn_get_enabled_mask,
> > .feature_is_enabled = smu_cmn_feature_is_enabled,
> > .disable_all_features_with_exception =
> > smu_cmn_disable_all_features_with_exception,
> > + .clear_cached_dpm_feature_status_with_exception =
> > +smu_cmn_clear_cached_dpm_feature_status_with_exception,
> > .notify_display_change = NULL,
> > .set_power_limit = smu_v11_0_set_power_limit,
> > .init_max_sustainable_clocks =
> > smu_v11_0_init_max_sustainable_clocks,
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > index 0ceb7329838c..b9c38abc9d37 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > @@ -594,6 +594,31 @@ int
> smu_cmn_disable_all_features_with_exception(struct smu_context *smu,
> > 0);
> > }
> >
> > +int smu_cmn_clear_cached_dpm_feature_status_with_exception(struct
> smu_context *smu,
> > + enum
> smu_feature_mask mask) {
> > + struct smu_feature *feature = &smu->smu_feature;
> > + uint64_t features_to_disable = U64_MAX;
> > + int skipped_feature_id;
> > +
> > + skipped_feature_id = smu_cmn_to_asic_specific_index(smu,
> > +
> CMN2ASIC_MAPPING_FEATURE,
> > + mask);
> > + if (skipped_feature_id < 0)
> > + return -EINVAL;
> > +
> > + features_to_disable &= ~(1ULL << skipped_feature_id);
> > +
> > + mutex_lock(&feature->mutex);
> > + bitmap_andnot(feature->enabled, feature->enabled,
> > + (unsigned long *)(&features_to_disable),
> SMU_FEATURE_MAX);
> > + bitmap_andnot(feature->supported, feature->supported,
> > + (unsigned long *)(&features_to_disable),
> SMU_FEATURE_MAX);
> > + mutex_unlock(&feature->mutex);
> > +
> > + return 0;
> > +}
> > +
> > int smu_cmn_get_smc_version(struct smu_context *smu,
> > uint32_t *if_version,
> > uint32_t *smu_version)
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > index da6ff6f024f9..12ad07132d15 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > @@ -77,6 +77,9 @@ int smu_cmn_set_pp_feature_mask(struct
> smu_context *smu,
> > int smu_cmn_disable_all_features_with_exception(struct smu_context
> *smu,
> > enum smu_feature_mask
> mask);
> >
> > +int smu_cmn_clear_cached_dpm_feature_status_with_exception(struct
> smu_context *smu,
> > + enum
> smu_feature_mask mask);
> > +
> > int smu_cmn_get_smc_version(struct smu_context *smu,
> > uint32_t *if_version,
> > uint32_t *smu_version);
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
> > index 33101dc93bcc..b12dcfab55fe 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
> > @@ -58,6 +58,7 @@
> > #define smu_feature_get_enabled_mask(smu, mask, num)
> smu_ppt_funcs(get_enabled_mask, 0, smu, mask, num)
> > #define smu_feature_is_enabled(smu, mask)
> smu_ppt_funcs(feature_is_enabled, 0, smu, mask)
> > #define smu_disable_all_features_with_exception(smu, mask)
> smu_ppt_funcs(disable_all_features_with_exception, 0, smu, mask)
> > +#define smu_clear_cached_dpm_feature_status_with_exception(smu,
> mask) smu_ppt_funcs(clear_cached_dpm_feature_status_with_exception,
> 0, smu, mask)
> > #define smu_is_dpm_running(smu)
> smu_ppt_funcs(is_dpm_running, 0 , smu)
> > #define smu_notify_display_change(smu)
> smu_ppt_funcs(notify_display_change, 0, smu)
> > #define smu_populate_umd_state_clk(smu)
> smu_ppt_funcs(populate_umd_state_clk, 0, smu)
> >
More information about the amd-gfx
mailing list