| Message ID | 20230922175056.244940-14-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce PMF Smart PC Solution Builder Feature | expand |
[Public] > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of > Shyam Sundar S K > Sent: Friday, September 22, 2023 1:51 PM > To: hdegoede@redhat.com; markgross@kernel.org; Natikar, Basavaraj > <Basavaraj.Natikar@amd.com>; jikos@kernel.org; > benjamin.tissoires@redhat.com; Deucher, Alexander > <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; > airlied@gmail.com; daniel@ffwll.ch > Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; amd- > gfx@lists.freedesktop.org; platform-driver-x86@vger.kernel.org; dri- > devel@lists.freedesktop.org; Patil Rajesh <Patil.Reddy@amd.com>; linux- > input@vger.kernel.org; Limonciello, Mario <Mario.Limonciello@amd.com> > Subject: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set > interface > > 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. This seems to be limited to backlight at this point. What does setting or not setting the backlight level mean for the system when this framework is in place? What if a user manually changes the backlight level? Additional comments below. > > Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > 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; > + > + 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; Are we using standardized units for the brightness? On the GPU side, we align with the standard blacklight interface, but internally, the driver has to convert units depending on the type of backlight controller used on the platform. Presumably PMF does something similar? Alex > + 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 > -- > 2.25.1
On 9/25/2023 11:27, Deucher, Alexander wrote: > [Public] > >> -----Original Message----- >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >> Shyam Sundar S K >> Sent: Friday, September 22, 2023 1:51 PM >> To: hdegoede@redhat.com; markgross@kernel.org; Natikar, Basavaraj >> <Basavaraj.Natikar@amd.com>; jikos@kernel.org; >> benjamin.tissoires@redhat.com; Deucher, Alexander >> <Alexander.Deucher@amd.com>; Koenig, Christian >> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; >> airlied@gmail.com; daniel@ffwll.ch >> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; amd- >> gfx@lists.freedesktop.org; platform-driver-x86@vger.kernel.org; dri- >> devel@lists.freedesktop.org; Patil Rajesh <Patil.Reddy@amd.com>; linux- >> input@vger.kernel.org; Limonciello, Mario <Mario.Limonciello@amd.com> >> Subject: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set >> interface >> >> 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. > > This seems to be limited to backlight at this point. What does setting or not setting the backlight level mean for the system when this framework is in place? What if a user manually changes the backlight level? Additional comments below. > It's also for the display count. >> >> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> 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; >> + >> + 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; > > Are we using standardized units for the brightness? On the GPU side, we align with the standard blacklight interface, but internally, the driver has to convert units depending on the type of backlight controller used on the platform. Presumably PMF does something similar? > > Alex > >> + 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 >> -- >> 2.25.1 >
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@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > 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). 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 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
On 9/25/2023 9:57 PM, Deucher, Alexander wrote: > [Public] > >> -----Original Message----- >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >> Shyam Sundar S K >> Sent: Friday, September 22, 2023 1:51 PM >> To: hdegoede@redhat.com; markgross@kernel.org; Natikar, Basavaraj >> <Basavaraj.Natikar@amd.com>; jikos@kernel.org; >> benjamin.tissoires@redhat.com; Deucher, Alexander >> <Alexander.Deucher@amd.com>; Koenig, Christian >> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; >> airlied@gmail.com; daniel@ffwll.ch >> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; amd- >> gfx@lists.freedesktop.org; platform-driver-x86@vger.kernel.org; dri- >> devel@lists.freedesktop.org; Patil Rajesh <Patil.Reddy@amd.com>; linux- >> input@vger.kernel.org; Limonciello, Mario <Mario.Limonciello@amd.com> >> Subject: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set >> interface >> >> 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. > > This seems to be limited to backlight at this point. What does setting or not setting the backlight level mean for the system when this framework is in place? What if a user manually changes the backlight level? Additional comments below. The unit here is nits that varies from 0 to 255. User can manually update the backlight but if there is an action from the TA to update the backlight, PMF driver would send a request to GPU driver to update the backlight to the updated value (in nits) At this point, yes. PMF is using to PMF-GPU interface to set backlight, but there are additional things to be added in future. This patch builds the initial plumbing for that. > >> >> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> 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; >> + >> + 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; > > Are we using standardized units for the brightness? On the GPU side, we align with the standard blacklight interface, but internally, the driver has to convert units depending on the type of backlight controller used on the platform. Presumably PMF does something similar? Yes its the standard nits. There is no conversion needed. > > Alex > >> + 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 >> -- >> 2.25.1 >
On 9/25/2023 10:00 PM, Mario Limonciello wrote: > On 9/25/2023 11:27, Deucher, Alexander wrote: >> [Public] >> >>> -----Original Message----- >>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of >>> Shyam Sundar S K >>> Sent: Friday, September 22, 2023 1:51 PM >>> To: hdegoede@redhat.com; markgross@kernel.org; Natikar, Basavaraj >>> <Basavaraj.Natikar@amd.com>; jikos@kernel.org; >>> benjamin.tissoires@redhat.com; Deucher, Alexander >>> <Alexander.Deucher@amd.com>; Koenig, Christian >>> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; >>> airlied@gmail.com; daniel@ffwll.ch >>> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; amd- >>> gfx@lists.freedesktop.org; platform-driver-x86@vger.kernel.org; dri- >>> devel@lists.freedesktop.org; Patil Rajesh <Patil.Reddy@amd.com>; >>> linux- >>> input@vger.kernel.org; Limonciello, Mario <Mario.Limonciello@amd.com> >>> Subject: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set >>> interface >>> >>> 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. >> >> This seems to be limited to backlight at this point. What does >> setting or not setting the backlight level mean for the system when >> this framework is in place? What if a user manually changes the >> backlight level? Additional comments below. >> > > It's also for the display count. display count is on the PMF-GPU GET interface. On the SET interface its only backlight for today. Thanks, Shyam > >>> >>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>> --- >>> 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; >>> + >>> + 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; >> >> Are we using standardized units for the brightness? On the GPU >> side, we align with the standard blacklight interface, but >> internally, the driver has to convert units depending on the type of >> backlight controller used on the platform. Presumably PMF does >> something similar? >> >> Alex >> >>> + 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 >>> -- >>> 2.25.1 >> >
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@amd.com> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> 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 >
Hi, On 9/26/23 13:24, Shyam Sundar S K wrote: > 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@amd.com> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>> --- >>> 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? Users can pass say acpi_backlight=video and use the acpi_video driver for backlight control instead of the native GPU backlight control. > >> >> 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? Insist is a strong word, but I think that it is a good idea to have this. Evenutally it looks like this code will need to either integrate with the drm drivers lot more; or the drm core needs to export some special hooks for this which the PMF code can then call. Actually thinking more about this, I think that the right thing to do here is make some code register brightness control as a cooling device (which I think is already done in some cases) and then have the PMF code use the cooling-device APIs for this. IMHO that would be a much cleaner solution then this hack. Regards, Hans > > 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 >> >
Am 26.09.23 um 14:56 schrieb Hans de Goede: > Hi, > > On 9/26/23 13:24, Shyam Sundar S K wrote: >> 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@amd.com> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>> --- >>>> 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? > Users can pass say acpi_backlight=video and use the acpi_video > driver for backlight control instead of the native GPU backlight > control. > >>> 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? > Insist is a strong word, but I think that it is a good idea to have > this. Evenutally it looks like this code will need to either integrate with > the drm drivers lot more; or the drm core needs to export some special > hooks for this which the PMF code can then call. > > Actually thinking more about this, I think that the right thing to do > here is make some code register brightness control as a cooling device > (which I think is already done in some cases) and then have the PMF > code use the cooling-device APIs for this. > > IMHO that would be a much cleaner solution then this hack. Yeah, fully agree with Hans. This looks like a rather extreme hack to me. Apart from that what exactly is this thing supposed to do? Prevent overheating by reducing the brightness? Regards, Christian. > > Regards, > > Hans > > > >> 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
Hi Christian, On 9/26/2023 6:47 PM, Christian König wrote: > Am 26.09.23 um 14:56 schrieb Hans de Goede: >> Hi, >> >> On 9/26/23 13:24, Shyam Sundar S K wrote: >>> 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@amd.com> >>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>> --- >>>>> 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? >> Users can pass say acpi_backlight=video and use the acpi_video >> driver for backlight control instead of the native GPU backlight >> control. >> >>>> 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? >> Insist is a strong word, but I think that it is a good idea to have >> this. Evenutally it looks like this code will need to either >> integrate with >> the drm drivers lot more; or the drm core needs to export some special >> hooks for this which the PMF code can then call. >> >> Actually thinking more about this, I think that the right thing to do >> here is make some code register brightness control as a cooling device >> (which I think is already done in some cases) and then have the PMF >> code use the cooling-device APIs for this. >> >> IMHO that would be a much cleaner solution then this hack. > > Yeah, fully agree with Hans. This looks like a rather extreme hack to me. Sure. Let me think in this direction. > > Apart from that what exactly is this thing supposed to do? Prevent > overheating by reducing the brightness? Yes that can be one of the cases. But the thought process here is to help OEMs build their own policies. A policy is combination of inputs and outputs. Inputs are the conditions that has to be met and outputs are the actions to be set/done. The output actions come from PMF TA. One example policy apart from the case you mentioned is: if ambient light (received from amd_sfh) is low ; reduce the screen brightness (received from amdgpu) or vice versa. Thanks, Shyam > > Regards, > Christian. > >> >> Regards, >> >> Hans >> >> >> >>> 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 >
HI, On 9/26/23 15:17, Christian König wrote: > Am 26.09.23 um 14:56 schrieb Hans de Goede: >> Hi, >> >> On 9/26/23 13:24, Shyam Sundar S K wrote: >>> 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@amd.com> >>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>> --- >>>>> 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? >> Users can pass say acpi_backlight=video and use the acpi_video >> driver for backlight control instead of the native GPU backlight >> control. >> >>>> 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? >> Insist is a strong word, but I think that it is a good idea to have >> this. Evenutally it looks like this code will need to either integrate with >> the drm drivers lot more; or the drm core needs to export some special >> hooks for this which the PMF code can then call. >> >> Actually thinking more about this, I think that the right thing to do >> here is make some code register brightness control as a cooling device >> (which I think is already done in some cases) and then have the PMF >> code use the cooling-device APIs for this. >> >> IMHO that would be a much cleaner solution then this hack. > > Yeah, fully agree with Hans. This looks like a rather extreme hack to me. Shyam, the cooling device interface is defined in: include/linux/thermal.h And then look for cooling_device . An example of code registering a cooling_device for backlight control is: drivers/acpi/acpi_video.c and then specifically the code starting around line 257 with: video_get_max_state() until static const struct thermal_cooling_device_ops video_cooling_ops = { ... And the code around line 1750 for actually registering the cooling-dev. To use the cooling_device interface witt amdgpu's native backlight control you will need to make the amdgpu backlight control register a cooling-device for this in a similar manner. Regards, Hans > > Apart from that what exactly is this thing supposed to do? Prevent overheating by reducing the brightness? > > Regards, > Christian. > >> >> Regards, >> >> Hans >> >> >> >>> 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 >
On Fri, 22 Sep 2023, 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@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > 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; > + > + 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) Changing return type but no caller is changed to handle the error?? > { > 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 >
Hi Hans, On 9/27/2023 6:34 PM, Hans de Goede wrote: > HI, > > On 9/26/23 15:17, Christian König wrote: >> Am 26.09.23 um 14:56 schrieb Hans de Goede: >>> Hi, >>> >>> On 9/26/23 13:24, Shyam Sundar S K wrote: >>>> 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@amd.com> >>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>>> --- >>>>>> 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? >>> Users can pass say acpi_backlight=video and use the acpi_video >>> driver for backlight control instead of the native GPU backlight >>> control. >>> >>>>> 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? >>> Insist is a strong word, but I think that it is a good idea to have >>> this. Evenutally it looks like this code will need to either integrate with >>> the drm drivers lot more; or the drm core needs to export some special >>> hooks for this which the PMF code can then call. >>> >>> Actually thinking more about this, I think that the right thing to do >>> here is make some code register brightness control as a cooling device >>> (which I think is already done in some cases) and then have the PMF >>> code use the cooling-device APIs for this. >>> >>> IMHO that would be a much cleaner solution then this hack. >> >> Yeah, fully agree with Hans. This looks like a rather extreme hack to me. > > Shyam, the cooling device interface is defined in: > > include/linux/thermal.h > > And then look for cooling_device . > > An example of code registering a cooling_device for backlight control is: > > drivers/acpi/acpi_video.c > > and then specifically the code starting around line 257 with: > > video_get_max_state() > > until > > static const struct thermal_cooling_device_ops video_cooling_ops = { > ... > > And the code around line 1750 for actually registering the cooling-dev. > > To use the cooling_device interface witt amdgpu's native backlight control > you will need to make the amdgpu backlight control register a cooling-device > for this in a similar manner. > Thank you for pointing to the references. I will address the remarks from Ilpo and respin a new version soon. Thanks, Shyam > Regards, > > Hans > > > > >> >> Apart from that what exactly is this thing supposed to do? Prevent overheating by reducing the brightness? >> >> Regards, >> Christian. >> >>> >>> Regards, >>> >>> Hans >>> >>> >>> >>>> 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 >> >
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; + + 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