Re: [PATCH v4 13/17] platform/x86/amd/pmf: Add PMF-AMDGPU get interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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,
Shyam

This 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





[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux