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

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>

quoted

quoted

quoted

quoted

quoted

+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