In the past this only led to compilation issues. Also the small amount of extra .text shouldn't really matter compared to the entire nouveau driver anyway.
Cc: Arnd Bergmann arnd@kernel.org Cc: Lyude Paul lyude@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Randy Dunlap rdunlap@infradead.org Cc: Daniel Vetter daniel@ffwll.ch Cc: nouveau@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau") Signed-off-by: Karol Herbst kherbst@redhat.com --- drivers/gpu/drm/nouveau/Kbuild | 2 +- drivers/gpu/drm/nouveau/Kconfig | 13 ++--------- drivers/gpu/drm/nouveau/dispnv50/disp.c | 4 ---- drivers/gpu/drm/nouveau/nouveau_connector.h | 24 --------------------- 4 files changed, 3 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild index 60586fb8275e..f6957e7ad807 100644 --- a/drivers/gpu/drm/nouveau/Kbuild +++ b/drivers/gpu/drm/nouveau/Kbuild @@ -49,7 +49,7 @@ nouveau-y += nouveau_ttm.o nouveau-y += nouveau_vmm.o
# DRM - modesetting -nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o +nouveau-y += nouveau_backlight.o nouveau-y += nouveau_bios.o nouveau-y += nouveau_connector.o nouveau-y += nouveau_display.o diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 9436310d0854..2e159b0ea7fb 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -7,14 +7,13 @@ config DRM_NOUVEAU select DRM_KMS_HELPER select DRM_TTM select DRM_TTM_HELPER - select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT - select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT + select BACKLIGHT_CLASS_DEVICE + select ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86 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 @@ -85,14 +84,6 @@ config NOUVEAU_DEBUG_PUSH Say Y here if you want to enable verbose push buffer debug output and sanity checks.
-config DRM_NOUVEAU_BACKLIGHT - bool "Support for backlight control" - depends on DRM_NOUVEAU - default y - help - Say Y here if you want to control the backlight of your display - (e.g. a laptop panel). - config DRM_NOUVEAU_SVM bool "(EXPERIMENTAL) Enable SVM (Shared Virtual Memory) support" depends on DEVICE_PRIVATE diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 093e1f7163b3..b30fd0f4a541 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1712,9 +1712,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_state *sta struct drm_device *dev = encoder->dev; struct nouveau_drm *drm = nouveau_drm(dev); struct nouveau_connector *nv_connector; -#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT struct nouveau_backlight *backlight; -#endif struct nvbios *bios = &drm->vbios; bool hda = false; u8 proto = NV507D_SOR_SET_CONTROL_PROTOCOL_CUSTOM; @@ -1790,12 +1788,10 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_state *sta
nv50_audio_enable(encoder, nv_crtc, nv_connector, state, mode);
-#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT backlight = nv_connector->backlight; if (backlight && backlight->uses_dpcd) drm_edp_backlight_enable(&nv_connector->aux, &backlight->edp_info, (u16)backlight->dev->props.brightness); -#endif
break; default: diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h index 40f90e353540..88ed64efe9e9 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.h +++ b/drivers/gpu/drm/nouveau/nouveau_connector.h @@ -45,7 +45,6 @@ struct nvkm_i2c_port; struct dcb_output;
-#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT struct nouveau_backlight { struct backlight_device *dev;
@@ -54,7 +53,6 @@ struct nouveau_backlight {
int id; }; -#endif
#define nouveau_conn_atom(p) \ container_of((p), struct nouveau_conn_atom, state) @@ -133,9 +131,7 @@ struct nouveau_connector { struct nouveau_encoder *detected_encoder; struct edid *edid; struct drm_display_mode *native_mode; -#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT struct nouveau_backlight *backlight; -#endif /* * Our connector property code expects a nouveau_conn_atom struct * even on pre-nv50 where we do not support atomic. This embedded @@ -220,29 +216,9 @@ nouveau_conn_mode_clock_valid(const struct drm_display_mode *, const unsigned max_clock, unsigned *clock);
-#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT extern int nouveau_backlight_init(struct drm_connector *); extern void nouveau_backlight_fini(struct drm_connector *); extern void nouveau_backlight_ctor(void); extern void nouveau_backlight_dtor(void); -#else -static inline int -nouveau_backlight_init(struct drm_connector *connector) -{ - return 0; -} - -static inline void -nouveau_backlight_fini(struct drm_connector *connector) { -} - -static inline void -nouveau_backlight_ctor(void) { -} - -static inline void -nouveau_backlight_dtor(void) { -} -#endif
#endif /* __NOUVEAU_CONNECTOR_H__ */
On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst kherbst@redhat.com wrote:
In the past this only led to compilation issues. Also the small amount of extra .text shouldn't really matter compared to the entire nouveau driver anyway.
select DRM_TTM_HELPER
select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHTselect ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT
select BACKLIGHT_CLASS_DEVICEselect ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86
I think the logic needs to be the reverse: instead of 'select BACKLIGHT_CLASS_DEVICE', this should be 'depends on BACKLIGHT_CLASS_DEVICE', and the same for ACPI_VIDEO.
We may want to add 'default DRM || FB' to BACKLIGHT_CLASS_DEVICE in the process so we don't lose it for users doing 'make oldconfig' or 'make defconfig'
The rest of the patch looks good to me.
Arnd
On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst kherbst@redhat.com wrote:
In the past this only led to compilation issues. Also the small amount of extra .text shouldn't really matter compared to the entire nouveau driver anyway.
select DRM_TTM_HELPER
select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHTselect ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT
select BACKLIGHT_CLASS_DEVICEselect ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86I think the logic needs to be the reverse: instead of 'select BACKLIGHT_CLASS_DEVICE', this should be 'depends on BACKLIGHT_CLASS_DEVICE', and the same for ACPI_VIDEO.
We may want to add 'default DRM || FB' to BACKLIGHT_CLASS_DEVICE in the process so we don't lose it for users doing 'make oldconfig' or 'make defconfig'
yeah.. not sure. I tested it locally (config without backlight enabled) and olddefconfig just worked. I think the problem with "depends" is that the user needs to enable backlight support first before even seeing nouveau and I don't know if that makes sense. But maybe "default" is indeed helping here in this case.
The rest of the patch looks good to me.
Arnd
On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst kherbst@redhat.com wrote:
In the past this only led to compilation issues. Also the small amount of extra .text shouldn't really matter compared to the entire nouveau driver anyway.
select DRM_TTM_HELPER
select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHTselect ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT
select BACKLIGHT_CLASS_DEVICEselect ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86I think the logic needs to be the reverse: instead of 'select BACKLIGHT_CLASS_DEVICE', this should be 'depends on BACKLIGHT_CLASS_DEVICE', and the same for ACPI_VIDEO.
We may want to add 'default DRM || FB' to BACKLIGHT_CLASS_DEVICE in the process so we don't lose it for users doing 'make oldconfig' or 'make defconfig'
I think the problem with "depends" is that the user needs to enable backlight support first before even seeing nouveau and I don't know if that makes sense. But maybe "default" is indeed helping here in this case.
In general, no driver should ever 'select' a subsystem. Otherwise you end up with two problems:
- enabling this one driver suddenly makes all other drivers that have a dependency on this visible, and some of those might have a 'default y', so you end up with a ton of stuff in the kernel that would otherwise not be there.
- It becomes impossible to turn it off as long as some driver has that 'select'. This is the pretty much the same problem as the one you describe, just the other side of it.
- You run into dependency loops that prevent a successful build when some other driver has a 'depends on'. Preventing these loops was the main reason I said we should do this change.
In theory we could change the other 85 drivers that use 'depends on' today, and make BACKLIGHT_CLASS_DEVICE a hidden symbol that only ever selected by the drivers that need it. This would avoid the third problem but not the other one.
Arnd
On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst kherbst@redhat.com wrote:
In the past this only led to compilation issues. Also the small amount of extra .text shouldn't really matter compared to the entire nouveau driver anyway.
select DRM_TTM_HELPER
select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHTselect ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT
select BACKLIGHT_CLASS_DEVICEselect ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86I think the logic needs to be the reverse: instead of 'select BACKLIGHT_CLASS_DEVICE', this should be 'depends on BACKLIGHT_CLASS_DEVICE', and the same for ACPI_VIDEO.
We may want to add 'default DRM || FB' to BACKLIGHT_CLASS_DEVICE in the process so we don't lose it for users doing 'make oldconfig' or 'make defconfig'
I think the problem with "depends" is that the user needs to enable backlight support first before even seeing nouveau and I don't know if that makes sense. But maybe "default" is indeed helping here in this case.
In general, no driver should ever 'select' a subsystem. Otherwise you end up with two problems:
- enabling this one driver suddenly makes all other drivers that have
a dependency on this visible, and some of those might have a 'default y', so you end up with a ton of stuff in the kernel that would otherwise not be there.
It becomes impossible to turn it off as long as some driver has that 'select'. This is the pretty much the same problem as the one you describe, just the other side of it.
You run into dependency loops that prevent a successful build when some other driver has a 'depends on'. Preventing these loops was the main reason I said we should do this change.
In theory we could change the other 85 drivers that use 'depends on' today, and make BACKLIGHT_CLASS_DEVICE a hidden symbol that only ever selected by the drivers that need it. This would avoid the third problem but not the other one.
Arnd
I see. Yeah, I guess we can do it this way then. I just wasn't aware of the bigger picture here. Thanks for explaining.
On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst kherbst@redhat.com wrote:
In the past this only led to compilation issues. Also the small amount of extra .text shouldn't really matter compared to the entire nouveau driver anyway.
select DRM_TTM_HELPER
select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHTselect ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT
select BACKLIGHT_CLASS_DEVICEselect ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86I think the logic needs to be the reverse: instead of 'select BACKLIGHT_CLASS_DEVICE', this should be 'depends on BACKLIGHT_CLASS_DEVICE', and the same for ACPI_VIDEO.
We may want to add 'default DRM || FB' to BACKLIGHT_CLASS_DEVICE in the process so we don't lose it for users doing 'make oldconfig' or 'make defconfig'
I think the problem with "depends" is that the user needs to enable backlight support first before even seeing nouveau and I don't know if that makes sense. But maybe "default" is indeed helping here in this case.
In general, no driver should ever 'select' a subsystem. Otherwise you end up with two problems:
- enabling this one driver suddenly makes all other drivers that have
a dependency on this visible, and some of those might have a 'default y', so you end up with a ton of stuff in the kernel that would otherwise not be there.
It becomes impossible to turn it off as long as some driver has that 'select'. This is the pretty much the same problem as the one you describe, just the other side of it.
You run into dependency loops that prevent a successful build when some other driver has a 'depends on'. Preventing these loops was the main reason I said we should do this change.
In theory we could change the other 85 drivers that use 'depends on' today, and make BACKLIGHT_CLASS_DEVICE a hidden symbol that only ever selected by the drivers that need it. This would avoid the third problem but not the other one.
ArndI see. Yeah, I guess we can do it this way then. I just wasn't aware of the bigger picture here. Thanks for explaining.
yeah... that doesn't work. So the issue is, that X86_PLATFORM_DEVICES is a little bit in the way. If I remove the select X86_PLATFORM_DEVICES then I guess problems once ACPI is enabled, but if I keep it, I get cyclic dep errors :/
On Sat, Jul 24, 2021 at 2:52 PM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst kherbst@redhat.com wrote:
- You run into dependency loops that prevent a successful build when some other driver has a 'depends on'. Preventing these loops was the main reason I said we should do this change.
In theory we could change the other 85 drivers that use 'depends on' today, and make BACKLIGHT_CLASS_DEVICE a hidden symbol that only ever selected by the drivers that need it. This would avoid the third problem but not the other one.
I see. Yeah, I guess we can do it this way then. I just wasn't aware of the bigger picture here. Thanks for explaining.
yeah... that doesn't work. So the issue is, that X86_PLATFORM_DEVICES is a little bit in the way. If I remove the select X86_PLATFORM_DEVICES then I guess problems once ACPI is enabled, but if I keep it, I get cyclic dep errors :/
Right, this is the exact problem I explained: since all other drivers use 'depends on X86_PLATFORM_DEVICES' instead of 'select', you get a loop again. Prior to changing the BACKLIGHT_CLASS_DEVICE dependency, nouveau was pretty much on top of everything else in the hierarchy, changing part of it can result in a loop.
I see that there are about ten more 'select' statements that look like they should not be there, and almost all of them were added in order to be able to 'select MXM_WMI'.
I think we can go as far as the patch below, which I've put in my randconfig build machine, on top of your patch. I'm not entirely sure how strong the dependency on MXM_WMI is: does it cause a build failure when that is not enabled, or was this select just added for convenience so users don't get surprised when it's missing?
Arnd
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 9c2108b48524..f2585416507e 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -3,21 +3,14 @@ config DRM_NOUVEAU tristate "Nouveau (NVIDIA) cards" depends on DRM && PCI && MMU depends on AGP || !AGP + depends on ACPI_VIDEO || !ACPI + depends on BACKLIGHT_CLASS_DEVICE + depends on MXM_WMI || !X86 || !ACPI select IOMMU_API select FW_LOADER select DRM_KMS_HELPER select DRM_TTM select DRM_TTM_HELPER - select BACKLIGHT_CLASS_DEVICE - select ACPI_VIDEO if ACPI && X86 && INPUT - select X86_PLATFORM_DEVICES if ACPI && X86 - select ACPI_WMI if ACPI && X86 - select MXM_WMI if ACPI && X86 - select POWER_SUPPLY - # Similar to i915, we need to select ACPI_VIDEO and it's dependencies - select INPUT if ACPI && X86 - select THERMAL if ACPI && X86 - select ACPI_VIDEO if ACPI && X86 select SND_HDA_COMPONENT if SND_HDA_CORE help Choose this option for open-source NVIDIA support.
On Sat, Jul 24, 2021 at 4:05 PM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 2:52 PM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst kherbst@redhat.com wrote:
On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst kherbst@redhat.com wrote:
- You run into dependency loops that prevent a successful build when some other driver has a 'depends on'. Preventing these loops was the main reason I said we should do this change.
In theory we could change the other 85 drivers that use 'depends on' today, and make BACKLIGHT_CLASS_DEVICE a hidden symbol that only ever selected by the drivers that need it. This would avoid the third problem but not the other one.
I see. Yeah, I guess we can do it this way then. I just wasn't aware of the bigger picture here. Thanks for explaining.
yeah... that doesn't work. So the issue is, that X86_PLATFORM_DEVICES is a little bit in the way. If I remove the select X86_PLATFORM_DEVICES then I guess problems once ACPI is enabled, but if I keep it, I get cyclic dep errors :/
Right, this is the exact problem I explained: since all other drivers use 'depends on X86_PLATFORM_DEVICES' instead of 'select', you get a loop again. Prior to changing the BACKLIGHT_CLASS_DEVICE dependency, nouveau was pretty much on top of everything else in the hierarchy, changing part of it can result in a loop.
I see that there are about ten more 'select' statements that look like they should not be there, and almost all of them were added in order to be able to 'select MXM_WMI'.
I think we can go as far as the patch below, which I've put in my randconfig build machine, on top of your patch. I'm not entirely sure how strong the dependency on MXM_WMI is: does it cause a build failure when that is not enabled, or was this select just added for convenience so users don't get surprised when it's missing?
Arnd
we use the MXM_WMI in code. We also have to keep arm in mind and not break stuff there. So I will try to play around with your changes and see how that goes.
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 9c2108b48524..f2585416507e 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -3,21 +3,14 @@ config DRM_NOUVEAU tristate "Nouveau (NVIDIA) cards" depends on DRM && PCI && MMU depends on AGP || !AGP
depends on ACPI_VIDEO || !ACPIdepends on BACKLIGHT_CLASS_DEVICEdepends on MXM_WMI || !X86 || !ACPI select IOMMU_API select FW_LOADER select DRM_KMS_HELPER select DRM_TTM select DRM_TTM_HELPER
select BACKLIGHT_CLASS_DEVICEselect ACPI_VIDEO if ACPI && X86 && INPUTselect X86_PLATFORM_DEVICES if ACPI && X86select ACPI_WMI if ACPI && X86select MXM_WMI if ACPI && X86select POWER_SUPPLY# Similar to i915, we need to select ACPI_VIDEO and it's dependenciesselect INPUT if ACPI && X86select THERMAL if ACPI && X86select ACPI_VIDEO if ACPI && X86 select SND_HDA_COMPONENT if SND_HDA_CORE help Choose this option for open-source NVIDIA support.
On Sat, Jul 24, 2021 at 4:14 PM Karol Herbst kherbst@redhat.com wrote:
we use the MXM_WMI in code. We also have to keep arm in mind and not break stuff there. So I will try to play around with your changes and see how that goes.
Ok, should find any randconfig build failures for arm, arm64 or x86 over the weekend. I also this on linux-next today
ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `intel_backlight_device_register': intel_panel.c:(.text+0x2804): undefined reference to `backlight_device_register' ld: intel_panel.c:(.text+0x284e): undefined reference to `backlight_device_register' ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `intel_backlight_device_unregister': intel_panel.c:(.text+0x28b1): undefined reference to `backlight_device_unregister'
and I added this same thing there to see how it goes:
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index 87825d36335b..69c6b7aec49e 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -3,6 +3,8 @@ config DRM_I915 tristate "Intel 8xx/9xx/G3x/G4x/HD Graphics" depends on DRM depends on X86 && PCI + depends on ACPI_VIDEO || !ACPI + depends on BACKLIGHT_CLASS_DEVICE select INTEL_GTT select INTERVAL_TREE # we need shmfs for the swappable backing store, and in particular @@ -16,10 +18,6 @@ config DRM_I915 select IRQ_WORK # i915 depends on ACPI_VIDEO when ACPI is enabled # but for select to work, need to select ACPI_VIDEO's dependencies, ick - select DRM_I915_BACKLIGHT if ACPI - select INPUT if ACPI - select ACPI_VIDEO if ACPI - select ACPI_BUTTON if ACPI select SYNC_FILE select IOSF_MBI select CRC32 @@ -64,13 +62,7 @@ config DRM_I915_FORCE_PROBE Use "*" to force probe the driver for all known devices.
config DRM_I915_BACKLIGHT - tristate "Control backlight support" - depends on DRM_I915 - default DRM_I915 - select BACKLIGHT_CLASS_DEVICE - help - Say Y here if you want to control the backlight of your display - (e.g. a laptop panel). + def_tristate DRM_I915
config DRM_I915_CAPTURE_ERROR bool "Enable capturing GPU state following a hang"
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Cc: Arnd Bergmann arnd@kernel.org Cc: Lyude Paul lyude@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Randy Dunlap rdunlap@infradead.org Cc: Daniel Vetter daniel@ffwll.ch Cc: nouveau@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/bridge/Kconfig | 2 +- drivers/gpu/drm/fsl-dcu/Kconfig | 2 +- drivers/gpu/drm/gud/Kconfig | 2 +- drivers/gpu/drm/nouveau/Kconfig | 2 +- drivers/platform/x86/Kconfig | 4 ++-- drivers/staging/olpc_dcon/Kconfig | 2 +- drivers/usb/misc/Kconfig | 2 +- drivers/video/fbdev/Kconfig | 2 +- 8 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 431b6e12a81f..dc68532ede38 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -173,9 +173,9 @@ config DRM_NXP_PTN3460 config DRM_PARADE_PS8622 tristate "Parade eDP/LVDS bridge" depends on OF + depends on BACKLIGHT_CLASS_DEVICE select DRM_PANEL select DRM_KMS_HELPER - select BACKLIGHT_CLASS_DEVICE help Parade eDP-LVDS bridge chip driver.
diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig index d7dd8ba90e3a..79bfd7e6f6dc 100644 --- a/drivers/gpu/drm/fsl-dcu/Kconfig +++ b/drivers/gpu/drm/fsl-dcu/Kconfig @@ -2,7 +2,7 @@ config DRM_FSL_DCU tristate "DRM Support for Freescale DCU" depends on DRM && OF && ARM && COMMON_CLK - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER select DRM_PANEL diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig index 1c8601bf4d91..91a118928af7 100644 --- a/drivers/gpu/drm/gud/Kconfig +++ b/drivers/gpu/drm/gud/Kconfig @@ -3,10 +3,10 @@ config DRM_GUD tristate "GUD USB Display" depends on DRM && USB + depends on BACKLIGHT_CLASS_DEVICE select LZ4_COMPRESS select DRM_KMS_HELPER select DRM_GEM_SHMEM_HELPER - select BACKLIGHT_CLASS_DEVICE help This is a DRM display driver for GUD USB Displays or display adapters. diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 2e159b0ea7fb..afb3eede8e2b 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -2,12 +2,12 @@ config DRM_NOUVEAU tristate "Nouveau (NVIDIA) cards" depends on DRM && PCI && MMU + depends on BACKLIGHT_CLASS_DEVICE select IOMMU_API select FW_LOADER select DRM_KMS_HELPER select DRM_TTM select DRM_TTM_HELPER - select BACKLIGHT_CLASS_DEVICE select ACPI_VIDEO if ACPI && X86 && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86 diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 7d385c3b2239..278368985fb2 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -838,7 +838,7 @@ config SAMSUNG_LAPTOP config SAMSUNG_Q10 tristate "Samsung Q10 Extras" depends on ACPI - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE help This driver provides support for backlight control on Samsung Q10 and related laptops, including Dell Latitude X200. @@ -935,7 +935,7 @@ config ACPI_CMPC tristate "CMPC Laptop Extras" depends on ACPI && INPUT depends on RFKILL || RFKILL=n - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE help Support for Intel Classmate PC ACPI devices, including some keys as input device, backlight device, tablet and accelerometer diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig index d1a0dea09ef0..a9f36538d7ab 100644 --- a/drivers/staging/olpc_dcon/Kconfig +++ b/drivers/staging/olpc_dcon/Kconfig @@ -4,7 +4,7 @@ config FB_OLPC_DCON depends on OLPC && FB depends on I2C depends on GPIO_CS5535 && ACPI - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE help In order to support very low power operation, the XO laptop uses a secondary Display CONtroller, or DCON. This secondary controller diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig index 8f1144359012..6f769a1616f0 100644 --- a/drivers/usb/misc/Kconfig +++ b/drivers/usb/misc/Kconfig @@ -132,7 +132,7 @@ config USB_FTDI_ELAN
config USB_APPLEDISPLAY tristate "Apple Cinema Display support" - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE help Say Y here if you want to control the backlight of Apple Cinema Displays over USB. This driver provides a sysfs interface. diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index d33c5cd684c0..b4d5837b61de 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -187,7 +187,7 @@ config FB_MACMODES config FB_BACKLIGHT tristate depends on FB - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE
config FB_MODE_HELPERS bool "Enable Video Mode Handling Helpers"
On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst kherbst@redhat.com wrote:
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Looks good to me. We'd probably want to make the BACKLIGHT_CLASS_DEVICE option itself 'default FB || DRM' though, to ensure that defconfigs keep working.
Arnd
On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst kherbst@redhat.com wrote:
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Looks good to me. We'd probably want to make the BACKLIGHT_CLASS_DEVICE option itself 'default FB || DRM' though, to ensure that defconfigs keep working.
okay cool. Will send out a proper updated patch series soonish.
Arnd
On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst kherbst@redhat.com wrote:
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Looks good to me. We'd probably want to make the BACKLIGHT_CLASS_DEVICE option itself 'default FB || DRM' though, to ensure that defconfigs keep working.
okay cool. Will send out a proper updated patch series soonish.
mhh, actually that breaks drivers selecting FB_BACKLIGHT as now BACKLIGHT_CLASS_DEVICE might be disabled :(
somehow it doesn't feel like worth the effort converting it all over to depend.. dunno.
Atm I would just use "select" in nouveau and deal with the conversion later once somebody gets annoyed enough or so...
Arnd
On Wed, Aug 4, 2021 at 8:59 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst kherbst@redhat.com wrote:
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Looks good to me. We'd probably want to make the BACKLIGHT_CLASS_DEVICE option itself 'default FB || DRM' though, to ensure that defconfigs keep working.
okay cool. Will send out a proper updated patch series soonish.
mhh, actually that breaks drivers selecting FB_BACKLIGHT as now BACKLIGHT_CLASS_DEVICE might be disabled :(
Are you sure? It should already be the case that any driver that selects FB_BACKLIGHT either 'depends on BACKLIGHT_CLASS_DEVICE' or 'select BACKLIGHT_CLASS_DEVICE'.
If you change all the 'select BACKLIGHT_CLASS_DEVICE' to 'depends on', I don't see a problem with doing 'select FB_BACKLIGHT' from those.
I have applied your patch to my randconfig tree and built a few dozen kernels, don't see any regressions so far, but will let it run over night.
Arnd
On Wed, Aug 4, 2021 at 11:10 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 8:59 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst kherbst@redhat.com wrote:
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Looks good to me. We'd probably want to make the BACKLIGHT_CLASS_DEVICE option itself 'default FB || DRM' though, to ensure that defconfigs keep working.
okay cool. Will send out a proper updated patch series soonish.
mhh, actually that breaks drivers selecting FB_BACKLIGHT as now BACKLIGHT_CLASS_DEVICE might be disabled :(
Are you sure? It should already be the case that any driver that selects FB_BACKLIGHT either 'depends on BACKLIGHT_CLASS_DEVICE' or 'select BACKLIGHT_CLASS_DEVICE'.
none of the fb drivers seem to do that.
If you change all the 'select BACKLIGHT_CLASS_DEVICE' to 'depends on', I don't see a problem with doing 'select FB_BACKLIGHT' from those.
I have applied your patch to my randconfig tree and built a few dozen kernels, don't see any regressions so far, but will let it run over night.
Arnd
On Thu, Aug 5, 2021 at 12:01 AM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 11:10 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 8:59 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst kherbst@redhat.com wrote:
On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann arnd@kernel.org wrote:
On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst kherbst@redhat.com wrote:
playing around a little bit with this, I think the original "select BACKLIGHT_CLASS_DEVICE" is fine. Atm we kind of have this weird mix of drivers selecting and others depending on it. We could of course convert everything over to depend, and break those cycling dependency issues with this.
Anyway this change on top of my initial patch is enough to make Kconfig happy and has the advantage of not having to mess with the deps of nouveau too much.
Looks good to me. We'd probably want to make the BACKLIGHT_CLASS_DEVICE option itself 'default FB || DRM' though, to ensure that defconfigs keep working.
okay cool. Will send out a proper updated patch series soonish.
mhh, actually that breaks drivers selecting FB_BACKLIGHT as now BACKLIGHT_CLASS_DEVICE might be disabled :(
Are you sure? It should already be the case that any driver that selects FB_BACKLIGHT either 'depends on BACKLIGHT_CLASS_DEVICE' or 'select BACKLIGHT_CLASS_DEVICE'.
none of the fb drivers seem to do that.
Ah, right, I see now that my randconfig series has a couple of patches applied that deal with other random failures, including this one:
https://patchwork.kernel.org/project/linux-fbdev/patch/20200417155553.675905...
Part of the series went in (through different ways) now, but this one never did.
Arnd
On Sat, 24 Jul 2021, Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 4:14 PM Karol Herbst kherbst@redhat.com wrote:
we use the MXM_WMI in code. We also have to keep arm in mind and not break stuff there. So I will try to play around with your changes and see how that goes.
Ok, should find any randconfig build failures for arm, arm64 or x86 over the weekend. I also this on linux-next today
ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `intel_backlight_device_register': intel_panel.c:(.text+0x2804): undefined reference to `backlight_device_register' ld: intel_panel.c:(.text+0x284e): undefined reference to `backlight_device_register' ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `intel_backlight_device_unregister': intel_panel.c:(.text+0x28b1): undefined reference to `backlight_device_unregister'
and I added this same thing there to see how it goes:
Last I checked (and it was a while a go) you really had to make all users of BACKLIGHT_CLASS_DEVICE depend not select it, otherwise you end up with recursive dependencies.
BR, Jani.
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index 87825d36335b..69c6b7aec49e 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -3,6 +3,8 @@ config DRM_I915 tristate "Intel 8xx/9xx/G3x/G4x/HD Graphics" depends on DRM depends on X86 && PCI
depends on ACPI_VIDEO || !ACPIdepends on BACKLIGHT_CLASS_DEVICE select INTEL_GTT select INTERVAL_TREE # we need shmfs for the swappable backing store, and in particular@@ -16,10 +18,6 @@ config DRM_I915 select IRQ_WORK # i915 depends on ACPI_VIDEO when ACPI is enabled # but for select to work, need to select ACPI_VIDEO's dependencies, ick
select DRM_I915_BACKLIGHT if ACPIselect INPUT if ACPIselect ACPI_VIDEO if ACPIselect ACPI_BUTTON if ACPI select SYNC_FILE select IOSF_MBI select CRC32@@ -64,13 +62,7 @@ config DRM_I915_FORCE_PROBE Use "*" to force probe the driver for all known devices.
config DRM_I915_BACKLIGHT
tristate "Control backlight support"depends on DRM_I915default DRM_I915select BACKLIGHT_CLASS_DEVICEhelpSay Y here if you want to control the backlight of your display(e.g. a laptop panel).
def_tristate DRM_I915config DRM_I915_CAPTURE_ERROR bool "Enable capturing GPU state following a hang"
On Mon, Aug 9, 2021 at 3:20 PM Jani Nikula jani.nikula@linux.intel.com wrote:
On Sat, 24 Jul 2021, Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 24, 2021 at 4:14 PM Karol Herbst kherbst@redhat.com wrote:
we use the MXM_WMI in code. We also have to keep arm in mind and not break stuff there. So I will try to play around with your changes and see how that goes.
Ok, should find any randconfig build failures for arm, arm64 or x86 over the weekend. I also this on linux-next today
ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `intel_backlight_device_register': intel_panel.c:(.text+0x2804): undefined reference to `backlight_device_register' ld: intel_panel.c:(.text+0x284e): undefined reference to `backlight_device_register' ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `intel_backlight_device_unregister': intel_panel.c:(.text+0x28b1): undefined reference to `backlight_device_unregister'
and I added this same thing there to see how it goes:
Last I checked (and it was a while a go) you really had to make all users of BACKLIGHT_CLASS_DEVICE depend not select it, otherwise you end up with recursive dependencies.
Yes, that is correct. It turns out that my randconfig tree already had a local patch to change most of the other users (everything outside of drivers/gpu) to 'depends on'.
Arnd
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Jani Nikula -
Karol Herbst