[PATCH V4 05/17] drm/amd/pm: do not expose those APIs used internally only in si_dpm.c

Quan, Evan Evan.Quan at amd.com
Fri Dec 10 07:07:06 UTC 2021


[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Thursday, December 9, 2021 8:09 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; Feng, Kenneth <Kenneth.Feng at amd.com>
> Subject: Re: [PATCH V4 05/17] drm/amd/pm: do not expose those APIs used
> internally only in si_dpm.c
> 
> 
> 
> On 12/3/2021 8:35 AM, Evan Quan wrote:
> > Move them to si_dpm.c instead.
> >
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > Change-Id: I288205cfd7c6ba09cfb22626ff70360d61ff0c67
> > --
> > v1->v2:
> >    - rename the API with "si_" prefix(Alex)
> > v2->v3:
> >    - rename other data structures used only in si_dpm.c(Lijo)
> > ---
> >   drivers/gpu/drm/amd/pm/amdgpu_dpm.c       |  25 -----
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h   |  25 -----
> >   drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 106
> +++++++++++++++-------
> >   drivers/gpu/drm/amd/pm/powerplay/si_dpm.h |  15 ++-
> >   4 files changed, 83 insertions(+), 88 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > index 72a8cb70d36b..b31858ad9b83 100644
> > --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > @@ -894,31 +894,6 @@ void amdgpu_add_thermal_controller(struct
> amdgpu_device *adev)
> >   	}
> >   }
> >
> > -enum amdgpu_pcie_gen amdgpu_get_pcie_gen_support(struct
> amdgpu_device *adev,
> > -						 u32 sys_mask,
> > -						 enum amdgpu_pcie_gen
> asic_gen,
> > -						 enum amdgpu_pcie_gen
> default_gen)
> > -{
> > -	switch (asic_gen) {
> > -	case AMDGPU_PCIE_GEN1:
> > -		return AMDGPU_PCIE_GEN1;
> > -	case AMDGPU_PCIE_GEN2:
> > -		return AMDGPU_PCIE_GEN2;
> > -	case AMDGPU_PCIE_GEN3:
> > -		return AMDGPU_PCIE_GEN3;
> > -	default:
> > -		if ((sys_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN3)
> &&
> > -		    (default_gen == AMDGPU_PCIE_GEN3))
> > -			return AMDGPU_PCIE_GEN3;
> > -		else if ((sys_mask &
> CAIL_PCIE_LINK_SPEED_SUPPORT_GEN2) &&
> > -			 (default_gen == AMDGPU_PCIE_GEN2))
> > -			return AMDGPU_PCIE_GEN2;
> > -		else
> > -			return AMDGPU_PCIE_GEN1;
> > -	}
> > -	return AMDGPU_PCIE_GEN1;
> > -}
> > -
> >   struct amd_vce_state*
> >   amdgpu_get_vce_clock_state(void *handle, u32 idx)
> >   {
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > index 6681b878e75f..f43b96dfe9d8 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > @@ -45,19 +45,6 @@ enum amdgpu_int_thermal_type {
> >   	THERMAL_TYPE_KV,
> >   };
> >
> > -enum amdgpu_dpm_auto_throttle_src {
> > -	AMDGPU_DPM_AUTO_THROTTLE_SRC_THERMAL,
> > -	AMDGPU_DPM_AUTO_THROTTLE_SRC_EXTERNAL
> > -};
> > -
> > -enum amdgpu_dpm_event_src {
> > -	AMDGPU_DPM_EVENT_SRC_ANALOG = 0,
> > -	AMDGPU_DPM_EVENT_SRC_EXTERNAL = 1,
> > -	AMDGPU_DPM_EVENT_SRC_DIGITAL = 2,
> > -	AMDGPU_DPM_EVENT_SRC_ANALOG_OR_EXTERNAL = 3,
> > -	AMDGPU_DPM_EVENT_SRC_DIGIAL_OR_EXTERNAL = 4
> > -};
> > -
> >   struct amdgpu_ps {
> >   	u32 caps; /* vbios flags */
> >   	u32 class; /* vbios flags */
> > @@ -252,13 +239,6 @@ struct amdgpu_dpm_fan {
> >   	bool ucode_fan_control;
> >   };
> >
> > -enum amdgpu_pcie_gen {
> > -	AMDGPU_PCIE_GEN1 = 0,
> > -	AMDGPU_PCIE_GEN2 = 1,
> > -	AMDGPU_PCIE_GEN3 = 2,
> > -	AMDGPU_PCIE_GEN_INVALID = 0xffff
> > -};
> > -
> >   #define amdgpu_dpm_reset_power_profile_state(adev, request) \
> >   		((adev)->powerplay.pp_funcs-
> >reset_power_profile_state(\
> >   			(adev)->powerplay.pp_handle, request)) @@ -
> 403,11 +383,6 @@ void
> > amdgpu_free_extended_power_table(struct amdgpu_device *adev);
> >
> >   void amdgpu_add_thermal_controller(struct amdgpu_device *adev);
> >
> > -enum amdgpu_pcie_gen amdgpu_get_pcie_gen_support(struct
> amdgpu_device *adev,
> > -						 u32 sys_mask,
> > -						 enum amdgpu_pcie_gen
> asic_gen,
> > -						 enum amdgpu_pcie_gen
> default_gen);
> > -
> >   struct amd_vce_state*
> >   amdgpu_get_vce_clock_state(void *handle, u32 idx);
> >
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > index 81f82aa05ec2..ab0fa6c79255 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > @@ -96,6 +96,19 @@ union pplib_clock_info {
> >   	struct _ATOM_PPLIB_SI_CLOCK_INFO si;
> >   };
> >
> > +enum si_dpm_auto_throttle_src {
> > +	DPM_AUTO_THROTTLE_SRC_THERMAL,
> > +	DPM_AUTO_THROTTLE_SRC_EXTERNAL
> > +};
> > +
> 
> Since the final usage is something like (1 <<
> DPM_AUTO_THROTTLE_SRC_EXTERNAL), it's better to associate the SI
> context also along with that - SI_DPM_AUTO_THROTTLE_SRC_EXTERNAL - to
> denote that these are SI specific values.
[Quan, Evan] Sure, I can do that.

BR
Evan
> 
> Thanks,
> Lijo
> 
> > +enum si_dpm_event_src {
> > +	DPM_EVENT_SRC_ANALOG = 0,
> > +	DPM_EVENT_SRC_EXTERNAL = 1,
> > +	DPM_EVENT_SRC_DIGITAL = 2,
> > +	DPM_EVENT_SRC_ANALOG_OR_EXTERNAL = 3,
> > +	DPM_EVENT_SRC_DIGIAL_OR_EXTERNAL = 4 };
> > +
> >   static const u32 r600_utc[R600_PM_NUMBER_OF_TC] =
> >   {
> >   	R600_UTC_DFLT_00,
> > @@ -3718,25 +3731,25 @@ static void si_set_dpm_event_sources(struct
> amdgpu_device *adev, u32 sources)
> >   {
> >   	struct rv7xx_power_info *pi = rv770_get_pi(adev);
> >   	bool want_thermal_protection;
> > -	enum amdgpu_dpm_event_src dpm_event_src;
> > +	enum si_dpm_event_src dpm_event_src;
> >
> >   	switch (sources) {
> >   	case 0:
> >   	default:
> >   		want_thermal_protection = false;
> >   		break;
> > -	case (1 << AMDGPU_DPM_AUTO_THROTTLE_SRC_THERMAL):
> > +	case (1 << DPM_AUTO_THROTTLE_SRC_THERMAL):
> >   		want_thermal_protection = true;
> > -		dpm_event_src = AMDGPU_DPM_EVENT_SRC_DIGITAL;
> > +		dpm_event_src = DPM_EVENT_SRC_DIGITAL;
> >   		break;
> > -	case (1 << AMDGPU_DPM_AUTO_THROTTLE_SRC_EXTERNAL):
> > +	case (1 << DPM_AUTO_THROTTLE_SRC_EXTERNAL):
> >   		want_thermal_protection = true;
> > -		dpm_event_src = AMDGPU_DPM_EVENT_SRC_EXTERNAL;
> > +		dpm_event_src = DPM_EVENT_SRC_EXTERNAL;
> >   		break;
> > -	case ((1 << AMDGPU_DPM_AUTO_THROTTLE_SRC_EXTERNAL) |
> > -	      (1 << AMDGPU_DPM_AUTO_THROTTLE_SRC_THERMAL)):
> > +	case ((1 << DPM_AUTO_THROTTLE_SRC_EXTERNAL) |
> > +	      (1 << DPM_AUTO_THROTTLE_SRC_THERMAL)):
> >   		want_thermal_protection = true;
> > -		dpm_event_src =
> AMDGPU_DPM_EVENT_SRC_DIGIAL_OR_EXTERNAL;
> > +		dpm_event_src = DPM_EVENT_SRC_DIGIAL_OR_EXTERNAL;
> >   		break;
> >   	}
> >
> > @@ -3750,7 +3763,7 @@ static void si_set_dpm_event_sources(struct
> amdgpu_device *adev, u32 sources)
> >   }
> >
> >   static void si_enable_auto_throttle_source(struct amdgpu_device *adev,
> > -					   enum
> amdgpu_dpm_auto_throttle_src source,
> > +					   enum si_dpm_auto_throttle_src
> source,
> >   					   bool enable)
> >   {
> >   	struct rv7xx_power_info *pi = rv770_get_pi(adev); @@ -4927,6
> > +4940,31 @@ static int si_populate_smc_initial_state(struct
> amdgpu_device *adev,
> >   	return 0;
> >   }
> >
> > +static enum si_pcie_gen si_gen_pcie_gen_support(struct amdgpu_device
> *adev,
> > +						u32 sys_mask,
> > +						enum si_pcie_gen asic_gen,
> > +						enum si_pcie_gen
> default_gen)
> > +{
> > +	switch (asic_gen) {
> > +	case PCIE_GEN1:
> > +		return PCIE_GEN1;
> > +	case PCIE_GEN2:
> > +		return PCIE_GEN2;
> > +	case PCIE_GEN3:
> > +		return PCIE_GEN3;
> > +	default:
> > +		if ((sys_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN3)
> &&
> > +		    (default_gen == PCIE_GEN3))
> > +			return PCIE_GEN3;
> > +		else if ((sys_mask &
> CAIL_PCIE_LINK_SPEED_SUPPORT_GEN2) &&
> > +			 (default_gen == PCIE_GEN2))
> > +			return PCIE_GEN2;
> > +		else
> > +			return PCIE_GEN1;
> > +	}
> > +	return PCIE_GEN1;
> > +}
> > +
> >   static int si_populate_smc_acpi_state(struct amdgpu_device *adev,
> >   				      SISLANDS_SMC_STATETABLE *table)
> >   {
> > @@ -4989,10 +5027,10 @@ static int si_populate_smc_acpi_state(struct
> amdgpu_device *adev,
> >   							      &table-
> >ACPIState.level.std_vddc);
> >   		}
> >   		table->ACPIState.level.gen2PCIE =
> > -			(u8)amdgpu_get_pcie_gen_support(adev,
> > -							si_pi->sys_pcie_mask,
> > -							si_pi->boot_pcie_gen,
> > -
> 	AMDGPU_PCIE_GEN1);
> > +			(u8)si_gen_pcie_gen_support(adev,
> > +						    si_pi->sys_pcie_mask,
> > +						    si_pi->boot_pcie_gen,
> > +						    PCIE_GEN1);
> >
> >   		if (si_pi->vddc_phase_shed_control)
> >   			si_populate_phase_shedding_value(adev,
> > @@ -5430,7 +5468,7 @@ static int si_convert_power_level_to_smc(struct
> amdgpu_device *adev,
> >   	bool gmc_pg = false;
> >
> >   	if (eg_pi->pcie_performance_request &&
> > -	    (si_pi->force_pcie_gen != AMDGPU_PCIE_GEN_INVALID))
> > +	    (si_pi->force_pcie_gen != PCIE_GEN_INVALID))
> >   		level->gen2PCIE = (u8)si_pi->force_pcie_gen;
> >   	else
> >   		level->gen2PCIE = (u8)pl->pcie_gen; @@ -6147,8 +6185,8
> @@ static
> > void si_enable_voltage_control(struct amdgpu_device *adev, bool enable)
> >   		WREG32_P(GENERAL_PWRMGT, 0, ~VOLT_PWRMGT_EN);
> >   }
> >
> > -static enum amdgpu_pcie_gen si_get_maximum_link_speed(struct
> amdgpu_device *adev,
> > -						      struct amdgpu_ps
> *amdgpu_state)
> > +static enum si_pcie_gen si_get_maximum_link_speed(struct
> amdgpu_device *adev,
> > +						  struct amdgpu_ps
> *amdgpu_state)
> >   {
> >   	struct si_ps *state = si_get_ps(amdgpu_state);
> >   	int i;
> > @@ -6177,27 +6215,27 @@ static void
> si_request_link_speed_change_before_state_change(struct amdgpu_devic
> >   							     struct amdgpu_ps
> *amdgpu_current_state)
> >   {
> >   	struct si_power_info *si_pi = si_get_pi(adev);
> > -	enum amdgpu_pcie_gen target_link_speed =
> si_get_maximum_link_speed(adev, amdgpu_new_state);
> > -	enum amdgpu_pcie_gen current_link_speed;
> > +	enum si_pcie_gen target_link_speed =
> si_get_maximum_link_speed(adev, amdgpu_new_state);
> > +	enum si_pcie_gen current_link_speed;
> >
> > -	if (si_pi->force_pcie_gen == AMDGPU_PCIE_GEN_INVALID)
> > +	if (si_pi->force_pcie_gen == PCIE_GEN_INVALID)
> >   		current_link_speed = si_get_maximum_link_speed(adev,
> amdgpu_current_state);
> >   	else
> >   		current_link_speed = si_pi->force_pcie_gen;
> >
> > -	si_pi->force_pcie_gen = AMDGPU_PCIE_GEN_INVALID;
> > +	si_pi->force_pcie_gen = PCIE_GEN_INVALID;
> >   	si_pi->pspp_notify_required = false;
> >   	if (target_link_speed > current_link_speed) {
> >   		switch (target_link_speed) {
> >   #if defined(CONFIG_ACPI)
> > -		case AMDGPU_PCIE_GEN3:
> > +		case PCIE_GEN3:
> >   			if (amdgpu_acpi_pcie_performance_request(adev,
> PCIE_PERF_REQ_PECI_GEN3, false) == 0)
> >   				break;
> > -			si_pi->force_pcie_gen = AMDGPU_PCIE_GEN2;
> > -			if (current_link_speed == AMDGPU_PCIE_GEN2)
> > +			si_pi->force_pcie_gen = PCIE_GEN2;
> > +			if (current_link_speed == PCIE_GEN2)
> >   				break;
> >   			fallthrough;
> > -		case AMDGPU_PCIE_GEN2:
> > +		case PCIE_GEN2:
> >   			if (amdgpu_acpi_pcie_performance_request(adev,
> PCIE_PERF_REQ_PECI_GEN2, false) == 0)
> >   				break;
> >   			fallthrough;
> > @@ -6217,13 +6255,13 @@ static void
> si_notify_link_speed_change_after_state_change(struct amdgpu_device
> >   							   struct amdgpu_ps
> *amdgpu_current_state)
> >   {
> >   	struct si_power_info *si_pi = si_get_pi(adev);
> > -	enum amdgpu_pcie_gen target_link_speed =
> si_get_maximum_link_speed(adev, amdgpu_new_state);
> > +	enum si_pcie_gen target_link_speed =
> si_get_maximum_link_speed(adev,
> > +amdgpu_new_state);
> >   	u8 request;
> >
> >   	if (si_pi->pspp_notify_required) {
> > -		if (target_link_speed == AMDGPU_PCIE_GEN3)
> > +		if (target_link_speed == PCIE_GEN3)
> >   			request = PCIE_PERF_REQ_PECI_GEN3;
> > -		else if (target_link_speed == AMDGPU_PCIE_GEN2)
> > +		else if (target_link_speed == PCIE_GEN2)
> >   			request = PCIE_PERF_REQ_PECI_GEN2;
> >   		else
> >   			request = PCIE_PERF_REQ_PECI_GEN1; @@ -6864,7
> +6902,7 @@ static
> > int si_dpm_enable(struct amdgpu_device *adev)
> >   	si_enable_sclk_control(adev, true);
> >   	si_start_dpm(adev);
> >
> > -	si_enable_auto_throttle_source(adev,
> AMDGPU_DPM_AUTO_THROTTLE_SRC_THERMAL, true);
> > +	si_enable_auto_throttle_source(adev,
> DPM_AUTO_THROTTLE_SRC_THERMAL,
> > +true);
> >   	si_thermal_start_thermal_controller(adev);
> >
> >   	ni_update_current_ps(adev, boot_ps); @@ -6904,7 +6942,7 @@
> static
> > void si_dpm_disable(struct amdgpu_device *adev)
> >   	si_enable_power_containment(adev, boot_ps, false);
> >   	si_enable_smc_cac(adev, boot_ps, false);
> >   	si_enable_spread_spectrum(adev, false);
> > -	si_enable_auto_throttle_source(adev,
> AMDGPU_DPM_AUTO_THROTTLE_SRC_THERMAL, false);
> > +	si_enable_auto_throttle_source(adev,
> DPM_AUTO_THROTTLE_SRC_THERMAL,
> > +false);
> >   	si_stop_dpm(adev);
> >   	si_reset_to_default(adev);
> >   	si_dpm_stop_smc(adev);
> > @@ -7148,10 +7186,10 @@ static void si_parse_pplib_clock_info(struct
> amdgpu_device *adev,
> >   	pl->vddc = le16_to_cpu(clock_info->si.usVDDC);
> >   	pl->vddci = le16_to_cpu(clock_info->si.usVDDCI);
> >   	pl->flags = le32_to_cpu(clock_info->si.ulFlags);
> > -	pl->pcie_gen = amdgpu_get_pcie_gen_support(adev,
> > -						   si_pi->sys_pcie_mask,
> > -						   si_pi->boot_pcie_gen,
> > -						   clock_info->si.ucPCIEGen);
> > +	pl->pcie_gen = si_gen_pcie_gen_support(adev,
> > +					       si_pi->sys_pcie_mask,
> > +					       si_pi->boot_pcie_gen,
> > +					       clock_info->si.ucPCIEGen);
> >
> >   	/* patch up vddc if necessary */
> >   	ret = si_get_leakage_voltage_from_leakage_index(adev, pl->vddc,
> @@
> > -7318,7 +7356,7 @@ static int si_dpm_init(struct amdgpu_device *adev)
> >
> >   	si_pi->sys_pcie_mask =
> >   		adev->pm.pcie_gen_mask &
> CAIL_PCIE_LINK_SPEED_SUPPORT_MASK;
> > -	si_pi->force_pcie_gen = AMDGPU_PCIE_GEN_INVALID;
> > +	si_pi->force_pcie_gen = PCIE_GEN_INVALID;
> >   	si_pi->boot_pcie_gen = si_get_current_pcie_speed(adev);
> >
> >   	si_set_max_cu_value(adev);
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.h
> > b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.h
> > index bc0be6818e21..77614ff10df6 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.h
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.h
> > @@ -595,13 +595,20 @@ struct rv7xx_power_info {
> >   	RV770_SMC_STATETABLE smc_statetable;
> >   };
> >
> > +enum si_pcie_gen {
> > +	PCIE_GEN1 = 0,
> > +	PCIE_GEN2 = 1,
> > +	PCIE_GEN3 = 2,
> > +	PCIE_GEN_INVALID = 0xffff
> > +};
> > +
> >   struct rv7xx_pl {
> >   	u32 sclk;
> >   	u32 mclk;
> >   	u16 vddc;
> >   	u16 vddci; /* eg+ only */
> >   	u32 flags;
> > -	enum amdgpu_pcie_gen pcie_gen; /* si+ only */
> > +	enum si_pcie_gen pcie_gen; /* si+ only */
> >   };
> >
> >   struct rv7xx_ps {
> > @@ -967,9 +974,9 @@ struct si_power_info {
> >   	struct si_ulv_param ulv;
> >   	u32 max_cu;
> >   	/* pcie gen */
> > -	enum amdgpu_pcie_gen force_pcie_gen;
> > -	enum amdgpu_pcie_gen boot_pcie_gen;
> > -	enum amdgpu_pcie_gen acpi_pcie_gen;
> > +	enum si_pcie_gen force_pcie_gen;
> > +	enum si_pcie_gen boot_pcie_gen;
> > +	enum si_pcie_gen acpi_pcie_gen;
> >   	u32 sys_pcie_mask;
> >   	/* flags */
> >   	bool enable_dte;
> >


More information about the amd-gfx mailing list