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
>
>
>
