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 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.

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.


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?

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 DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux