RE: [PATCH V2 4/5] drm/amd/pm: clear the cached dpm feature status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[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



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux