Re: [PATCH] platform/x86/amd/pmf: Enable Custom BIOS Inputs for PMF-TA

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 12/6/2024 00:45, Mario Limonciello wrote:
> On 12/5/2024 04:19, Shyam Sundar S K wrote:
>> Introduce support for passing custom BIOS inputs to the PMF-TA to
>> assess
>> BIOS input policy conditions. The PMF driver will adjust system
>> settings
>> based on these BIOS input conditions and their corresponding output
>> actions.
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> I do have one suggestion for a follow up to consider.
> 
>> ---
>>   drivers/platform/x86/amd/pmf/acpi.c | 24 ++++++++++++++++++++++++
>>   drivers/platform/x86/amd/pmf/pmf.h  | 15 ++++++++++++++-
>>   drivers/platform/x86/amd/pmf/spc.c  | 24 ++++++++++++++++++++++++
>>   3 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/
>> x86/amd/pmf/acpi.c
>> index 1b9c7acf0ddf..21ccd677ffa4 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -321,6 +321,18 @@ int apmf_get_sbios_requests(struct amd_pmf_dev
>> *pdev, struct apmf_sbios_req *req
>>                                        req, sizeof(*req));
>>   }
>>   +static void apmf_event_handler_v2(acpi_handle handle, u32 event,
>> void *data)
>> +{
>> +    struct amd_pmf_dev *pmf_dev = data;
>> +    int ret;
>> +
>> +    guard(mutex)(&pmf_dev->cb_mutex);
>> +
>> +    ret = apmf_get_sbios_requests_v2(pmf_dev, &pmf_dev->req);
>> +    if (ret)
>> +        dev_err(pmf_dev->dev, "Failed to get v2 SBIOS requests:
>> %d\n", ret);
>> +}
>> +
>>   static void apmf_event_handler(acpi_handle handle, u32 event, void
>> *data)
>>   {
>>       struct amd_pmf_dev *pmf_dev = data;
>> @@ -430,6 +442,15 @@ int apmf_install_handler(struct amd_pmf_dev
>> *pmf_dev)
>>           apmf_event_handler(ahandle, 0, pmf_dev);
>>       }
>>   +    if (pmf_dev->smart_pc_enabled && pmf_dev->pmf_if_version ==
>> PMF_IF_V2) {
>> +        status = acpi_install_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>> +                             apmf_event_handler_v2, pmf_dev);
>> +        if (ACPI_FAILURE(status)) {
>> +            dev_err(pmf_dev->dev, "failed to install notify handler
>> for custom BIOS inputs\n");
>> +            return -ENODEV;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>   @@ -480,6 +501,9 @@ void apmf_acpi_deinit(struct amd_pmf_dev
>> *pmf_dev)
>>       if (is_apmf_func_supported(pmf_dev, APMF_FUNC_AUTO_MODE) &&
>>           is_apmf_func_supported(pmf_dev, APMF_FUNC_SBIOS_REQUESTS))
>>           acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>> apmf_event_handler);
>> +
>> +    if (pmf_dev->smart_pc_enabled && pmf_dev->pmf_if_version ==
>> PMF_IF_V2)
>> +        acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>> apmf_event_handler_v2);
>>   }
>>     int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/
>> x86/amd/pmf/pmf.h
>> index a79808fda1d8..255c5ad2300e 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -370,6 +370,8 @@ struct amd_pmf_dev {
>>       struct input_dev *pmf_idev;
>>       size_t mtable_size;
>>       struct resource *res;
>> +    struct apmf_sbios_req_v2 req; /* To get custom bios pending
>> request */
>> +    struct mutex cb_mutex;
>>   };
>>     struct apmf_sps_prop_granular_v2 {
>> @@ -616,6 +618,16 @@ enum ta_slider {
>>       TA_MAX,
>>   };
>>   +enum apmf_smartpc_custom_bios_inputs {
>> +    APMF_SMARTPC_CUSTOM_BIOS_INPUT1,
>> +    APMF_SMARTPC_CUSTOM_BIOS_INPUT2,
>> +};
>> +
>> +enum apmf_preq_smartpc {
>> +    NOTIFY_CUSTOM_BIOS_INPUT1 = 5,
>> +    NOTIFY_CUSTOM_BIOS_INPUT2,
>> +};
> 
> Maybe as a follow up you should combine these all together in a common
> header:
> 
> APMF_AMT_NOTIFICATION
> APMF_CQL_NOTIFICATION
> 

Mario - Sorry, I missed this remark. Will address in the larger
reordering of the PMF series.

Thanks,
Shyam

>> +
>>   /* Command ids for TA communication */
>>   enum ta_pmf_command {
>>       TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
>> @@ -657,7 +669,8 @@ struct ta_pmf_condition_info {
>>       u32 power_slider;
>>       u32 lid_state;
>>       bool user_present;
>> -    u32 rsvd1[2];
>> +    u32 bios_input1;
>> +    u32 bios_input2;
>>       u32 monitor_count;
>>       u32 rsvd2[2];
>>       u32 bat_design;
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/
>> x86/amd/pmf/spc.c
>> index 06226eb0eab3..05bed8206365 100644
>> --- a/drivers/platform/x86/amd/pmf/spc.c
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -47,12 +47,35 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev
>> *dev, struct ta_pmf_enact_table *
>>       dev_dbg(dev->dev, "LID State: %s\n", in->ev_info.lid_state ?
>> "close" : "open");
>>       dev_dbg(dev->dev, "User Presence: %s\n", in-
>> >ev_info.user_present ? "Present" : "Away");
>>       dev_dbg(dev->dev, "Ambient Light: %d\n", in-
>> >ev_info.ambient_light);
>> +    dev_dbg(dev->dev, "Custom BIOS input1: %u\n", in-
>> >ev_info.bios_input1);
>> +    dev_dbg(dev->dev, "Custom BIOS input2: %u\n", in-
>> >ev_info.bios_input2);
>>       dev_dbg(dev->dev, "==== TA inputs END ====\n");
>>   }
>>   #else
>>   void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct
>> ta_pmf_enact_table *in) {}
>>   #endif
>>   +static void amd_pmf_get_custom_bios_inputs(struct amd_pmf_dev *pdev,
>> +                       struct ta_pmf_enact_table *in)
>> +{
>> +    if (!pdev->req.pending_req)
>> +        return;
>> +
>> +    switch (pdev->req.pending_req) {
>> +    case BIT(NOTIFY_CUSTOM_BIOS_INPUT1):
>> +        in->ev_info.bios_input1 = pdev-
>> >req.custom_policy[APMF_SMARTPC_CUSTOM_BIOS_INPUT1];
>> +        break;
>> +    case BIT(NOTIFY_CUSTOM_BIOS_INPUT2):
>> +        in->ev_info.bios_input2 = pdev-
>> >req.custom_policy[APMF_SMARTPC_CUSTOM_BIOS_INPUT2];
>> +        break;
>> +    default:
>> +        dev_dbg(pdev->dev, "Invalid preq for BIOS input: 0x%x\n",
>> pdev->req.pending_req);
>> +    }
>> +
>> +    /* Clear pending requests after handling */
>> +    memset(&pdev->req, 0, sizeof(pdev->req));
>> +}
>> +
>>   static void amd_pmf_get_c0_residency(u16 *core_res, size_t size,
>> struct ta_pmf_enact_table *in)
>>   {
>>       u16 max, avg = 0;
>> @@ -201,4 +224,5 @@ void amd_pmf_populate_ta_inputs(struct
>> amd_pmf_dev *dev, struct ta_pmf_enact_tab
>>       amd_pmf_get_battery_info(dev, in);
>>       amd_pmf_get_slider_info(dev, in);
>>       amd_pmf_get_sensor_info(dev, in);
>> +    amd_pmf_get_custom_bios_inputs(dev, in);
>>   }
>