On 9/25/2023 9:57 PM, Deucher, Alexander wrote:
> [Public]
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-boun...@lists.freedesktop.org> On Behalf Of
>> Shyam Sundar S K
>> Sent: Friday, September 22, 2023 1:51 PM
>> To: hdego...@redhat.com; markgr...@kernel.org; Natikar, Basavaraj
>> <basavaraj.nati...@amd.com>; ji...@kernel.org;
>> benjamin.tissoi...@redhat.com; Deucher, Alexander
>> <alexander.deuc...@amd.com>; Koenig, Christian
>> <christian.koe...@amd.com>; Pan, Xinhui <xinhui....@amd.com>;
>> airl...@gmail.com; dan...@ffwll.ch
>> Cc: S-k, Shyam-sundar <shyam-sundar....@amd.com>; amd-
>> g...@lists.freedesktop.org; platform-driver-...@vger.kernel.org; dri-
>> de...@lists.freedesktop.org; Patil Rajesh <patil.re...@amd.com>; linux-
>> in...@vger.kernel.org; Limonciello, Mario <mario.limoncie...@amd.com>
>> Subject: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set
>> interface
>>
>> 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.
>
> This seems to be limited to backlight at this point. What does setting or
> not setting the backlight level mean for the system when this framework is in
> place? What if a user manually changes the backlight level? Additional
> comments below.
The unit here is nits that varies from 0 to 255. User can manually
update the backlight but if there is an action from the TA to update
the backlight, PMF driver would send a request to GPU driver to update
the backlight to the updated value (in nits)
At this point, yes. PMF is using to PMF-GPU interface to set
backlight, but there are additional things to be added in future. This
patch builds the initial plumbing for that.
>
>>
>> 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/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;
>> +
>> + 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;
>
> Are we using standardized units for the brightness? On the GPU side, we
> align with the standard blacklight interface, but internally, the driver has
> to convert units depending on the type of backlight controller used on the
> platform. Presumably PMF does something similar?
Yes its the standard nits. There is no conversion needed.
>
> Alex
>
>> + 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
>> --
>> 2.25.1
>
