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

["Christian König" <christian.koenig@amd.com>]

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.

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