Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 7/22/2021 2:03 PM, Quan, Evan wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Thursday, July 22, 2021 4:10 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
>> properly for NV1x
>>
>>
>>
>> On 7/22/2021 8:50 AM, Evan Quan wrote:
>>> The customized OD settings can be divided into two parts: those
>>> committed ones and non-committed ones.
>>>     - For those changes which had been fed to SMU before S3/S4/Runpm
>>>       suspend kicked, they are committed changes. They should be properly
>>>       restored and fed to SMU on S3/S4/Runpm resume.
>>>     - For those non-committed changes, they are restored only without
>> feeding
>>>       to SMU.
>>>
>>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
>>> Signed-off-by: Evan Quan <evan.quan@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  8 +++
>>>    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     |  9 ++++
>>>    .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c   | 52
>> ++++++++++++++-----
>>>    .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c    | 12 +++++
>>>    4 files changed, 69 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> index 3e89852e4820..8ba53f16d2d9 100644
>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
>>>    	uint32_t power_limit;
>>>    	uint32_t fan_speed_percent;
>>>    	uint32_t flags;
>>> +	uint32_t committed_od;
>>>
>>>    	/* user clock state information */
>>>    	uint32_t clk_mask[SMU_CLK_COUNT];
>>> @@ -352,6 +353,7 @@ struct smu_table_context
>>>
>>>    	void				*overdrive_table;
>>>    	void                            *boot_overdrive_table;
>>> +	void				*committed_overdrive_table;
>>
>> May be rename it to user_overdrive_table - OD table with user settings?
> [Quan, Evan] Actually "overdrive_table " is  the user_overdrive_table. It contains all the modification including those not committed( the commit is performed by echo "c" > /sys/class/drm/card1/device/pp_od_clk_voltage).
> The new member added refers to those user customized but committed only settings. That's why it was named as " committed_overdrive_table". Any good suggestion for the naming?

As far as I understand, the problem is overdrive_table can have 
intemediary settings as the edit/commit is a two-step process. At any 
point we can have user_od_table keep the latest user mode settings. That 
is true when user restores to default settings also; that time we can 
just copy the boot settings back to user_od table and keep the flag as 
false indicating that there is no custom settings.

>>
>>>    	uint32_t			gpu_metrics_table_size;
>>>    	void				*gpu_metrics_table;
>>> @@ -623,6 +625,12 @@ struct pptable_funcs {
>>>    				 enum PP_OD_DPM_TABLE_COMMAND
>> type,
>>>    				 long *input, uint32_t size);
>>>
>>> +	/**
>>> +	 * @restore_committed_od_settings: Restore the customized and
>> committed
>>> +	 *                                 OD settings on S3/S4/Runpm resume.
>>> +	 */
>>> +	int (*restore_committed_od_settings)(struct smu_context *smu);
>>> +
>>>    	/**
>>>    	 * @get_clock_by_type_with_latency: Get the speed and latency of
>> a clock
>>>    	 *                                  domain.
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> index ebe672142808..5f7d98e99f76 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct
>> smu_context *smu)
>>>    		}
>>>    	}
>>>
>>> +	/* Restore customized and committed OD settings */
>>> +	if (smu->user_dpm_profile.committed_od) {
>>> +		if (smu->ppt_funcs->restore_committed_od_settings) {
>>> +			ret = smu->ppt_funcs-
>>> restore_committed_od_settings(smu);
>>> +			if (ret)
>>> +				dev_err(smu->adev->dev, "Failed to upload
>> customized OD settings\n");
>>> +		}
>>> +	}
>>> +
>>
>> Just thinking if there is a need to handle it ASIC specific. The flags and table
>> pointer are maintained in common structure. So can't we do the restore of
>> user OD settings directly in this common flow instead of having each ASIC to
>> implement the callback?
> [Quan, Evan] The OD settings restoring is ASIC specific as it performed on OD table and that(OD table) is ASIC specific.

I was thinking in terms of final logic that is being done. The below 
structures are available in common table context and we have a common 
logic to update the table. If there is no custom OD settings available, 
we could handle it with the flag.

+	struct smu_table_context *table_context = &smu->smu_table;
+	void *od_table = table_context->committed_overdrive_table;
+	int ret = 0;
+
+	ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
(void *)od_table, true);

>>
>>>    	/* Disable restore flag */
>>>    	smu->user_dpm_profile.flags &=
>> ~SMU_DPM_USER_PROFILE_RESTORE;
>>>    }
>>> 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 59ea59acfb00..4752299d7f91 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> @@ -2296,39 +2296,45 @@ static int
>> navi10_set_default_od_settings(struct smu_context *smu)
>>>    		(OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
>>>    	int ret = 0;
>>>
>>> -	ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
>> (void *)od_table, false);
>>> +	ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
>> (void
>>> +*)boot_od_table, false);
>>>    	if (ret) {
>>>    		dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
>>>    		return ret;
>>>    	}
>>>
>>> -	if (!od_table->GfxclkVolt1) {
>>> +	if (!boot_od_table->GfxclkVolt1) {
>>>    		ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
>>> -								&od_table-
>>> GfxclkVolt1,
>>> -								od_table-
>>> GfxclkFreq1);
>>> +
>> 	&boot_od_table->GfxclkVolt1,
>>> +
>> 	boot_od_table->GfxclkFreq1);
>>>    		if (ret)
>>>    			return ret;
>>>    	}
>>>
>>> -	if (!od_table->GfxclkVolt2) {
>>> +	if (!boot_od_table->GfxclkVolt2) {
>>>    		ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
>>> -								&od_table-
>>> GfxclkVolt2,
>>> -								od_table-
>>> GfxclkFreq2);
>>> +
>> 	&boot_od_table->GfxclkVolt2,
>>> +
>> 	boot_od_table->GfxclkFreq2);
>>>    		if (ret)
>>>    			return ret;
>>>    	}
>>>
>>> -	if (!od_table->GfxclkVolt3) {
>>> +	if (!boot_od_table->GfxclkVolt3) {
>>>    		ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
>>> -								&od_table-
>>> GfxclkVolt3,
>>> -								od_table-
>>> GfxclkFreq3);
>>> +
>> 	&boot_od_table->GfxclkVolt3,
>>> +
>> 	boot_od_table->GfxclkFreq3);
>>>    		if (ret)
>>>    			return ret;
>>>    	}
>>>
>>> -	memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
>>> +	navi10_dump_od_table(smu, boot_od_table);
>>>
>>> -	navi10_dump_od_table(smu, od_table);
>>> +	/*
>>> +	 * For S3/S4/Runpm, no need to install boot od table to
>> overdrive_table as
>>> +	 *   - either they already share the same OD settings on bootup
>>> +	 *   - or they have user customized OD settings
>>> +	 */
>>> +	if (!smu->adev->in_suspend)
>>> +		memcpy(od_table, boot_od_table,
>> sizeof(OverDriveTable_t));
>>>
>>>    	return 0;
>>>    }
>>> @@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct
>> smu_context *smu, enum PP_OD_DPM_TABL
>>>    			dev_err(smu->adev->dev, "Failed to import
>> overdrive table!\n");
>>>    			return ret;
>>>    		}
>>> +		if (memcmp(table_context->overdrive_table, table_context-
>>> boot_overdrive_table,
>>> +			sizeof(OverDriveTable_t))) {
>>
>> Shouldn't this be - compare against the edited settings and last committed
>> settings, overdrive_table vs committed_overdrive_table?
>>
>> Basically, user_od_table can be initialized with boot_od settings and the flag
>> gives the indication that user_od table is being used or not.
>> Before updating, check if the edited table (overdrive_table) and the current
>> user_od table are different, then commit the new table.
> [Quan, Evan] Yes, I also considered that. But that cannot handle the case below:
> 1 user made some customizations  -> 2 the customizations were committed -> 3 user restored to default(boot) OD settings(by "echo 'r'") and committed
> Although there were some changes between 2 and 3, there is actually no real customized changes compared to default(boot) OD settings.

On restore to default, just copy the boot_table settings to user_od and 
keep the flag as false. We are using user_od as the latest user 
preferred settings and overdrive_table is only used for intermediate 
updates.

The flag decides whether to restore or not, but at any point we can 
refer the user_od table to look upon the latest preferred user settings 
(default or custom).

Thanks,
Lijo

>>
>>> +			smu->user_dpm_profile.committed_od = true;
>>> +			memcpy(table_context-
>>> committed_overdrive_table, table_context->overdrive_table,
>>> +					sizeof(OverDriveTable_t));
>>> +		} else {
>>> +			smu->user_dpm_profile.committed_od = false;
>>> +		}
>>>    		break;
>>>    	case PP_OD_EDIT_VDDC_CURVE:
>>>    		if (!navi10_od_feature_is_supported(od_settings,
>>> SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@ static int
>> navi10_od_edit_dpm_table(struct smu_context *smu, enum
>> PP_OD_DPM_TABL
>>>    	return ret;
>>>    }
>>>
>>> +static int navi10_restore_committed_od_settings(struct smu_context
>>> +*smu) {
>>> +	struct smu_table_context *table_context = &smu->smu_table;
>>> +	void *od_table = table_context->committed_overdrive_table;
>>> +	int ret = 0;
>>> +
>>> +	ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
>> (void *)od_table, true);
>>> +	if (ret)
>>> +		dev_err(smu->adev->dev, "Failed to import overdrive
>> table!\n");
>>> +
>>> +	return ret;
>>> +}
>>
>> As mentioned before, not sure if this is needed as callback. Even if, this can
>> be something common for smuv11, there is nothing ASIC specific in this logic,
>> right?
> [Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11 common API.
> 
> BR
> Evan
>>
>> Thanks,
>> Lijo
>>
>>>    static int navi10_run_btc(struct smu_context *smu)
>>>    {
>>>    	int ret = 0;
>>> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
>> navi10_ppt_funcs = {
>>>    	.set_soft_freq_limited_range =
>> smu_v11_0_set_soft_freq_limited_range,
>>>    	.set_default_od_settings = navi10_set_default_od_settings,
>>>    	.od_edit_dpm_table = navi10_od_edit_dpm_table,
>>> +	.restore_committed_od_settings =
>>> +navi10_restore_committed_od_settings,
>>>    	.run_btc = navi10_run_btc,
>>>    	.set_power_source = smu_v11_0_set_power_source,
>>>    	.get_pp_feature_mask = smu_cmn_get_pp_feature_mask, diff --git
>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> index 0a5d46ac9ccd..28fc3f17c1b1 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
>> smu_context *smu)
>>>    			ret = -ENOMEM;
>>>    			goto err3_out;
>>>    		}
>>> +
>>> +		smu_table->committed_overdrive_table =
>>> +			kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
>> GFP_KERNEL);
>>> +		if (!smu_table->committed_overdrive_table) {
>>> +			ret = -ENOMEM;
>>> +			goto err4_out;
>>> +		}
>>> +
>>>    	}
>>>
>>>    	return 0;
>>>
>>> +err4_out:
>>> +	kfree(smu_table->boot_overdrive_table);
>>>    err3_out:
>>>    	kfree(smu_table->overdrive_table);
>>>    err2_out:
>>> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
>> smu_context *smu)
>>>    	struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
>>>
>>>    	kfree(smu_table->gpu_metrics_table);
>>> +	kfree(smu_table->committed_overdrive_table);
>>>    	kfree(smu_table->boot_overdrive_table);
>>>    	kfree(smu_table->overdrive_table);
>>>    	kfree(smu_table->max_sustainable_clocks);
>>>    	kfree(smu_table->driver_pptable);
>>>    	kfree(smu_table->clocks_table);
>>>    	smu_table->gpu_metrics_table = NULL;
>>> +	smu_table->committed_overdrive_table = NULL;
>>>    	smu_table->boot_overdrive_table = NULL;
>>>    	smu_table->overdrive_table = NULL;
>>>    	smu_table->max_sustainable_clocks = NULL;
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx