On 10/19/2023 2:31 PM, Christian König wrote:
> 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.limoncie...@amd.com>
>>>>>>>> Signed-off-by: Mario Limonciello <mario.limoncie...@amd.com>
>>>>>>>> Signed-off-by: Shyam Sundar S K <shyam-sundar....@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.
PMF is there since sometime and this is an additional feature, so you
should presume that its being consulted.
>
> 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.
Well. Same should be case even with PMF driver as well. It should
provide overall interfaces for OEMs and ecosystem partners.
So, PMF should not have the DRM related changes IMHO and should have
an interface defined to talk to AMDGPU.
>
> When it comes to hardware state like GPU engine utilization then we
> don't have that inside amdgpu either.
I spent sometime to get the stats for GPU utilization and other
information required to be passed to PMF TA, but the results were not
really favourable (maybe these could be issues in the PMF-TA as well).
So for now, I think it makes sense to have the DRM related code
changes within the PMF driver. But once we have other inputs from the
GPU/DRM subsystem ready, IMO GPU driver should provide these inputs
back to PMF driver.
Let me know if you have a different opinion on this.
Thanks,
Shyam
>
> 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....@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....@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
>>>>>>>>
>
