On Tue, Sep 19, 2023 at 11:00 PM Ma, Jun <majun@xxxxxxx> wrote:
>
> Hi Alex,
>
> On 9/18/2023 10:05 PM, Alex Deucher wrote:
> > On Mon, Sep 11, 2023 at 2:00 AM Ma Jun <Jun.Ma2@xxxxxxx> wrote:
> >>
> >> Add reset option for fan_curve.
> >> User can use command "echo r > fan_cure" to reset the fan_curve
> >> to boot value
> >>
> >> Signed-off-by: Ma Jun <Jun.Ma2@xxxxxxx>
> >> ---
> >> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 8 ++++
> >> .../drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c | 43 +++++++++++++++++--
> >> 2 files changed, 47 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >> index d05d9cd61331..2acac21387bc 100644
> >> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >> @@ -3437,6 +3437,11 @@ static int parse_input_od_command_lines(const char *buf,
> >> case 'c':
> >> *type = PP_OD_COMMIT_DPM_TABLE;
> >> return 0;
> >> + case 'r':
> >> + params[parameter_size] = *type;
> >> + *num_of_params = 1;
> >> + *type = PP_OD_RESTORE_DEFAULT_TABLE;
> >> + return 0;
> >> default:
> >> break;
> >> }
> >> @@ -3531,6 +3536,9 @@ amdgpu_distribute_custom_od_settings(struct amdgpu_device *adev,
> >> * When you have finished the editing, write "c" (commit) to the file to commit
> >> * your changes.
> >> *
> >> + * If you want to reset to the default value, write "r" (reset) to the file to
> >> + * reset them
> >> + *
> >> * There are two fan control modes supported: auto and manual. With auto mode,
> >> * PMFW handles the fan speed control(how fan speed reacts to ASIC temperature).
> >> * While with manual mode, users can set their own fan curve line as what
> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
> >> index a719bae54e2c..644773c4bccb 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
> >> @@ -1484,6 +1484,36 @@ static int smu_v13_0_0_print_clk_levels(struct smu_context *smu,
> >> return size;
> >> }
> >>
> >> +
> >> +static int smu_v13_0_0_od_restore_table_single(struct smu_context *smu, long input)
> >> +{
> >> + struct smu_table_context *table_context = &smu->smu_table;
> >> + OverDriveTableExternal_t *boot_overdrive_table =
> >> + (OverDriveTableExternal_t *)table_context->boot_overdrive_table;
> >> + OverDriveTableExternal_t *od_table =
> >> + (OverDriveTableExternal_t *)table_context->overdrive_table;
> >> + struct amdgpu_device *adev = smu->adev;
> >> + int i;
> >> +
> >> + switch (input) {
> >> + case PP_OD_EDIT_FAN_CURVE:
> >> + for (i = 0; i < NUM_OD_FAN_MAX_POINTS; i++) {
> >> + od_table->OverDriveTable.FanLinearTempPoints[i] =
> >> + boot_overdrive_table->OverDriveTable.FanLinearTempPoints[i];
> >> + od_table->OverDriveTable.FanLinearPwmPoints[i] =
> >> + boot_overdrive_table->OverDriveTable.FanLinearPwmPoints[i];
> >> + }
> >> + od_table->OverDriveTable.FanMode = FAN_MODE_AUTO;
> >> + od_table->OverDriveTable.FeatureCtrlMask |= BIT(PP_OD_FEATURE_FAN_CURVE_BIT);
> >> + break;
> >> + default:
> >> + dev_info(adev->dev, "Invalid table index: %ld\n", input);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int smu_v13_0_0_od_edit_dpm_table(struct smu_context *smu,
> >> enum PP_OD_DPM_TABLE_COMMAND type,
> >> long input[],
> >> @@ -1770,13 +1800,18 @@ static int smu_v13_0_0_od_edit_dpm_table(struct smu_context *smu,
> >> break;
> >>
> >> case PP_OD_RESTORE_DEFAULT_TABLE:
> >> - feature_ctrlmask = od_table->OverDriveTable.FeatureCtrlMask;
> >> - memcpy(od_table,
> >> + if (size == 1) {
> >> + ret = smu_v13_0_0_od_restore_table_single(smu, input[0]);
> >> + if (ret)
> >> + return ret;
> >> + } else {
> >> + feature_ctrlmask = od_table->OverDriveTable.FeatureCtrlMask;
> >> + memcpy(od_table,
> >> table_context->boot_overdrive_table,
> >> sizeof(OverDriveTableExternal_t));
> >> - od_table->OverDriveTable.FeatureCtrlMask = feature_ctrlmask;
> >> + od_table->OverDriveTable.FeatureCtrlMask = feature_ctrlmask;
> >> + }
> >> fallthrough;
> >
> > I'm not sure the fallthrough here is correct. I think setting the
> > reset should just reset the values, but you'd still need to commit to
> > actually set them to the firmware. Double check that the behavior is
> > consistent with older chips.
> >
>
> We also use the "fallthrough" in sienna_cichlid_od_edit_dpm_table() and
> smu_v13_0_7_od_edit_dpm_table().
>
> But other chips used "break" here for reset. I think user maybe need
> to use command " echo r > xx" and "echo c > xx" to reset.
>
> So, It seems that it makes more sense to use "fallthrough" here.
>
I guess if that's what we do for seinna_cichlid, it's fine. Too bad
we lost consistency here.
Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
> Regards,
> Ma Jun
>
> > Alex
> >
> >
> >> -
> >> case PP_OD_COMMIT_DPM_TABLE:
> >> /*
> >> * The member below instructs PMFW the settings focused in
> >> --
> >> 2.34.1
> >>
