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

[Hans de Goede <hdegoede@redhat.com>]

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) {...}

Regards,

Hans