Re: [PATCH v4 13/17] platform/x86/amd/pmf: Add PMF-AMDGPU get interface

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

Hi Hans,

Apologies for the long delay.

On 10/19/2023 12:38 AM, Hans de Goede wrote:
> Hi,
> 
> I was not following this at first, so my apologies for
> jumping in in the middle of the thread:
> 
> 
> <snip>
> 
>>>>>> +static int amd_pmf_gpu_get_cur_state(struct thermal_cooling_device *cooling_dev,
>>>>>> +                     unsigned long *state)
>>>>>> +{
>>>>>> +    struct backlight_device *bd;
>>>>>> +
>>>>>> +    if (!acpi_video_backlight_use_native())
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>>> +    if (!bd)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    *state = backlight_get_brightness(bd);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int amd_pmf_gpu_get_max_state(struct thermal_cooling_device *cooling_dev,
>>>>>> +                     unsigned long *state)
>>>>>> +{
>>>>>> +    struct backlight_device *bd;
>>>>>> +
>>>>>> +    if (!acpi_video_backlight_use_native())
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>>> +    if (!bd)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    if (backlight_is_blank(bd))
>>>>>> +        *state = 0;
>>>>>> +    else
>>>>>> +        *state = bd->props.max_brightness;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = {
>>>>>> +    .get_max_state = amd_pmf_gpu_get_max_state,
>>>>>> +    .get_cur_state = amd_pmf_gpu_get_cur_state,
>>>>>> +};
> 
> So first of all, good to see that this is using the
> thermal_cooling_device APIs now, that is great thank you.
> 
> But the whole idea behind using the thermal_cooling_device APIs
> is that amdgpu exports the cooling_device itself, rather then have
> the AMD PMF code export it. Now the AMD PMF code is still poking
> at the backlight_device itself, while the idea was to delegate
> this to the GPU driver.
> 
> Actually seeing all the acpi_video_backlight_use_native()
> checks here, I wonder why only have this work with native backlight
> control. One step better would be to add thermal_cooling_device
> support to the backlight core in:
> drivers/video/backlight/backlight.c
> 
> Then it will work with any backlight control provider!
> 
> 
> 
> Last but not least this code MUST not call
> acpi_video_backlight_use_native()
> 
> No code other then native GPU drivers must ever call
> acpi_video_backlight_use_native(). This special function
> not only checks if the native backlight control is the
> one which the detection code in drivers/acpi/video_detect.c
> has selected, it also signals to video_detect.c that
> native GPU backlight control is available.
> 
> So by calling this in the AMD PMF code you are now
> telling video_detect.c that native GPU backlight control
> is available on all systems where AMD PMF runs.
> 
> As I already said I really believe the whole cooling
> device should be registered somewhere else. But if you
> do end up sticking with this then you MUST replace
> the acpi_video_backlight_use_native() calls with:
> 
> 	if (acpi_video_get_backlight_type() == acpi_backlight_native) {...}

Thank you very much for your detailed feedback. This helped.

I have moved the code from amdgpu to PMF driver which has changes for
DRM. This also has changed w.r.t thermal device change what you suggested.

I have used the checks where ever appropriate:
acpi_video_get_backlight_type() == acpi_backlight_native

Kindly take a look at v5 submission.

Thanks,
Shyam

> 
> Regards,
> 
> Hans
> 
> 
>