Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface

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

Hi Hans,

On 9/27/2023 6:34 PM, Hans de Goede wrote:
> HI,
> 
> On 9/26/23 15:17, Christian König wrote:
>> Am 26.09.23 um 14:56 schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 9/26/23 13:24, Shyam Sundar S K wrote:
>>>> 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@amd.com>
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@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;
>>>>> 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?
>>> Users can pass say acpi_backlight=video and use the acpi_video
>>> driver for backlight control instead of the native GPU backlight
>>> control.
>>>
>>>>> 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?
>>> Insist is a strong word, but I think that it is a good idea to have
>>> this. Evenutally it looks like this code will need to either integrate with
>>> the drm drivers lot more; or the drm core needs to export some special
>>> hooks for this which the PMF code can then call.
>>>
>>> Actually thinking more about this, I think that the right thing to do
>>> here is make some code register brightness control as a cooling device
>>> (which I think is already done in some cases) and then have the PMF
>>> code use the cooling-device APIs for this.
>>>
>>> IMHO that would be a much cleaner solution then this hack.
>>
>> Yeah, fully agree with Hans. This looks like a rather extreme hack to me.
> 
> Shyam, the cooling device interface is defined in:
> 
> include/linux/thermal.h
> 
> And then look for cooling_device .
> 
> An example of code registering a cooling_device for backlight control is:
> 
> drivers/acpi/acpi_video.c
> 
> and then specifically the code starting around line 257 with:
> 
> video_get_max_state()
> 
> until
> 
> static const struct thermal_cooling_device_ops video_cooling_ops = {
> ...
> 
> And the code around line 1750 for actually registering the cooling-dev.
> 
> To use the cooling_device interface witt amdgpu's native backlight control
> you will need to make the amdgpu backlight control register a cooling-device
> for this in a similar manner.
> 

Thank you for pointing to the references. I will address the remarks
from Ilpo and respin a new version soon.

Thanks,
Shyam

> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>> Apart from that what exactly is this thing supposed to do? Prevent overheating by reducing the brightness?
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>> 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
>>
>