Nouveau - Aug 2022 - Question about selecting ACPI_VIDEO for nouveau on non X86 platforms

Hi All,

Since 6.0-rc1 it is possible for ACPI_VIDEO to be enabled on non X86
platforms. I already send an email about this being a possible problem
for nouveau, when nouveau is builtin and apci_video is a module:

https://lore.kernel.org/linux-acpi/a385b626-217f-25be-f076-7478da3d1147 at
redhat.com/

"""
I just noticed this change:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=038275d227841d4978ceceb397b584b4b39f2b50

--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -210,7 +210,7 @@ config ACPI_TINY_POWER_BUTTON_SIGNAL
 
 config ACPI_VIDEO
 	tristate "Video"
-	depends on X86 && BACKLIGHT_CLASS_DEVICE
+	depends on BACKLIGHT_CLASS_DEVICE
 	depends on INPUT
 	select THERMAL
 	help

I think this is going to cause random-config build errors because
at the nouveau driver calls acpi_video functions and it expects
those functions to either be there or to get the stubs.

This is an issue when acpi_video is build as a module and the
i915 / nouveau code is builtin.

To avoid this issue nouveau does a select on ACPI_VIDEO,
here is the code from drivers/gpu/drm/nouveau/Kconfig

        # Similar to i915, we need to select ACPI_VIDEO and it's
dependencies
        select BACKLIGHT_CLASS_DEVICE if ACPI && X86
        select INPUT if ACPI && X86
        select THERMAL if ACPI && X86
        select ACPI_VIDEO if ACPI && X86

notice the if ACPI && X86, since ACPI_VIDEO can now be selected
on ARM, this can lead to ACPI_VIDEO being enabled as
module (so no stubs) while nouveau can be builtin which will
lead to unresolved symbol errors in nouveau when building the zImage.

I believe that to fix this the conditions after the select must
be changed from "if ACPI && X86" to just "if ACPI"
"""

I was looking at a lkp warning in my backlight refactor series
which is explained by the above today:

https://lore.kernel.org/lkml/202208231625.icHjRXDI-lkp at intel.com/

and while working on fixing this I noticed that nouveau not
only assumes ACPI[_VIDEO] is only available on X86 in its Kconfig
but also in other places:

drivers/gpu/drm/nouveau/Kbuild:

ifdef CONFIG_X86
nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
endif

drivers/gpu/drm/nouveau/nouveau_acpi.h:

#if defined(CONFIG_ACPI) && defined(CONFIG_X86)
bool nouveau_is_optimus(void);
...
#else
static inline bool nouveau_is_optimus(void) { return false; };
...
#endif

Since currently the only nouveau dep on acpi_video is
inside nouveau_acpi.c the not building + stubbing of
nouveau_acpi.c is saving us from the problem which I expected
in my mail quoted above.

The backlight refactor series breaks this though, because
it adds calls to:
acpi_video_backlight_use_native()
acpi_video_register_backlight()

to nouveau outside of the drivers/gpu/drm/nouveau/nouveau_acpi.c
file.

To fix this for the next version of the backlight refactor series
I have added wrappers for:
acpi_video_backlight_use_native()
acpi_video_register_backlight()
to nouveau_acpi.?

After thinking about this a bit this seemed by far the cleanest
and consistent with how the current nouveau code is abstracting
other ACPI use, so this makes the backlight changes consistent
with existing nouveau practices.

Lyude, because of these changes I've dropped your Reviewed-by.
I'll send out a new version 4 series sometime today, can you
please re-review the 2 nouveau patches?

###

My first instinct to fix this, was to adjust the nouveau Kconfig bits
selecting ACPI_VIDEO to take into account that it now is available
on non X86 too:

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 03d12caf9e26..26b895a49b9c 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -16,10 +16,10 @@ config DRM_NOUVEAU
 	select MXM_WMI if ACPI && X86
 	select POWER_SUPPLY
 	# Similar to i915, we need to select ACPI_VIDEO and it's dependencies
-	select BACKLIGHT_CLASS_DEVICE if ACPI && X86
-	select INPUT if ACPI && X86
-	select THERMAL if ACPI && X86
-	select ACPI_VIDEO if ACPI && X86
+	select BACKLIGHT_CLASS_DEVICE if ACPI
+	select INPUT if ACPI
+	select THERMAL if ACPI
+	select ACPI_VIDEO if ACPI
 	select SND_HDA_COMPONENT if SND_HDA_CORE
 	help
 	  Choose this option for open-source NVIDIA support.

We could still make this change together with Kbuild +
nouveau_acpi.h to build on non x86 too. This might be good to
prepare for some aarch64 devices which may use some of the
ACPI bits, they may need e.g. acpi_video_get_edid().

OTOH this might cause unexpected issue so it might be better
to wait with making such a change until it is actually
necessary.

Regards,

Hans