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@xxxxxxx> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> --- drivers/gpu/drm/amd/amdgpu/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, ShyamThis whole solution doesn't looks well thought through. Regards, Christian.Regards, Christian.+ * + * Author: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> + * + */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@xxxxxxx> + */ + +#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
