| Message ID | 20231018070241.2041529-1-Shyam-sundar.S-k@amd.com |
|---|---|
| Headers | show |
| Series | Introduce PMF Smart PC Solution Builder Feature | expand |
Am 18.10.23 um 11:28 schrieb Shyam Sundar S K: > > On 10/18/2023 2:50 PM, Ilpo Järvinen wrote: >> On Wed, 18 Oct 2023, Shyam Sundar S K wrote: >> >>> In order to provide GPU inputs to TA for the Smart PC solution to work, we >>> need to have interface between the PMF driver and the AMDGPU driver. >>> >>> Add the initial code path for get interface from 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/Makefile | 2 + >>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + >>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 138 ++++++++++++++++++++++++ >>> drivers/platform/x86/amd/pmf/Kconfig | 1 + >>> drivers/platform/x86/amd/pmf/core.c | 1 + >>> drivers/platform/x86/amd/pmf/pmf.h | 3 + >>> drivers/platform/x86/amd/pmf/spc.c | 13 +++ >>> drivers/platform/x86/amd/pmf/tee-if.c | 26 +++++ >>> include/linux/amd-pmf-io.h | 35 ++++++ >>> 9 files changed, 220 insertions(+) >>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>> create mode 100644 include/linux/amd-pmf-io.h >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile >>> index 384b798a9bad..7fafccefbd7a 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile >>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >>> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o >>> >>> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o >>> >>> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o >>> + >>> # add asic specific block >>> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \ >>> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>> index a79d53bdbe13..26ffa1b4fe57 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>> @@ -50,6 +50,7 @@ >>> #include <linux/hashtable.h> >>> #include <linux/dma-fence.h> >>> #include <linux/pci.h> >>> +#include <linux/amd-pmf-io.h> >>> >>> #include <drm/ttm/ttm_bo.h> >>> #include <drm/ttm/ttm_placement.h> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>> new file mode 100644 >>> index 000000000000..ac981848df50 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>> @@ -0,0 +1,138 @@ >>> +/* >>> + * Copyright 2023 Advanced Micro Devices, Inc. >>> + * >>> + * Permission is hereby granted, free of charge, to any person obtaining a >>> + * copy of this software and associated documentation files (the "Software"), >>> + * to deal in the Software without restriction, including without limitation >>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >>> + * and/or sell copies of the Software, and to permit persons to whom the >>> + * Software is furnished to do so, subject to the following conditions: >>> + * >>> + * The above copyright notice and this permission notice shall be included in >>> + * all copies or substantial portions of the Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR >>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >>> + * OTHER DEALINGS IN THE SOFTWARE. >> This is MIT, right? Add the required SPDX-License-Identifier line for it >> at the top of the file, thank you. >> > all the files in drivers/gpu/drm/amd/amdgpu/* carry the same license > text. So, have retained it to maintain uniformity. Please add the SPDX License Identifier for any file you add. Apart from that the whole approach of attaching this directly to amdgpu looks extremely questionable to me. Regards, Christian. > >>> + * >>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>> + * >>> + */ >> Remove the extra empty line at the end of the comment. >> > I just took the standard template for all the gpu files. Is that OK to > retain the blank line? > > If not, I can remove it in the next version. > > Rest all remarks I will address. > > Thanks, > Shyam > >>> + >>> +#include <linux/backlight.h> >>> +#include "amdgpu.h" >>> + >>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf) >>> +{ >>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev); >>> + struct drm_mode_config *mode_config = &drm_dev->mode_config; >>> + struct amdgpu_device *adev = drm_to_adev(drm_dev); >>> + struct drm_connector_list_iter iter; >>> + struct drm_connector *connector; >>> + int i = 0; >>> + >>> + /* Reset the count to zero */ >>> + pmf->display_count = 0; >>> + if (!(adev->flags & AMD_IS_APU)) { >>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>> + return -ENODEV; >>> + } >>> + >>> + mutex_lock(&mode_config->mutex); >>> + drm_connector_list_iter_begin(drm_dev, &iter); >>> + drm_for_each_connector_iter(connector, &iter) { >>> + if (connector->status == connector_status_connected) >>> + pmf->display_count++; >>> + if (connector->status != pmf->con_status[i]) >>> + pmf->con_status[i] = connector->status; >>> + if (connector->connector_type != pmf->connector_type[i]) >>> + pmf->connector_type[i] = connector->connector_type; >>> + >>> + i++; >>> + if (i >= MAX_SUPPORTED) >>> + break; >>> + } >>> + drm_connector_list_iter_end(&iter); >>> + mutex_unlock(&mode_config->mutex); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data); >>> + >>> +static int amd_pmf_gpu_get_cur_state(struct thermal_cooling_device *cooling_dev, >>> + unsigned long *state) >>> +{ >>> + struct backlight_device *bd; >>> + >>> + if (!acpi_video_backlight_use_native()) >>> + return -ENODEV; >>> + >>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>> + if (!bd) >>> + return -ENODEV; >>> + >>> + *state = backlight_get_brightness(bd); >>> + >>> + return 0; >>> +} >>> + >>> +static int amd_pmf_gpu_get_max_state(struct thermal_cooling_device *cooling_dev, >>> + unsigned long *state) >>> +{ >>> + struct backlight_device *bd; >>> + >>> + if (!acpi_video_backlight_use_native()) >>> + return -ENODEV; >>> + >>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>> + if (!bd) >>> + return -ENODEV; >>> + >>> + if (backlight_is_blank(bd)) >>> + *state = 0; >>> + else >>> + *state = bd->props.max_brightness; >>> + >>> + return 0; >>> +} >>> + >>> +static const struct thermal_cooling_device_ops bd_cooling_ops = { >>> + .get_max_state = amd_pmf_gpu_get_max_state, >>> + .get_cur_state = amd_pmf_gpu_get_cur_state, >>> +}; >>> + >>> +int amd_pmf_gpu_init(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); >>> + >>> + if (!(adev->flags & AMD_IS_APU)) { >>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (!acpi_video_backlight_use_native()) >>> + return -ENODEV; >>> + >>> + pmf->raw_bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>> + if (!pmf->raw_bd) >>> + return -ENODEV; >>> + >>> + pmf->cooling_dev = thermal_cooling_device_register("pmf_gpu_bd", >>> + pmf, &bd_cooling_ops); >>> + if (IS_ERR(pmf->cooling_dev)) >>> + return -ENODEV; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_init); >>> + >>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf) >>> +{ >>> + thermal_cooling_device_unregister(pmf->cooling_dev); >>> +} >>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_deinit); >>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig >>> index f246252bddd8..7f430de7af44 100644 >>> --- a/drivers/platform/x86/amd/pmf/Kconfig >>> +++ b/drivers/platform/x86/amd/pmf/Kconfig >>> @@ -10,6 +10,7 @@ config AMD_PMF >>> depends on AMD_NB >>> select ACPI_PLATFORM_PROFILE >>> depends on TEE && AMDTEE >>> + depends on DRM_AMDGPU >>> help >>> This driver provides support for the AMD Platform Management Framework. >>> The goal is to enhance end user experience by making AMD PCs smarter, >>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c >>> index 4b8156033fa6..c59ba527ff49 100644 >>> --- a/drivers/platform/x86/amd/pmf/core.c >>> +++ b/drivers/platform/x86/amd/pmf/core.c >>> @@ -411,6 +411,7 @@ static int amd_pmf_probe(struct platform_device *pdev) >>> } >>> >>> dev->cpu_id = rdev->device; >>> + dev->root = rdev; >>> >>> err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val); >>> if (err) { >>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h >>> index 8712299ad52b..615cd3a31986 100644 >>> --- a/drivers/platform/x86/amd/pmf/pmf.h >>> +++ b/drivers/platform/x86/amd/pmf/pmf.h >>> @@ -13,6 +13,7 @@ >>> >>> #include <linux/acpi.h> >>> #include <linux/platform_profile.h> >>> +#include <linux/amd-pmf-io.h> >>> >>> #define POLICY_BUF_MAX_SZ 0x4b000 >>> #define POLICY_SIGN_COOKIE 0x31535024 >>> @@ -228,9 +229,11 @@ struct amd_pmf_dev { >>> void *shbuf; >>> struct delayed_work pb_work; >>> struct pmf_action_table *prev_data; >>> + struct amd_gpu_pmf_data gfx_data; >>> u64 policy_addr; >>> void *policy_base; >>> bool smart_pc_enabled; >>> + struct pci_dev *root; >>> }; >>> >>> struct apmf_sps_prop_granular { >>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c >>> index 512e0c66efdc..cf4962ef97c2 100644 >>> --- a/drivers/platform/x86/amd/pmf/spc.c >>> +++ b/drivers/platform/x86/amd/pmf/spc.c >>> @@ -44,6 +44,10 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table * >>> dev_dbg(dev->dev, "Max C0 Residency : %u\n", in->ev_info.max_c0residency); >>> dev_dbg(dev->dev, "GFX Busy : %u\n", in->ev_info.gfx_busy); >>> dev_dbg(dev->dev, "Connected Display Count : %u\n", in->ev_info.monitor_count); >>> + dev_dbg(dev->dev, "Primary Display Type : %s\n", >>> + drm_get_connector_type_name(in->ev_info.display_type)); >>> + dev_dbg(dev->dev, "Primary Display State : %s\n", in->ev_info.display_state ? >>> + "Connected" : "disconnected/unknown"); >>> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open"); >>> dev_dbg(dev->dev, "==== TA inputs END ====\n"); >>> } >>> @@ -146,6 +150,14 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ >>> return 0; >>> } >>> >>> +static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in) >>> +{ >>> + amd_pmf_get_gfx_data(&dev->gfx_data); >>> + in->ev_info.monitor_count = dev->gfx_data.display_count; >>> + in->ev_info.display_type = dev->gfx_data.connector_type[0]; >>> + in->ev_info.display_state = dev->gfx_data.con_status[0]; >>> +} >>> + >>> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in) >>> { >>> /* TA side lid open is 1 and close is 0, hence the ! here */ >>> @@ -154,4 +166,5 @@ void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_tab >>> amd_pmf_get_smu_info(dev, in); >>> amd_pmf_get_battery_info(dev, in); >>> amd_pmf_get_slider_info(dev, in); >>> + amd_pmf_get_gpu_info(dev, in); >>> } >>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c >>> index 2f5fb8623c20..956e66b78605 100644 >>> --- a/drivers/platform/x86/amd/pmf/tee-if.c >>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c >>> @@ -9,6 +9,7 @@ >>> */ >>> >>> #include <linux/debugfs.h> >>> +#include <linux/pci.h> >>> #include <linux/tee_drv.h> >>> #include <linux/uuid.h> >>> #include "pmf.h" >>> @@ -357,6 +358,19 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev) >>> return amd_pmf_start_policy_engine(dev); >>> } >>> >>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data) >>> +{ >>> + struct amd_pmf_dev *dev = data; >>> + >>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) { >>> + /* Found the amdgpu handle from the pci root after walking through the pci bus */ >> If you insist on having this comment, make it a function comment instead >> (with appropriate modifications into the content of it) but I personally >> don't find it that useful so it could be just dropped as well, IMO. >> >>> + dev->gfx_data.gpu_dev = pdev; >>> + return 1; /* Stop walking */ >>> + } >>> + >>> + return 0; /* Continue walking */ >>> +} >>> + >>> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data) >>> { >>> return ver->impl_id == TEE_IMPL_ID_AMDTEE; >>> @@ -446,6 +460,15 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) >>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd); >>> amd_pmf_set_dram_addr(dev); >>> amd_pmf_get_bios_buffer(dev); >>> + >>> + /* Get amdgpu handle */ >> Useless comment. >> >>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev); >>> + if (!dev->gfx_data.gpu_dev) >>> + dev_err(dev->dev, "GPU handle not found!\n"); >>> + >>> + if (!amd_pmf_gpu_init(&dev->gfx_data)) >>> + dev->gfx_data.gpu_dev_en = true; >>> + >>> dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL); >>> if (!dev->prev_data) >>> return -ENOMEM; >>> @@ -461,5 +484,8 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev) >>> kfree(dev->prev_data); >>> kfree(dev->policy_buf); >>> cancel_delayed_work_sync(&dev->pb_work); >>> + if (dev->gfx_data.gpu_dev_en) >>> + amd_pmf_gpu_deinit(&dev->gfx_data); >>> + pci_dev_put(dev->gfx_data.gpu_dev); >>> amd_pmf_tee_deinit(dev); >>> } >>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h >>> new file mode 100644 >>> index 000000000000..5f79e66a41b3 >>> --- /dev/null >>> +++ b/include/linux/amd-pmf-io.h >>> @@ -0,0 +1,35 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * AMD Platform Management Framework Interface >>> + * >>> + * Copyright (c) 2023, Advanced Micro Devices, Inc. >>> + * All Rights Reserved. >>> + * >>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>> + */ >>> + >>> +#ifndef AMD_PMF_IO_H >>> +#define AMD_PMF_IO_H >>> + >>> +#include <acpi/video.h> >>> +#include <drm/drm_connector.h> >>> +#include <linux/backlight.h> >>> +#include <linux/thermal.h> >>> + >>> +#define MAX_SUPPORTED 4 >>> + >>> +/* amdgpu */ >> Document the structure properly with kerneldoc instead of an unhelpful >> comment like above :-). Please also check if you add any other structs >> into kernel-wide headers that you didn't document yet. Or fields into >> existing structs. >> >>> +struct amd_gpu_pmf_data { >>> + struct pci_dev *gpu_dev; >>> + struct backlight_device *raw_bd; >>> + struct thermal_cooling_device *cooling_dev; >>> + enum drm_connector_status con_status[MAX_SUPPORTED]; >>> + int display_count; >>> + int connector_type[MAX_SUPPORTED]; >>> + bool gpu_dev_en; >>> +}; >>> + >>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf); >>> +int amd_pmf_gpu_init(struct amd_gpu_pmf_data *pmf); >>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf); >>> +#endif >>>
On 10/18/2023 08:40, Christian König wrote: > > > Am 18.10.23 um 11:28 schrieb Shyam Sundar S K: >> >> On 10/18/2023 2:50 PM, Ilpo Järvinen wrote: >>> On Wed, 18 Oct 2023, Shyam Sundar S K wrote: >>> >>>> In order to provide GPU inputs to TA for the Smart PC solution to >>>> work, we >>>> need to have interface between the PMF driver and the AMDGPU driver. >>>> >>>> Add the initial code path for get interface from 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/Makefile | 2 + >>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 138 >>>> ++++++++++++++++++++++++ >>>> drivers/platform/x86/amd/pmf/Kconfig | 1 + >>>> drivers/platform/x86/amd/pmf/core.c | 1 + >>>> drivers/platform/x86/amd/pmf/pmf.h | 3 + >>>> drivers/platform/x86/amd/pmf/spc.c | 13 +++ >>>> drivers/platform/x86/amd/pmf/tee-if.c | 26 +++++ >>>> include/linux/amd-pmf-io.h | 35 ++++++ >>>> 9 files changed, 220 insertions(+) >>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>> create mode 100644 include/linux/amd-pmf-io.h >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile >>>> b/drivers/gpu/drm/amd/amdgpu/Makefile >>>> index 384b798a9bad..7fafccefbd7a 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile >>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >>>> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o >>>> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o >>>> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o >>>> + >>>> # add asic specific block >>>> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \ >>>> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> index a79d53bdbe13..26ffa1b4fe57 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> @@ -50,6 +50,7 @@ >>>> #include <linux/hashtable.h> >>>> #include <linux/dma-fence.h> >>>> #include <linux/pci.h> >>>> +#include <linux/amd-pmf-io.h> >>>> #include <drm/ttm/ttm_bo.h> >>>> #include <drm/ttm/ttm_placement.h> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>> new file mode 100644 >>>> index 000000000000..ac981848df50 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>> @@ -0,0 +1,138 @@ >>>> +/* >>>> + * Copyright 2023 Advanced Micro Devices, Inc. >>>> + * >>>> + * Permission is hereby granted, free of charge, to any person >>>> obtaining a >>>> + * copy of this software and associated documentation files (the >>>> "Software"), >>>> + * to deal in the Software without restriction, including without >>>> limitation >>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>> sublicense, >>>> + * and/or sell copies of the Software, and to permit persons to >>>> whom the >>>> + * Software is furnished to do so, subject to the following >>>> conditions: >>>> + * >>>> + * The above copyright notice and this permission notice shall be >>>> included in >>>> + * all copies or substantial portions of the Software. >>>> + * >>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>> EXPRESS OR >>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>> MERCHANTABILITY, >>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>> EVENT SHALL >>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, >>>> DAMAGES OR >>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR >>>> OTHERWISE, >>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE >>>> USE OR >>>> + * OTHER DEALINGS IN THE SOFTWARE. >>> This is MIT, right? Add the required SPDX-License-Identifier line for it >>> at the top of the file, thank you. >>> >> all the files in drivers/gpu/drm/amd/amdgpu/* carry the same license >> text. So, have retained it to maintain uniformity. > > Please add the SPDX License Identifier for any file you add. > > Apart from that the whole approach of attaching this directly to amdgpu > looks extremely questionable to me. > What's the long term outlook for things that are needed directly from amdgpu? Is there going to be more besides the backlight and the display count/type? At least for the display count I suppose one way that it could be "decoupled" from amdgpu is to use drm_for_each_connector_iter to iterate all the connectors and then count the connected ones. > Regards, > Christian. > >> >>>> + * >>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>> + * >>>> + */ >>> Remove the extra empty line at the end of the comment. >>> >> I just took the standard template for all the gpu files. Is that OK to >> retain the blank line? >> >> If not, I can remove it in the next version. >> >> Rest all remarks I will address. >> >> Thanks, >> Shyam >> >>>> + >>>> +#include <linux/backlight.h> >>>> +#include "amdgpu.h" >>>> + >>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf) >>>> +{ >>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev); >>>> + struct drm_mode_config *mode_config = &drm_dev->mode_config; >>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev); >>>> + struct drm_connector_list_iter iter; >>>> + struct drm_connector *connector; >>>> + int i = 0; >>>> + >>>> + /* Reset the count to zero */ >>>> + pmf->display_count = 0; >>>> + if (!(adev->flags & AMD_IS_APU)) { >>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + mutex_lock(&mode_config->mutex); >>>> + drm_connector_list_iter_begin(drm_dev, &iter); >>>> + drm_for_each_connector_iter(connector, &iter) { >>>> + if (connector->status == connector_status_connected) >>>> + pmf->display_count++; >>>> + if (connector->status != pmf->con_status[i]) >>>> + pmf->con_status[i] = connector->status; >>>> + if (connector->connector_type != pmf->connector_type[i]) >>>> + pmf->connector_type[i] = connector->connector_type; >>>> + >>>> + i++; >>>> + if (i >= MAX_SUPPORTED) >>>> + break; >>>> + } >>>> + drm_connector_list_iter_end(&iter); >>>> + mutex_unlock(&mode_config->mutex); >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data); >>>> + >>>> +static int amd_pmf_gpu_get_cur_state(struct thermal_cooling_device >>>> *cooling_dev, >>>> + unsigned long *state) >>>> +{ >>>> + struct backlight_device *bd; >>>> + >>>> + if (!acpi_video_backlight_use_native()) >>>> + return -ENODEV; >>>> + >>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>> + if (!bd) >>>> + return -ENODEV; >>>> + >>>> + *state = backlight_get_brightness(bd); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int amd_pmf_gpu_get_max_state(struct thermal_cooling_device >>>> *cooling_dev, >>>> + unsigned long *state) >>>> +{ >>>> + struct backlight_device *bd; >>>> + >>>> + if (!acpi_video_backlight_use_native()) >>>> + return -ENODEV; >>>> + >>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>> + if (!bd) >>>> + return -ENODEV; >>>> + >>>> + if (backlight_is_blank(bd)) >>>> + *state = 0; >>>> + else >>>> + *state = bd->props.max_brightness; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = { >>>> + .get_max_state = amd_pmf_gpu_get_max_state, >>>> + .get_cur_state = amd_pmf_gpu_get_cur_state, >>>> +}; >>>> + >>>> +int amd_pmf_gpu_init(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); >>>> + >>>> + if (!(adev->flags & AMD_IS_APU)) { >>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + if (!acpi_video_backlight_use_native()) >>>> + return -ENODEV; >>>> + >>>> + pmf->raw_bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>> + if (!pmf->raw_bd) >>>> + return -ENODEV; >>>> + >>>> + pmf->cooling_dev = thermal_cooling_device_register("pmf_gpu_bd", >>>> + pmf, &bd_cooling_ops); >>>> + if (IS_ERR(pmf->cooling_dev)) >>>> + return -ENODEV; >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_init); >>>> + >>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf) >>>> +{ >>>> + thermal_cooling_device_unregister(pmf->cooling_dev); >>>> +} >>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_deinit); >>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig >>>> b/drivers/platform/x86/amd/pmf/Kconfig >>>> index f246252bddd8..7f430de7af44 100644 >>>> --- a/drivers/platform/x86/amd/pmf/Kconfig >>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig >>>> @@ -10,6 +10,7 @@ config AMD_PMF >>>> depends on AMD_NB >>>> select ACPI_PLATFORM_PROFILE >>>> depends on TEE && AMDTEE >>>> + depends on DRM_AMDGPU >>>> help >>>> This driver provides support for the AMD Platform Management >>>> Framework. >>>> The goal is to enhance end user experience by making AMD PCs >>>> smarter, >>>> diff --git a/drivers/platform/x86/amd/pmf/core.c >>>> b/drivers/platform/x86/amd/pmf/core.c >>>> index 4b8156033fa6..c59ba527ff49 100644 >>>> --- a/drivers/platform/x86/amd/pmf/core.c >>>> +++ b/drivers/platform/x86/amd/pmf/core.c >>>> @@ -411,6 +411,7 @@ static int amd_pmf_probe(struct platform_device >>>> *pdev) >>>> } >>>> dev->cpu_id = rdev->device; >>>> + dev->root = rdev; >>>> err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val); >>>> if (err) { >>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h >>>> b/drivers/platform/x86/amd/pmf/pmf.h >>>> index 8712299ad52b..615cd3a31986 100644 >>>> --- a/drivers/platform/x86/amd/pmf/pmf.h >>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h >>>> @@ -13,6 +13,7 @@ >>>> #include <linux/acpi.h> >>>> #include <linux/platform_profile.h> >>>> +#include <linux/amd-pmf-io.h> >>>> #define POLICY_BUF_MAX_SZ 0x4b000 >>>> #define POLICY_SIGN_COOKIE 0x31535024 >>>> @@ -228,9 +229,11 @@ struct amd_pmf_dev { >>>> void *shbuf; >>>> struct delayed_work pb_work; >>>> struct pmf_action_table *prev_data; >>>> + struct amd_gpu_pmf_data gfx_data; >>>> u64 policy_addr; >>>> void *policy_base; >>>> bool smart_pc_enabled; >>>> + struct pci_dev *root; >>>> }; >>>> struct apmf_sps_prop_granular { >>>> diff --git a/drivers/platform/x86/amd/pmf/spc.c >>>> b/drivers/platform/x86/amd/pmf/spc.c >>>> index 512e0c66efdc..cf4962ef97c2 100644 >>>> --- a/drivers/platform/x86/amd/pmf/spc.c >>>> +++ b/drivers/platform/x86/amd/pmf/spc.c >>>> @@ -44,6 +44,10 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev >>>> *dev, struct ta_pmf_enact_table * >>>> dev_dbg(dev->dev, "Max C0 Residency : %u\n", >>>> in->ev_info.max_c0residency); >>>> dev_dbg(dev->dev, "GFX Busy : %u\n", in->ev_info.gfx_busy); >>>> dev_dbg(dev->dev, "Connected Display Count : %u\n", >>>> in->ev_info.monitor_count); >>>> + dev_dbg(dev->dev, "Primary Display Type : %s\n", >>>> + drm_get_connector_type_name(in->ev_info.display_type)); >>>> + dev_dbg(dev->dev, "Primary Display State : %s\n", >>>> in->ev_info.display_state ? >>>> + "Connected" : "disconnected/unknown"); >>>> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? >>>> "Close" : "Open"); >>>> dev_dbg(dev->dev, "==== TA inputs END ====\n"); >>>> } >>>> @@ -146,6 +150,14 @@ static int amd_pmf_get_slider_info(struct >>>> amd_pmf_dev *dev, struct ta_pmf_enact_ >>>> return 0; >>>> } >>>> +static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev, struct >>>> ta_pmf_enact_table *in) >>>> +{ >>>> + amd_pmf_get_gfx_data(&dev->gfx_data); >>>> + in->ev_info.monitor_count = dev->gfx_data.display_count; >>>> + in->ev_info.display_type = dev->gfx_data.connector_type[0]; >>>> + in->ev_info.display_state = dev->gfx_data.con_status[0]; >>>> +} >>>> + >>>> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct >>>> ta_pmf_enact_table *in) >>>> { >>>> /* TA side lid open is 1 and close is 0, hence the ! here */ >>>> @@ -154,4 +166,5 @@ void amd_pmf_populate_ta_inputs(struct >>>> amd_pmf_dev *dev, struct ta_pmf_enact_tab >>>> amd_pmf_get_smu_info(dev, in); >>>> amd_pmf_get_battery_info(dev, in); >>>> amd_pmf_get_slider_info(dev, in); >>>> + amd_pmf_get_gpu_info(dev, in); >>>> } >>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c >>>> b/drivers/platform/x86/amd/pmf/tee-if.c >>>> index 2f5fb8623c20..956e66b78605 100644 >>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c >>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c >>>> @@ -9,6 +9,7 @@ >>>> */ >>>> #include <linux/debugfs.h> >>>> +#include <linux/pci.h> >>>> #include <linux/tee_drv.h> >>>> #include <linux/uuid.h> >>>> #include "pmf.h" >>>> @@ -357,6 +358,19 @@ static int amd_pmf_get_bios_buffer(struct >>>> amd_pmf_dev *dev) >>>> return amd_pmf_start_policy_engine(dev); >>>> } >>>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data) >>>> +{ >>>> + struct amd_pmf_dev *dev = data; >>>> + >>>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) { >>>> + /* Found the amdgpu handle from the pci root after walking >>>> through the pci bus */ >>> If you insist on having this comment, make it a function comment instead >>> (with appropriate modifications into the content of it) but I personally >>> don't find it that useful so it could be just dropped as well, IMO. >>> >>>> + dev->gfx_data.gpu_dev = pdev; >>>> + return 1; /* Stop walking */ >>>> + } >>>> + >>>> + return 0; /* Continue walking */ >>>> +} >>>> + >>>> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data >>>> *ver, const void *data) >>>> { >>>> return ver->impl_id == TEE_IMPL_ID_AMDTEE; >>>> @@ -446,6 +460,15 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) >>>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd); >>>> amd_pmf_set_dram_addr(dev); >>>> amd_pmf_get_bios_buffer(dev); >>>> + >>>> + /* Get amdgpu handle */ >>> Useless comment. >>> >>>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev); >>>> + if (!dev->gfx_data.gpu_dev) >>>> + dev_err(dev->dev, "GPU handle not found!\n"); >>>> + >>>> + if (!amd_pmf_gpu_init(&dev->gfx_data)) >>>> + dev->gfx_data.gpu_dev_en = true; >>>> + >>>> dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL); >>>> if (!dev->prev_data) >>>> return -ENOMEM; >>>> @@ -461,5 +484,8 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev >>>> *dev) >>>> kfree(dev->prev_data); >>>> kfree(dev->policy_buf); >>>> cancel_delayed_work_sync(&dev->pb_work); >>>> + if (dev->gfx_data.gpu_dev_en) >>>> + amd_pmf_gpu_deinit(&dev->gfx_data); >>>> + pci_dev_put(dev->gfx_data.gpu_dev); >>>> amd_pmf_tee_deinit(dev); >>>> } >>>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h >>>> new file mode 100644 >>>> index 000000000000..5f79e66a41b3 >>>> --- /dev/null >>>> +++ b/include/linux/amd-pmf-io.h >>>> @@ -0,0 +1,35 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* >>>> + * AMD Platform Management Framework Interface >>>> + * >>>> + * Copyright (c) 2023, Advanced Micro Devices, Inc. >>>> + * All Rights Reserved. >>>> + * >>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>> + */ >>>> + >>>> +#ifndef AMD_PMF_IO_H >>>> +#define AMD_PMF_IO_H >>>> + >>>> +#include <acpi/video.h> >>>> +#include <drm/drm_connector.h> >>>> +#include <linux/backlight.h> >>>> +#include <linux/thermal.h> >>>> + >>>> +#define MAX_SUPPORTED 4 >>>> + >>>> +/* amdgpu */ >>> Document the structure properly with kerneldoc instead of an unhelpful >>> comment like above :-). Please also check if you add any other structs >>> into kernel-wide headers that you didn't document yet. Or fields into >>> existing structs. >>> >>>> +struct amd_gpu_pmf_data { >>>> + struct pci_dev *gpu_dev; >>>> + struct backlight_device *raw_bd; >>>> + struct thermal_cooling_device *cooling_dev; >>>> + enum drm_connector_status con_status[MAX_SUPPORTED]; >>>> + int display_count; >>>> + int connector_type[MAX_SUPPORTED]; >>>> + bool gpu_dev_en; >>>> +}; >>>> + >>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf); >>>> +int amd_pmf_gpu_init(struct amd_gpu_pmf_data *pmf); >>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf); >>>> +#endif >>>> >
On 10/18/2023 9:37 PM, Christian König wrote: > Am 18.10.23 um 17:47 schrieb Mario Limonciello: >> On 10/18/2023 08:40, Christian König wrote: >>> >>> >>> Am 18.10.23 um 11:28 schrieb Shyam Sundar S K: >>>> >>>> On 10/18/2023 2:50 PM, Ilpo Järvinen wrote: >>>>> On Wed, 18 Oct 2023, Shyam Sundar S K wrote: >>>>> >>>>>> In order to provide GPU inputs to TA for the Smart PC solution >>>>>> to work, we >>>>>> need to have interface between the PMF driver and the AMDGPU >>>>>> driver. >>>>>> >>>>>> Add the initial code path for get interface from 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/Makefile | 2 + >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 138 >>>>>> ++++++++++++++++++++++++ >>>>>> drivers/platform/x86/amd/pmf/Kconfig | 1 + >>>>>> drivers/platform/x86/amd/pmf/core.c | 1 + >>>>>> drivers/platform/x86/amd/pmf/pmf.h | 3 + >>>>>> drivers/platform/x86/amd/pmf/spc.c | 13 +++ >>>>>> drivers/platform/x86/amd/pmf/tee-if.c | 26 +++++ >>>>>> include/linux/amd-pmf-io.h | 35 ++++++ >>>>>> 9 files changed, 220 insertions(+) >>>>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>> create mode 100644 include/linux/amd-pmf-io.h >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>> index 384b798a9bad..7fafccefbd7a 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o >>>>>> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o >>>>>> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o >>>>>> + >>>>>> # add asic specific block >>>>>> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \ >>>>>> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> index a79d53bdbe13..26ffa1b4fe57 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> @@ -50,6 +50,7 @@ >>>>>> #include <linux/hashtable.h> >>>>>> #include <linux/dma-fence.h> >>>>>> #include <linux/pci.h> >>>>>> +#include <linux/amd-pmf-io.h> >>>>>> #include <drm/ttm/ttm_bo.h> >>>>>> #include <drm/ttm/ttm_placement.h> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>> new file mode 100644 >>>>>> index 000000000000..ac981848df50 >>>>>> --- /dev/null >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>> @@ -0,0 +1,138 @@ >>>>>> +/* >>>>>> + * Copyright 2023 Advanced Micro Devices, Inc. >>>>>> + * >>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>> obtaining a >>>>>> + * copy of this software and associated documentation files >>>>>> (the "Software"), >>>>>> + * to deal in the Software without restriction, including >>>>>> without limitation >>>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>>> sublicense, >>>>>> + * and/or sell copies of the Software, and to permit persons to >>>>>> whom the >>>>>> + * Software is furnished to do so, subject to the following >>>>>> conditions: >>>>>> + * >>>>>> + * The above copyright notice and this permission notice shall >>>>>> be included in >>>>>> + * all copies or substantial portions of the Software. >>>>>> + * >>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY >>>>>> KIND, EXPRESS OR >>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>> MERCHANTABILITY, >>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>>>> EVENT SHALL >>>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY >>>>>> CLAIM, DAMAGES OR >>>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR >>>>>> OTHERWISE, >>>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR >>>>>> THE USE OR >>>>>> + * OTHER DEALINGS IN THE SOFTWARE. >>>>> This is MIT, right? Add the required SPDX-License-Identifier line >>>>> for it >>>>> at the top of the file, thank you. >>>>> >>>> all the files in drivers/gpu/drm/amd/amdgpu/* carry the same license >>>> text. So, have retained it to maintain uniformity. >>> >>> Please add the SPDX License Identifier for any file you add. OK. I thought to keep it same like the other files. I will change this to SPDX in the next revision. >>> >>> Apart from that the whole approach of attaching this directly to >>> amdgpu looks extremely questionable to me. >>> >> >> What's the long term outlook for things that are needed directly >> from amdgpu? Is there going to be more besides the backlight and >> the display count/type? > > Yeah, that goes into the same direction as my concern. PMF is an APU only feature and will need inputs from multiple subsystems (in this case its GPU) to send changing system information to PMF TA (Trusted Application, running on ASP/PSP) as input. Its not only about the display count/type and backlight, there are many others things in pipe like the GPU engine running time, utilization percentage etc, that guide the TA to make better decisions. This series is only targeted to build the initial plubming with the subsystems inside the kernel and later keep adding changes to get more information from other subsystems. that is the reason you see that, this patch proposes amd-pmf-io.h as the interface to talk to other subsystems. For the initial path, I have opted to get information from SFH and GPU drivers. But this is meant to grow in future. > >> >> At least for the display count I suppose one way that it could be >> "decoupled" from amdgpu is to use drm_for_each_connector_iter to >> iterate all the connectors and then count the connected ones. > > And what if the number of connected displays change? How is amdgpu > supposed to signal events like that? PMF driver polls for the information based on a configurable sampling frequency. you can look at amd_pmf_get_gpu_info(), that gets called from amd_pmf_populate_ta_inputs() in spc.c in this proposed series. Thanks, Shyam > > This whole solution doesn't looks well thought through. > > Regards, > Christian. > >> >>> Regards, >>> Christian. >>> >>>> >>>>>> + * >>>>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>>> + * >>>>>> + */ >>>>> Remove the extra empty line at the end of the comment. >>>>> >>>> I just took the standard template for all the gpu files. Is that >>>> OK to >>>> retain the blank line? >>>> >>>> If not, I can remove it in the next version. >>>> >>>> Rest all remarks I will address. >>>> >>>> Thanks, >>>> Shyam >>>> >>>>>> + >>>>>> +#include <linux/backlight.h> >>>>>> +#include "amdgpu.h" >>>>>> + >>>>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf) >>>>>> +{ >>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev); >>>>>> + struct drm_mode_config *mode_config = &drm_dev->mode_config; >>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev); >>>>>> + struct drm_connector_list_iter iter; >>>>>> + struct drm_connector *connector; >>>>>> + int i = 0; >>>>>> + >>>>>> + /* Reset the count to zero */ >>>>>> + pmf->display_count = 0; >>>>>> + if (!(adev->flags & AMD_IS_APU)) { >>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>>>>> + return -ENODEV; >>>>>> + } >>>>>> + >>>>>> + mutex_lock(&mode_config->mutex); >>>>>> + drm_connector_list_iter_begin(drm_dev, &iter); >>>>>> + drm_for_each_connector_iter(connector, &iter) { >>>>>> + if (connector->status == connector_status_connected) >>>>>> + pmf->display_count++; >>>>>> + if (connector->status != pmf->con_status[i]) >>>>>> + pmf->con_status[i] = connector->status; >>>>>> + if (connector->connector_type != pmf->connector_type[i]) >>>>>> + pmf->connector_type[i] = connector->connector_type; >>>>>> + >>>>>> + i++; >>>>>> + if (i >= MAX_SUPPORTED) >>>>>> + break; >>>>>> + } >>>>>> + drm_connector_list_iter_end(&iter); >>>>>> + mutex_unlock(&mode_config->mutex); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data); >>>>>> + >>>>>> +static int amd_pmf_gpu_get_cur_state(struct >>>>>> thermal_cooling_device *cooling_dev, >>>>>> + unsigned long *state) >>>>>> +{ >>>>>> + struct backlight_device *bd; >>>>>> + >>>>>> + if (!acpi_video_backlight_use_native()) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>> + if (!bd) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + *state = backlight_get_brightness(bd); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int amd_pmf_gpu_get_max_state(struct >>>>>> thermal_cooling_device *cooling_dev, >>>>>> + unsigned long *state) >>>>>> +{ >>>>>> + struct backlight_device *bd; >>>>>> + >>>>>> + if (!acpi_video_backlight_use_native()) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>> + if (!bd) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + if (backlight_is_blank(bd)) >>>>>> + *state = 0; >>>>>> + else >>>>>> + *state = bd->props.max_brightness; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = { >>>>>> + .get_max_state = amd_pmf_gpu_get_max_state, >>>>>> + .get_cur_state = amd_pmf_gpu_get_cur_state, >>>>>> +}; >>>>>> + >>>>>> +int amd_pmf_gpu_init(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); >>>>>> + >>>>>> + if (!(adev->flags & AMD_IS_APU)) { >>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>>>>> + return -ENODEV; >>>>>> + } >>>>>> + >>>>>> + if (!acpi_video_backlight_use_native()) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + pmf->raw_bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>> + if (!pmf->raw_bd) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + pmf->cooling_dev = >>>>>> thermal_cooling_device_register("pmf_gpu_bd", >>>>>> + pmf, &bd_cooling_ops); >>>>>> + if (IS_ERR(pmf->cooling_dev)) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_init); >>>>>> + >>>>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf) >>>>>> +{ >>>>>> + thermal_cooling_device_unregister(pmf->cooling_dev); >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_deinit); >>>>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig >>>>>> b/drivers/platform/x86/amd/pmf/Kconfig >>>>>> index f246252bddd8..7f430de7af44 100644 >>>>>> --- a/drivers/platform/x86/amd/pmf/Kconfig >>>>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig >>>>>> @@ -10,6 +10,7 @@ config AMD_PMF >>>>>> depends on AMD_NB >>>>>> select ACPI_PLATFORM_PROFILE >>>>>> depends on TEE && AMDTEE >>>>>> + depends on DRM_AMDGPU >>>>>> help >>>>>> This driver provides support for the AMD Platform >>>>>> Management Framework. >>>>>> The goal is to enhance end user experience by making AMD >>>>>> PCs smarter, >>>>>> diff --git a/drivers/platform/x86/amd/pmf/core.c >>>>>> b/drivers/platform/x86/amd/pmf/core.c >>>>>> index 4b8156033fa6..c59ba527ff49 100644 >>>>>> --- a/drivers/platform/x86/amd/pmf/core.c >>>>>> +++ b/drivers/platform/x86/amd/pmf/core.c >>>>>> @@ -411,6 +411,7 @@ static int amd_pmf_probe(struct >>>>>> platform_device *pdev) >>>>>> } >>>>>> dev->cpu_id = rdev->device; >>>>>> + dev->root = rdev; >>>>>> err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val); >>>>>> if (err) { >>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h >>>>>> b/drivers/platform/x86/amd/pmf/pmf.h >>>>>> index 8712299ad52b..615cd3a31986 100644 >>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h >>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h >>>>>> @@ -13,6 +13,7 @@ >>>>>> #include <linux/acpi.h> >>>>>> #include <linux/platform_profile.h> >>>>>> +#include <linux/amd-pmf-io.h> >>>>>> #define POLICY_BUF_MAX_SZ 0x4b000 >>>>>> #define POLICY_SIGN_COOKIE 0x31535024 >>>>>> @@ -228,9 +229,11 @@ struct amd_pmf_dev { >>>>>> void *shbuf; >>>>>> struct delayed_work pb_work; >>>>>> struct pmf_action_table *prev_data; >>>>>> + struct amd_gpu_pmf_data gfx_data; >>>>>> u64 policy_addr; >>>>>> void *policy_base; >>>>>> bool smart_pc_enabled; >>>>>> + struct pci_dev *root; >>>>>> }; >>>>>> struct apmf_sps_prop_granular { >>>>>> diff --git a/drivers/platform/x86/amd/pmf/spc.c >>>>>> b/drivers/platform/x86/amd/pmf/spc.c >>>>>> index 512e0c66efdc..cf4962ef97c2 100644 >>>>>> --- a/drivers/platform/x86/amd/pmf/spc.c >>>>>> +++ b/drivers/platform/x86/amd/pmf/spc.c >>>>>> @@ -44,6 +44,10 @@ void amd_pmf_dump_ta_inputs(struct >>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_table * >>>>>> dev_dbg(dev->dev, "Max C0 Residency : %u\n", >>>>>> in->ev_info.max_c0residency); >>>>>> dev_dbg(dev->dev, "GFX Busy : %u\n", in->ev_info.gfx_busy); >>>>>> dev_dbg(dev->dev, "Connected Display Count : %u\n", >>>>>> in->ev_info.monitor_count); >>>>>> + dev_dbg(dev->dev, "Primary Display Type : %s\n", >>>>>> + drm_get_connector_type_name(in->ev_info.display_type)); >>>>>> + dev_dbg(dev->dev, "Primary Display State : %s\n", >>>>>> in->ev_info.display_state ? >>>>>> + "Connected" : "disconnected/unknown"); >>>>>> dev_dbg(dev->dev, "LID State : %s\n", >>>>>> in->ev_info.lid_state ? "Close" : "Open"); >>>>>> dev_dbg(dev->dev, "==== TA inputs END ====\n"); >>>>>> } >>>>>> @@ -146,6 +150,14 @@ static int amd_pmf_get_slider_info(struct >>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_ >>>>>> return 0; >>>>>> } >>>>>> +static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev, >>>>>> struct ta_pmf_enact_table *in) >>>>>> +{ >>>>>> + amd_pmf_get_gfx_data(&dev->gfx_data); >>>>>> + in->ev_info.monitor_count = dev->gfx_data.display_count; >>>>>> + in->ev_info.display_type = dev->gfx_data.connector_type[0]; >>>>>> + in->ev_info.display_state = dev->gfx_data.con_status[0]; >>>>>> +} >>>>>> + >>>>>> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, >>>>>> struct ta_pmf_enact_table *in) >>>>>> { >>>>>> /* TA side lid open is 1 and close is 0, hence the ! here */ >>>>>> @@ -154,4 +166,5 @@ void amd_pmf_populate_ta_inputs(struct >>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_tab >>>>>> amd_pmf_get_smu_info(dev, in); >>>>>> amd_pmf_get_battery_info(dev, in); >>>>>> amd_pmf_get_slider_info(dev, in); >>>>>> + amd_pmf_get_gpu_info(dev, in); >>>>>> } >>>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c >>>>>> b/drivers/platform/x86/amd/pmf/tee-if.c >>>>>> index 2f5fb8623c20..956e66b78605 100644 >>>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c >>>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c >>>>>> @@ -9,6 +9,7 @@ >>>>>> */ >>>>>> #include <linux/debugfs.h> >>>>>> +#include <linux/pci.h> >>>>>> #include <linux/tee_drv.h> >>>>>> #include <linux/uuid.h> >>>>>> #include "pmf.h" >>>>>> @@ -357,6 +358,19 @@ static int amd_pmf_get_bios_buffer(struct >>>>>> amd_pmf_dev *dev) >>>>>> return amd_pmf_start_policy_engine(dev); >>>>>> } >>>>>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void >>>>>> *data) >>>>>> +{ >>>>>> + struct amd_pmf_dev *dev = data; >>>>>> + >>>>>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) { >>>>>> + /* Found the amdgpu handle from the pci root after >>>>>> walking through the pci bus */ >>>>> If you insist on having this comment, make it a function comment >>>>> instead >>>>> (with appropriate modifications into the content of it) but I >>>>> personally >>>>> don't find it that useful so it could be just dropped as well, IMO. >>>>> >>>>>> + dev->gfx_data.gpu_dev = pdev; >>>>>> + return 1; /* Stop walking */ >>>>>> + } >>>>>> + >>>>>> + return 0; /* Continue walking */ >>>>>> +} >>>>>> + >>>>>> static int amd_pmf_amdtee_ta_match(struct >>>>>> tee_ioctl_version_data *ver, const void *data) >>>>>> { >>>>>> return ver->impl_id == TEE_IMPL_ID_AMDTEE; >>>>>> @@ -446,6 +460,15 @@ int amd_pmf_init_smart_pc(struct >>>>>> amd_pmf_dev *dev) >>>>>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd); >>>>>> amd_pmf_set_dram_addr(dev); >>>>>> amd_pmf_get_bios_buffer(dev); >>>>>> + >>>>>> + /* Get amdgpu handle */ >>>>> Useless comment. >>>>> >>>>>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev); >>>>>> + if (!dev->gfx_data.gpu_dev) >>>>>> + dev_err(dev->dev, "GPU handle not found!\n"); >>>>>> + >>>>>> + if (!amd_pmf_gpu_init(&dev->gfx_data)) >>>>>> + dev->gfx_data.gpu_dev_en = true; >>>>>> + >>>>>> dev->prev_data = kzalloc(sizeof(*dev->prev_data), >>>>>> GFP_KERNEL); >>>>>> if (!dev->prev_data) >>>>>> return -ENOMEM; >>>>>> @@ -461,5 +484,8 @@ void amd_pmf_deinit_smart_pc(struct >>>>>> amd_pmf_dev *dev) >>>>>> kfree(dev->prev_data); >>>>>> kfree(dev->policy_buf); >>>>>> cancel_delayed_work_sync(&dev->pb_work); >>>>>> + if (dev->gfx_data.gpu_dev_en) >>>>>> + amd_pmf_gpu_deinit(&dev->gfx_data); >>>>>> + pci_dev_put(dev->gfx_data.gpu_dev); >>>>>> amd_pmf_tee_deinit(dev); >>>>>> } >>>>>> diff --git a/include/linux/amd-pmf-io.h >>>>>> b/include/linux/amd-pmf-io.h >>>>>> new file mode 100644 >>>>>> index 000000000000..5f79e66a41b3 >>>>>> --- /dev/null >>>>>> +++ b/include/linux/amd-pmf-io.h >>>>>> @@ -0,0 +1,35 @@ >>>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>>> +/* >>>>>> + * AMD Platform Management Framework Interface >>>>>> + * >>>>>> + * Copyright (c) 2023, Advanced Micro Devices, Inc. >>>>>> + * All Rights Reserved. >>>>>> + * >>>>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>>> + */ >>>>>> + >>>>>> +#ifndef AMD_PMF_IO_H >>>>>> +#define AMD_PMF_IO_H >>>>>> + >>>>>> +#include <acpi/video.h> >>>>>> +#include <drm/drm_connector.h> >>>>>> +#include <linux/backlight.h> >>>>>> +#include <linux/thermal.h> >>>>>> + >>>>>> +#define MAX_SUPPORTED 4 >>>>>> + >>>>>> +/* amdgpu */ >>>>> Document the structure properly with kerneldoc instead of an >>>>> unhelpful >>>>> comment like above :-). Please also check if you add any other >>>>> structs >>>>> into kernel-wide headers that you didn't document yet. Or fields >>>>> into >>>>> existing structs. >>>>> >>>>>> +struct amd_gpu_pmf_data { >>>>>> + struct pci_dev *gpu_dev; >>>>>> + struct backlight_device *raw_bd; >>>>>> + struct thermal_cooling_device *cooling_dev; >>>>>> + enum drm_connector_status con_status[MAX_SUPPORTED]; >>>>>> + int display_count; >>>>>> + int connector_type[MAX_SUPPORTED]; >>>>>> + bool gpu_dev_en; >>>>>> +}; >>>>>> + >>>>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf); >>>>>> +int amd_pmf_gpu_init(struct amd_gpu_pmf_data *pmf); >>>>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf); >>>>>> +#endif >>>>>> >>> >> >
Am 18.10.23 um 19:05 schrieb Shyam Sundar S K: > > On 10/18/2023 9:37 PM, Christian König wrote: >> Am 18.10.23 um 17:47 schrieb Mario Limonciello: >>> On 10/18/2023 08:40, Christian König wrote: >>>> >>>> Am 18.10.23 um 11:28 schrieb Shyam Sundar S K: >>>>> On 10/18/2023 2:50 PM, Ilpo Järvinen wrote: >>>>>> On Wed, 18 Oct 2023, Shyam Sundar S K wrote: >>>>>> >>>>>>> In order to provide GPU inputs to TA for the Smart PC solution >>>>>>> to work, we >>>>>>> need to have interface between the PMF driver and the AMDGPU >>>>>>> driver. >>>>>>> >>>>>>> Add the initial code path for get interface from 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/Makefile | 2 + >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 138 >>>>>>> ++++++++++++++++++++++++ >>>>>>> drivers/platform/x86/amd/pmf/Kconfig | 1 + >>>>>>> drivers/platform/x86/amd/pmf/core.c | 1 + >>>>>>> drivers/platform/x86/amd/pmf/pmf.h | 3 + >>>>>>> drivers/platform/x86/amd/pmf/spc.c | 13 +++ >>>>>>> drivers/platform/x86/amd/pmf/tee-if.c | 26 +++++ >>>>>>> include/linux/amd-pmf-io.h | 35 ++++++ >>>>>>> 9 files changed, 220 insertions(+) >>>>>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>>> create mode 100644 include/linux/amd-pmf-io.h >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>>> index 384b798a9bad..7fafccefbd7a 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >>>>>>> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o >>>>>>> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o >>>>>>> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o >>>>>>> + >>>>>>> # add asic specific block >>>>>>> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \ >>>>>>> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>>> index a79d53bdbe13..26ffa1b4fe57 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>>> @@ -50,6 +50,7 @@ >>>>>>> #include <linux/hashtable.h> >>>>>>> #include <linux/dma-fence.h> >>>>>>> #include <linux/pci.h> >>>>>>> +#include <linux/amd-pmf-io.h> >>>>>>> #include <drm/ttm/ttm_bo.h> >>>>>>> #include <drm/ttm/ttm_placement.h> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>>> new file mode 100644 >>>>>>> index 000000000000..ac981848df50 >>>>>>> --- /dev/null >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c >>>>>>> @@ -0,0 +1,138 @@ >>>>>>> +/* >>>>>>> + * Copyright 2023 Advanced Micro Devices, Inc. >>>>>>> + * >>>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>>> obtaining a >>>>>>> + * copy of this software and associated documentation files >>>>>>> (the "Software"), >>>>>>> + * to deal in the Software without restriction, including >>>>>>> without limitation >>>>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>>>> sublicense, >>>>>>> + * and/or sell copies of the Software, and to permit persons to >>>>>>> whom the >>>>>>> + * Software is furnished to do so, subject to the following >>>>>>> conditions: >>>>>>> + * >>>>>>> + * The above copyright notice and this permission notice shall >>>>>>> be included in >>>>>>> + * all copies or substantial portions of the Software. >>>>>>> + * >>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY >>>>>>> KIND, EXPRESS OR >>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>>> MERCHANTABILITY, >>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>>>>> EVENT SHALL >>>>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY >>>>>>> CLAIM, DAMAGES OR >>>>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR >>>>>>> OTHERWISE, >>>>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR >>>>>>> THE USE OR >>>>>>> + * OTHER DEALINGS IN THE SOFTWARE. >>>>>> This is MIT, right? Add the required SPDX-License-Identifier line >>>>>> for it >>>>>> at the top of the file, thank you. >>>>>> >>>>> all the files in drivers/gpu/drm/amd/amdgpu/* carry the same license >>>>> text. So, have retained it to maintain uniformity. >>>> Please add the SPDX License Identifier for any file you add. > OK. I thought to keep it same like the other files. I will change this > to SPDX in the next revision. > >>>> Apart from that the whole approach of attaching this directly to >>>> amdgpu looks extremely questionable to me. >>>> >>> What's the long term outlook for things that are needed directly >>> from amdgpu? Is there going to be more besides the backlight and >>> the display count/type? >> Yeah, that goes into the same direction as my concern. > PMF is an APU only feature and will need inputs from multiple > subsystems (in this case its GPU) to send changing system information > to PMF TA (Trusted Application, running on ASP/PSP) as input. > > Its not only about the display count/type and backlight, there are > many others things in pipe like the GPU engine running time, > utilization percentage etc, that guide the TA to make better decisions. > > This series is only targeted to build the initial plubming with the > subsystems inside the kernel and later keep adding changes to get more > information from other subsystems. Yeah, and that's what I strongly disagree on. Don't come up with such an approach without consulting with upstream maintainers first. What you propose here is a system wide framework for improving power management, that's fine. The problem is that we already have something very similar in the thermal framework. See for example the Intel solution here https://docs.kernel.org/driver-api/thermal/intel_dptf.html From the general design the job of the amdgpu driver is to drive the display hardware. So this should in general be completely decoupled from this driver. As Mario suggested as well you can iterate over the connected displays independently. Same thing for the backlight. When it comes to hardware state like GPU engine utilization then we don't have that inside amdgpu either. Regards, Christian. > > that is the reason you see that, this patch proposes amd-pmf-io.h as > the interface to talk to other subsystems. For the initial path, I > have opted to get information from SFH and GPU drivers. But this is > meant to grow in future. > >>> At least for the display count I suppose one way that it could be >>> "decoupled" from amdgpu is to use drm_for_each_connector_iter to >>> iterate all the connectors and then count the connected ones. >> And what if the number of connected displays change? How is amdgpu >> supposed to signal events like that? > PMF driver polls for the information based on a configurable sampling > frequency. > > you can look at amd_pmf_get_gpu_info(), that gets called from > amd_pmf_populate_ta_inputs() in spc.c in this proposed series. > > Thanks, > Shyam > >> This whole solution doesn't looks well thought through. >> >> Regards, >> Christian. >> >>>> Regards, >>>> Christian. >>>> >>>>>>> + * >>>>>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>>>> + * >>>>>>> + */ >>>>>> Remove the extra empty line at the end of the comment. >>>>>> >>>>> I just took the standard template for all the gpu files. Is that >>>>> OK to >>>>> retain the blank line? >>>>> >>>>> If not, I can remove it in the next version. >>>>> >>>>> Rest all remarks I will address. >>>>> >>>>> Thanks, >>>>> Shyam >>>>> >>>>>>> + >>>>>>> +#include <linux/backlight.h> >>>>>>> +#include "amdgpu.h" >>>>>>> + >>>>>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf) >>>>>>> +{ >>>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev); >>>>>>> + struct drm_mode_config *mode_config = &drm_dev->mode_config; >>>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev); >>>>>>> + struct drm_connector_list_iter iter; >>>>>>> + struct drm_connector *connector; >>>>>>> + int i = 0; >>>>>>> + >>>>>>> + /* Reset the count to zero */ >>>>>>> + pmf->display_count = 0; >>>>>>> + if (!(adev->flags & AMD_IS_APU)) { >>>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>>>>>> + return -ENODEV; >>>>>>> + } >>>>>>> + >>>>>>> + mutex_lock(&mode_config->mutex); >>>>>>> + drm_connector_list_iter_begin(drm_dev, &iter); >>>>>>> + drm_for_each_connector_iter(connector, &iter) { >>>>>>> + if (connector->status == connector_status_connected) >>>>>>> + pmf->display_count++; >>>>>>> + if (connector->status != pmf->con_status[i]) >>>>>>> + pmf->con_status[i] = connector->status; >>>>>>> + if (connector->connector_type != pmf->connector_type[i]) >>>>>>> + pmf->connector_type[i] = connector->connector_type; >>>>>>> + >>>>>>> + i++; >>>>>>> + if (i >= MAX_SUPPORTED) >>>>>>> + break; >>>>>>> + } >>>>>>> + drm_connector_list_iter_end(&iter); >>>>>>> + mutex_unlock(&mode_config->mutex); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data); >>>>>>> + >>>>>>> +static int amd_pmf_gpu_get_cur_state(struct >>>>>>> thermal_cooling_device *cooling_dev, >>>>>>> + unsigned long *state) >>>>>>> +{ >>>>>>> + struct backlight_device *bd; >>>>>>> + >>>>>>> + if (!acpi_video_backlight_use_native()) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>>> + if (!bd) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + *state = backlight_get_brightness(bd); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +static int amd_pmf_gpu_get_max_state(struct >>>>>>> thermal_cooling_device *cooling_dev, >>>>>>> + unsigned long *state) >>>>>>> +{ >>>>>>> + struct backlight_device *bd; >>>>>>> + >>>>>>> + if (!acpi_video_backlight_use_native()) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>>> + if (!bd) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + if (backlight_is_blank(bd)) >>>>>>> + *state = 0; >>>>>>> + else >>>>>>> + *state = bd->props.max_brightness; >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = { >>>>>>> + .get_max_state = amd_pmf_gpu_get_max_state, >>>>>>> + .get_cur_state = amd_pmf_gpu_get_cur_state, >>>>>>> +}; >>>>>>> + >>>>>>> +int amd_pmf_gpu_init(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); >>>>>>> + >>>>>>> + if (!(adev->flags & AMD_IS_APU)) { >>>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n"); >>>>>>> + return -ENODEV; >>>>>>> + } >>>>>>> + >>>>>>> + if (!acpi_video_backlight_use_native()) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + pmf->raw_bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>>> + if (!pmf->raw_bd) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + pmf->cooling_dev = >>>>>>> thermal_cooling_device_register("pmf_gpu_bd", >>>>>>> + pmf, &bd_cooling_ops); >>>>>>> + if (IS_ERR(pmf->cooling_dev)) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_init); >>>>>>> + >>>>>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf) >>>>>>> +{ >>>>>>> + thermal_cooling_device_unregister(pmf->cooling_dev); >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_deinit); >>>>>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig >>>>>>> b/drivers/platform/x86/amd/pmf/Kconfig >>>>>>> index f246252bddd8..7f430de7af44 100644 >>>>>>> --- a/drivers/platform/x86/amd/pmf/Kconfig >>>>>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig >>>>>>> @@ -10,6 +10,7 @@ config AMD_PMF >>>>>>> depends on AMD_NB >>>>>>> select ACPI_PLATFORM_PROFILE >>>>>>> depends on TEE && AMDTEE >>>>>>> + depends on DRM_AMDGPU >>>>>>> help >>>>>>> This driver provides support for the AMD Platform >>>>>>> Management Framework. >>>>>>> The goal is to enhance end user experience by making AMD >>>>>>> PCs smarter, >>>>>>> diff --git a/drivers/platform/x86/amd/pmf/core.c >>>>>>> b/drivers/platform/x86/amd/pmf/core.c >>>>>>> index 4b8156033fa6..c59ba527ff49 100644 >>>>>>> --- a/drivers/platform/x86/amd/pmf/core.c >>>>>>> +++ b/drivers/platform/x86/amd/pmf/core.c >>>>>>> @@ -411,6 +411,7 @@ static int amd_pmf_probe(struct >>>>>>> platform_device *pdev) >>>>>>> } >>>>>>> dev->cpu_id = rdev->device; >>>>>>> + dev->root = rdev; >>>>>>> err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val); >>>>>>> if (err) { >>>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h >>>>>>> b/drivers/platform/x86/amd/pmf/pmf.h >>>>>>> index 8712299ad52b..615cd3a31986 100644 >>>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h >>>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h >>>>>>> @@ -13,6 +13,7 @@ >>>>>>> #include <linux/acpi.h> >>>>>>> #include <linux/platform_profile.h> >>>>>>> +#include <linux/amd-pmf-io.h> >>>>>>> #define POLICY_BUF_MAX_SZ 0x4b000 >>>>>>> #define POLICY_SIGN_COOKIE 0x31535024 >>>>>>> @@ -228,9 +229,11 @@ struct amd_pmf_dev { >>>>>>> void *shbuf; >>>>>>> struct delayed_work pb_work; >>>>>>> struct pmf_action_table *prev_data; >>>>>>> + struct amd_gpu_pmf_data gfx_data; >>>>>>> u64 policy_addr; >>>>>>> void *policy_base; >>>>>>> bool smart_pc_enabled; >>>>>>> + struct pci_dev *root; >>>>>>> }; >>>>>>> struct apmf_sps_prop_granular { >>>>>>> diff --git a/drivers/platform/x86/amd/pmf/spc.c >>>>>>> b/drivers/platform/x86/amd/pmf/spc.c >>>>>>> index 512e0c66efdc..cf4962ef97c2 100644 >>>>>>> --- a/drivers/platform/x86/amd/pmf/spc.c >>>>>>> +++ b/drivers/platform/x86/amd/pmf/spc.c >>>>>>> @@ -44,6 +44,10 @@ void amd_pmf_dump_ta_inputs(struct >>>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_table * >>>>>>> dev_dbg(dev->dev, "Max C0 Residency : %u\n", >>>>>>> in->ev_info.max_c0residency); >>>>>>> dev_dbg(dev->dev, "GFX Busy : %u\n", in->ev_info.gfx_busy); >>>>>>> dev_dbg(dev->dev, "Connected Display Count : %u\n", >>>>>>> in->ev_info.monitor_count); >>>>>>> + dev_dbg(dev->dev, "Primary Display Type : %s\n", >>>>>>> + drm_get_connector_type_name(in->ev_info.display_type)); >>>>>>> + dev_dbg(dev->dev, "Primary Display State : %s\n", >>>>>>> in->ev_info.display_state ? >>>>>>> + "Connected" : "disconnected/unknown"); >>>>>>> dev_dbg(dev->dev, "LID State : %s\n", >>>>>>> in->ev_info.lid_state ? "Close" : "Open"); >>>>>>> dev_dbg(dev->dev, "==== TA inputs END ====\n"); >>>>>>> } >>>>>>> @@ -146,6 +150,14 @@ static int amd_pmf_get_slider_info(struct >>>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_ >>>>>>> return 0; >>>>>>> } >>>>>>> +static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev, >>>>>>> struct ta_pmf_enact_table *in) >>>>>>> +{ >>>>>>> + amd_pmf_get_gfx_data(&dev->gfx_data); >>>>>>> + in->ev_info.monitor_count = dev->gfx_data.display_count; >>>>>>> + in->ev_info.display_type = dev->gfx_data.connector_type[0]; >>>>>>> + in->ev_info.display_state = dev->gfx_data.con_status[0]; >>>>>>> +} >>>>>>> + >>>>>>> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, >>>>>>> struct ta_pmf_enact_table *in) >>>>>>> { >>>>>>> /* TA side lid open is 1 and close is 0, hence the ! here */ >>>>>>> @@ -154,4 +166,5 @@ void amd_pmf_populate_ta_inputs(struct >>>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_tab >>>>>>> amd_pmf_get_smu_info(dev, in); >>>>>>> amd_pmf_get_battery_info(dev, in); >>>>>>> amd_pmf_get_slider_info(dev, in); >>>>>>> + amd_pmf_get_gpu_info(dev, in); >>>>>>> } >>>>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c >>>>>>> b/drivers/platform/x86/amd/pmf/tee-if.c >>>>>>> index 2f5fb8623c20..956e66b78605 100644 >>>>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c >>>>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c >>>>>>> @@ -9,6 +9,7 @@ >>>>>>> */ >>>>>>> #include <linux/debugfs.h> >>>>>>> +#include <linux/pci.h> >>>>>>> #include <linux/tee_drv.h> >>>>>>> #include <linux/uuid.h> >>>>>>> #include "pmf.h" >>>>>>> @@ -357,6 +358,19 @@ static int amd_pmf_get_bios_buffer(struct >>>>>>> amd_pmf_dev *dev) >>>>>>> return amd_pmf_start_policy_engine(dev); >>>>>>> } >>>>>>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void >>>>>>> *data) >>>>>>> +{ >>>>>>> + struct amd_pmf_dev *dev = data; >>>>>>> + >>>>>>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) { >>>>>>> + /* Found the amdgpu handle from the pci root after >>>>>>> walking through the pci bus */ >>>>>> If you insist on having this comment, make it a function comment >>>>>> instead >>>>>> (with appropriate modifications into the content of it) but I >>>>>> personally >>>>>> don't find it that useful so it could be just dropped as well, IMO. >>>>>> >>>>>>> + dev->gfx_data.gpu_dev = pdev; >>>>>>> + return 1; /* Stop walking */ >>>>>>> + } >>>>>>> + >>>>>>> + return 0; /* Continue walking */ >>>>>>> +} >>>>>>> + >>>>>>> static int amd_pmf_amdtee_ta_match(struct >>>>>>> tee_ioctl_version_data *ver, const void *data) >>>>>>> { >>>>>>> return ver->impl_id == TEE_IMPL_ID_AMDTEE; >>>>>>> @@ -446,6 +460,15 @@ int amd_pmf_init_smart_pc(struct >>>>>>> amd_pmf_dev *dev) >>>>>>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd); >>>>>>> amd_pmf_set_dram_addr(dev); >>>>>>> amd_pmf_get_bios_buffer(dev); >>>>>>> + >>>>>>> + /* Get amdgpu handle */ >>>>>> Useless comment. >>>>>> >>>>>>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev); >>>>>>> + if (!dev->gfx_data.gpu_dev) >>>>>>> + dev_err(dev->dev, "GPU handle not found!\n"); >>>>>>> + >>>>>>> + if (!amd_pmf_gpu_init(&dev->gfx_data)) >>>>>>> + dev->gfx_data.gpu_dev_en = true; >>>>>>> + >>>>>>> dev->prev_data = kzalloc(sizeof(*dev->prev_data), >>>>>>> GFP_KERNEL); >>>>>>> if (!dev->prev_data) >>>>>>> return -ENOMEM; >>>>>>> @@ -461,5 +484,8 @@ void amd_pmf_deinit_smart_pc(struct >>>>>>> amd_pmf_dev *dev) >>>>>>> kfree(dev->prev_data); >>>>>>> kfree(dev->policy_buf); >>>>>>> cancel_delayed_work_sync(&dev->pb_work); >>>>>>> + if (dev->gfx_data.gpu_dev_en) >>>>>>> + amd_pmf_gpu_deinit(&dev->gfx_data); >>>>>>> + pci_dev_put(dev->gfx_data.gpu_dev); >>>>>>> amd_pmf_tee_deinit(dev); >>>>>>> } >>>>>>> diff --git a/include/linux/amd-pmf-io.h >>>>>>> b/include/linux/amd-pmf-io.h >>>>>>> new file mode 100644 >>>>>>> index 000000000000..5f79e66a41b3 >>>>>>> --- /dev/null >>>>>>> +++ b/include/linux/amd-pmf-io.h >>>>>>> @@ -0,0 +1,35 @@ >>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>>>> +/* >>>>>>> + * AMD Platform Management Framework Interface >>>>>>> + * >>>>>>> + * Copyright (c) 2023, Advanced Micro Devices, Inc. >>>>>>> + * All Rights Reserved. >>>>>>> + * >>>>>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>>>>> + */ >>>>>>> + >>>>>>> +#ifndef AMD_PMF_IO_H >>>>>>> +#define AMD_PMF_IO_H >>>>>>> + >>>>>>> +#include <acpi/video.h> >>>>>>> +#include <drm/drm_connector.h> >>>>>>> +#include <linux/backlight.h> >>>>>>> +#include <linux/thermal.h> >>>>>>> + >>>>>>> +#define MAX_SUPPORTED 4 >>>>>>> + >>>>>>> +/* amdgpu */ >>>>>> Document the structure properly with kerneldoc instead of an >>>>>> unhelpful >>>>>> comment like above :-). Please also check if you add any other >>>>>> structs >>>>>> into kernel-wide headers that you didn't document yet. Or fields >>>>>> into >>>>>> existing structs. >>>>>> >>>>>>> +struct amd_gpu_pmf_data { >>>>>>> + struct pci_dev *gpu_dev; >>>>>>> + struct backlight_device *raw_bd; >>>>>>> + struct thermal_cooling_device *cooling_dev; >>>>>>> + enum drm_connector_status con_status[MAX_SUPPORTED]; >>>>>>> + int display_count; >>>>>>> + int connector_type[MAX_SUPPORTED]; >>>>>>> + bool gpu_dev_en; >>>>>>> +}; >>>>>>> + >>>>>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf); >>>>>>> +int amd_pmf_gpu_init(struct amd_gpu_pmf_data *pmf); >>>>>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf); >>>>>>> +#endif >>>>>>>
Hi Hans, Apologies for the long delay. On 10/19/2023 12:38 AM, Hans de Goede wrote: > Hi, > > I was not following this at first, so my apologies for > jumping in in the middle of the thread: > > > <snip> > >>>>>> +static int amd_pmf_gpu_get_cur_state(struct thermal_cooling_device *cooling_dev, >>>>>> + unsigned long *state) >>>>>> +{ >>>>>> + struct backlight_device *bd; >>>>>> + >>>>>> + if (!acpi_video_backlight_use_native()) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>> + if (!bd) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + *state = backlight_get_brightness(bd); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int amd_pmf_gpu_get_max_state(struct thermal_cooling_device *cooling_dev, >>>>>> + unsigned long *state) >>>>>> +{ >>>>>> + struct backlight_device *bd; >>>>>> + >>>>>> + if (!acpi_video_backlight_use_native()) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW); >>>>>> + if (!bd) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + if (backlight_is_blank(bd)) >>>>>> + *state = 0; >>>>>> + else >>>>>> + *state = bd->props.max_brightness; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = { >>>>>> + .get_max_state = amd_pmf_gpu_get_max_state, >>>>>> + .get_cur_state = amd_pmf_gpu_get_cur_state, >>>>>> +}; > > So first of all, good to see that this is using the > thermal_cooling_device APIs now, that is great thank you. > > But the whole idea behind using the thermal_cooling_device APIs > is that amdgpu exports the cooling_device itself, rather then have > the AMD PMF code export it. Now the AMD PMF code is still poking > at the backlight_device itself, while the idea was to delegate > this to the GPU driver. > > Actually seeing all the acpi_video_backlight_use_native() > checks here, I wonder why only have this work with native backlight > control. One step better would be to add thermal_cooling_device > support to the backlight core in: > drivers/video/backlight/backlight.c > > Then it will work with any backlight control provider! > > > > Last but not least this code MUST not call > acpi_video_backlight_use_native() > > No code other then native GPU drivers must ever call > acpi_video_backlight_use_native(). This special function > not only checks if the native backlight control is the > one which the detection code in drivers/acpi/video_detect.c > has selected, it also signals to video_detect.c that > native GPU backlight control is available. > > So by calling this in the AMD PMF code you are now > telling video_detect.c that native GPU backlight control > is available on all systems where AMD PMF runs. > > As I already said I really believe the whole cooling > device should be registered somewhere else. But if you > do end up sticking with this then you MUST replace > the acpi_video_backlight_use_native() calls with: > > if (acpi_video_get_backlight_type() == acpi_backlight_native) {...} Thank you very much for your detailed feedback. This helped. I have moved the code from amdgpu to PMF driver which has changes for DRM. This also has changed w.r.t thermal device change what you suggested. I have used the checks where ever appropriate: acpi_video_get_backlight_type() == acpi_backlight_native Kindly take a look at v5 submission. Thanks, Shyam > > Regards, > > Hans > > >