Hi Hans, On 9/26/2023 4:05 PM, Hans de Goede wrote: > Hi, > > On 9/22/23 19:50, Shyam Sundar S K wrote: >> For the Smart PC Solution to fully work, it has to enact to the actions >> coming from TA. Add the initial code path for set interface to AMDGPU. >> >> Co-developed-by: Mario Limonciello <mario.limonciello@xxxxxxx> >> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +++++++++++++++++++++ >> drivers/platform/x86/amd/pmf/pmf.h | 2 ++ >> drivers/platform/x86/amd/pmf/tee-if.c | 19 +++++++++++++++++-- >> include/linux/amd-pmf-io.h | 1 + >> 4 files changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >> index 232d11833ddc..5c567bff0548 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf) >> return 0; >> } >> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data); >> + >> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf) >> +{ >> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev); >> + struct amdgpu_device *adev = drm_to_adev(drm_dev); >> + struct backlight_device *bd; >> + >> + if (!(adev->flags & AMD_IS_APU)) { >> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >> + return -ENODEV; >> + } >> + >> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >> + if (!bd) >> + return -ENODEV; > > This assumes that the backlight is always controller by the amdgpu's > native backlight driver, but it might e.g. also be handled by > eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU + > nvidia dgpu). PMF is meant for AMD APUs(atleast for now) and the _HID will only be made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW should be safe, right? > > For now what should be done here is to call acpi_video_get_backlight_type() > and then translate the return value from this into a backlight-type: > > acpi_backlight_video -> BACKLIGHT_FIRMWARE > acpi_backlight_vendor, -> BACKLIGHT_PLATFORM > acpi_backlight_native, -> BACKLIGHT_RAW > acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE > acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM > I can add this change in the v2, do you insist on this? Thanks, Shyam > Also I'm worried about probe order here, this code currently assumes > that the GPU or other backlight driver has loaded before this runs, > which is not necessarily the case. > > I think that if the backlight_device_get_by_type() fails this > should be retried say every 10 seconds from some delayed workqueue > for at least a couple of minutes after boot. > > Regards, > > Hans > > > > >> + >> + backlight_device_set_brightness(bd, pmf->brightness); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data); >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h >> index 9032df4ba48a..ce89cc0daa5a 100644 >> --- a/drivers/platform/x86/amd/pmf/pmf.h >> +++ b/drivers/platform/x86/amd/pmf/pmf.h >> @@ -73,6 +73,7 @@ >> #define PMF_POLICY_STT_SKINTEMP_APU 7 >> #define PMF_POLICY_STT_SKINTEMP_HS2 8 >> #define PMF_POLICY_SYSTEM_STATE 9 >> +#define PMF_POLICY_DISPLAY_BRIGHTNESS 12 >> #define PMF_POLICY_P3T 38 >> >> /* TA macros */ >> @@ -480,6 +481,7 @@ enum ta_pmf_error_type { >> }; >> >> struct pmf_action_table { >> + unsigned long display_brightness; >> enum system_state system_state; >> unsigned long spl; /* in mW */ >> unsigned long sppt; /* in mW */ >> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c >> index 1608996654e8..eefffff83a4c 100644 >> --- a/drivers/platform/x86/amd/pmf/tee-if.c >> +++ b/drivers/platform/x86/amd/pmf/tee-if.c >> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event) >> return 0; >> } >> >> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out) >> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out) >> { >> u32 val, event = 0; >> - int idx; >> + int idx, ret; >> >> for (idx = 0; idx < out->actions_count; idx++) { >> val = out->actions_list[idx].value; >> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_ >> dev->prev_data->system_state = 0; >> } >> break; >> + >> + case PMF_POLICY_DISPLAY_BRIGHTNESS: >> + ret = amd_pmf_get_gfx_data(&dev->gfx_data); >> + if (ret) >> + return ret; >> + >> + dev->prev_data->display_brightness = dev->gfx_data.brightness; >> + if (dev->prev_data->display_brightness != val) { >> + dev->gfx_data.brightness = val; >> + amd_pmf_set_gfx_data(&dev->gfx_data); >> + dev_dbg(dev->dev, "update DISPLAY_BRIGHTNESS : %d\n", val); >> + } >> + break; >> } >> } >> + >> + return 0; >> } >> >> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev) >> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h >> index a2d4af231362..ecae387ddaa6 100644 >> --- a/include/linux/amd-pmf-io.h >> +++ b/include/linux/amd-pmf-io.h >> @@ -25,4 +25,5 @@ struct amd_gpu_pmf_data { >> }; >> >> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf); >> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf); >> #endif >