Hi Hans,
On 9/26/2023 4:05 PM, Hans de Goede wrote:
> Hi,
>
> On 9/22/23 19:50, Shyam Sundar S K wrote:
>> For the Smart PC Solution to fully work, it has to enact to the actions
>> coming from TA. Add the initial code path for set interface to 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/amdgpu_pmf.c | 21 +++++++++++++++++++++
>> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
>> drivers/platform/x86/amd/pmf/tee-if.c | 19 +++++++++++++++++--
>> include/linux/amd-pmf-io.h | 1 +
>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> index 232d11833ddc..5c567bff0548 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>> +
>> +int amd_pmf_set_gfx_data(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);
>> + struct backlight_device *bd;
>> +
>> + if (!(adev->flags & AMD_IS_APU)) {
>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>> + return -ENODEV;
>> + }
>> +
>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>> + if (!bd)
>> + return -ENODEV;
>
> This assumes that the backlight is always controller by the amdgpu's
> native backlight driver, but it might e.g. also be handled by
> eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
> nvidia dgpu).
PMF is meant for AMD APUs(atleast for now) and the _HID will only be
made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
should be safe, right?
>
> For now what should be done here is to call acpi_video_get_backlight_type()
> and then translate the return value from this into a backlight-type:
>
> acpi_backlight_video -> BACKLIGHT_FIRMWARE
> acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
> acpi_backlight_native, -> BACKLIGHT_RAW
> acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
> acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>
I can add this change in the v2, do you insist on this?
Thanks,
Shyam
> Also I'm worried about probe order here, this code currently assumes
> that the GPU or other backlight driver has loaded before this runs,
> which is not necessarily the case.
>
> I think that if the backlight_device_get_by_type() fails this
> should be retried say every 10 seconds from some delayed workqueue
> for at least a couple of minutes after boot.
>
> Regards,
>
> Hans
>
>
>
>
>> +
>> + backlight_device_set_brightness(bd, pmf->brightness);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 9032df4ba48a..ce89cc0daa5a 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -73,6 +73,7 @@
>> #define PMF_POLICY_STT_SKINTEMP_APU 7
>> #define PMF_POLICY_STT_SKINTEMP_HS2 8
>> #define PMF_POLICY_SYSTEM_STATE 9
>> +#define PMF_POLICY_DISPLAY_BRIGHTNESS 12
>> #define PMF_POLICY_P3T 38
>>
>> /* TA macros */
>> @@ -480,6 +481,7 @@ enum ta_pmf_error_type {
>> };
>>
>> struct pmf_action_table {
>> + unsigned long display_brightness;
>> enum system_state system_state;
>> unsigned long spl; /* in mW */
>> unsigned long sppt; /* in mW */
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 1608996654e8..eefffff83a4c 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
>> return 0;
>> }
>>
>> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>> {
>> u32 val, event = 0;
>> - int idx;
>> + int idx, ret;
>>
>> for (idx = 0; idx < out->actions_count; idx++) {
>> val = out->actions_list[idx].value;
>> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>> dev->prev_data->system_state = 0;
>> }
>> break;
>> +
>> + case PMF_POLICY_DISPLAY_BRIGHTNESS:
>> + ret = amd_pmf_get_gfx_data(&dev->gfx_data);
>> + if (ret)
>> + return ret;
>> +
>> + dev->prev_data->display_brightness = dev->gfx_data.brightness;
>> + if (dev->prev_data->display_brightness != val) {
>> + dev->gfx_data.brightness = val;
>> + amd_pmf_set_gfx_data(&dev->gfx_data);
>> + dev_dbg(dev->dev, "update DISPLAY_BRIGHTNESS : %d\n", val);
>> + }
>> + break;
>> }
>> }
>> +
>> + return 0;
>> }
>>
>> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
>> index a2d4af231362..ecae387ddaa6 100644
>> --- a/include/linux/amd-pmf-io.h
>> +++ b/include/linux/amd-pmf-io.h
>> @@ -25,4 +25,5 @@ struct amd_gpu_pmf_data {
>> };
>>
>> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
>> #endif
>
