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

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

[Public]

Hi Evan,

In that case, using "od_edit" table for the intermediate table

During commit command, what if it's done like
	1) Copy and update table if od_edit table is different than user_od table
	2) Set the flag to false if od_edit table and boot_od table are different

Then current od settings may always be picked from user_od table and the flag may be used to distinguish if it's boot od settings or custom settings (for restore or other purposes).

Thanks,
Lijo

-----Original Message-----
From: Quan, Evan <Evan.Quan@amd.com> 
Sent: Friday, July 23, 2021 12:51 PM
To: Lazar, Lijo <Lijo.Lazar@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

[AMD Official Use Only]

Hi Lijo,

Sorry, I doubled checked. The implementation of Navi1x is right. 
The original design for "restores to default settings" is a two-step process.
That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish whether it's an usual customized od setting update or just restoring to defaults.
And my current implementation for that is as below. Any comments?

+               if (memcmp(table_context->overdrive_table, table_context->boot_overdrive_table,
+                       sizeof(OverDriveTable_t))) {
+                       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;
+               }

BR
Evan
> -----Original Message-----
> From: Quan, Evan
> Sent: Friday, July 23, 2021 2:48 PM
> To: Lazar, Lijo <Lijo.Lazar@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
> 
> [AMD Official Use Only]
> 
> 
> 
> > -----Original Message-----
> > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > Sent: Thursday, July 22, 2021 5:03 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 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.
> [Quan, Evan] For now, on Navi1x the "restores to default settings" is 
> also a two-step process. That will make "copy the boot settings back 
> to user_od table and keep the flag as false" tricky. However, it seems 
> that does not fit original design. As per original design, the "restores to default settings"
> should be a one-step process. Let me correct them with new patch 
> series and proceed further discussion then.
> 
> BR
> Evan
> >
> > >>
> > >>>    	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