[Public]
Hi Lijo,
Per original design, all non ASIC specific code goes to amdgpu_smu.c. To me, it sounds you want to separate some of them(from amdgpu_smu.c) into a new file. I'm OK with that.
However as I said, to do that, we may need to make the bitmasks stored in feature->enabled non asic bundled/specific. I need more time to consider and will put that on my TODO list.
For this specific patch, I will update it with code fit into smu_cmn_disable_all_features_with_exception(). Are you OK with that?
BR
Evan
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Monday, June 7, 2021 4:05 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: RE: [PATCH V2 4/5] drm/amd/pm: clear the cached dpm feature
> status
>
> [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@xxxxxxx>
> Sent: Monday, June 7, 2021 1:04 PM
> To: Lazar, Lijo <Lijo.Lazar@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> 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@xxxxxxx>
> > Sent: Friday, June 4, 2021 7:56 PM
> > To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> > 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@xxxxxxx>
> > > ---
> > > 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)
> > >
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
