* [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration @ 2016-11-08 13:56 Arnd Bergmann 2016-11-08 15:46 ` Ilia Mirkin 2016-11-08 16:10 ` Martin Peres 0 siblings, 2 replies; 9+ messages in thread From: Arnd Bergmann @ 2016-11-08 13:56 UTC (permalink / raw) To: Ben Skeggs Cc: Arnd Bergmann, David Airlie, Martin Peres, dri-devel, nouveau, linux-kernel The newly introduced LED handling for nouveau fails to link when the driver is built-in but the LED subsystem is a loadable module: drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' This adds a separate Kconfig symbol for the LED support that correctly tracks the dependencies. Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/gpu/drm/nouveau/Kbuild | 2 +- drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild index fde6e3656636..5e00e911daa6 100644 --- a/drivers/gpu/drm/nouveau/Kbuild +++ b/drivers/gpu/drm/nouveau/Kbuild @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o nouveau-y += nouveau_drm.o nouveau-y += nouveau_hwmon.o nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o nouveau-y += nouveau_nvif.o nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o nouveau-y += nouveau_usif.o # userspace <-> nvif diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 78631fb61adf..715cd6f4dc31 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG The paranoia and spam levels will add a lot of extra checks which may potentially slow down driver operation. +config DRM_NOUVEAU_LED + bool "Support for logo LED" + depends on DRM_NOUVEAU && LEDS_CLASS + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) + help + Say Y here to enabling controlling the brightness of the + LED behind NVIDIA logo on certain Titan cards. + config NOUVEAU_DEBUG_DEFAULT int "Default debug level" depends on DRM_NOUVEAU diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h index 187ecdb82002..9c9bb6ac938e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_led.h +++ b/drivers/gpu/drm/nouveau/nouveau_led.h @@ -42,7 +42,7 @@ nouveau_led(struct drm_device *dev) } /* nouveau_led.c */ -#if IS_ENABLED(CONFIG_LEDS_CLASS) +#if IS_ENABLED(CONFIG_DRM_NOUVEAU_LED) int nouveau_led_init(struct drm_device *dev); void nouveau_led_suspend(struct drm_device *dev); void nouveau_led_resume(struct drm_device *dev); -- 2.9.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 13:56 [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Arnd Bergmann @ 2016-11-08 15:46 ` Ilia Mirkin 2016-11-08 15:52 ` Arnd Bergmann 2016-11-08 16:16 ` Karol Herbst 2016-11-08 16:10 ` Martin Peres 1 sibling, 2 replies; 9+ messages in thread From: Ilia Mirkin @ 2016-11-08 15:46 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Ben Skeggs, nouveau, linux-kernel, dri-devel On Tue, Nov 8, 2016 at 8:56 AM, Arnd Bergmann <arnd@arndb.de> wrote: > The newly introduced LED handling for nouveau fails to link when the > driver is built-in but the LED subsystem is a loadable module: > > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': > tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': > tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': > tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': > tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' > > This adds a separate Kconfig symbol for the LED support that > correctly tracks the dependencies. > > Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/nouveau/Kbuild | 2 +- > drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ > drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild > index fde6e3656636..5e00e911daa6 100644 > --- a/drivers/gpu/drm/nouveau/Kbuild > +++ b/drivers/gpu/drm/nouveau/Kbuild > @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o > nouveau-y += nouveau_drm.o > nouveau-y += nouveau_hwmon.o > nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o > -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o > +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o > nouveau-y += nouveau_nvif.o > nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o > nouveau-y += nouveau_usif.o # userspace <-> nvif > diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig > index 78631fb61adf..715cd6f4dc31 100644 > --- a/drivers/gpu/drm/nouveau/Kconfig > +++ b/drivers/gpu/drm/nouveau/Kconfig > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG > The paranoia and spam levels will add a lot of extra checks which > may potentially slow down driver operation. > > +config DRM_NOUVEAU_LED > + bool "Support for logo LED" > + depends on DRM_NOUVEAU && LEDS_CLASS > + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) > + help > + Say Y here to enabling controlling the brightness of the > + LED behind NVIDIA logo on certain Titan cards. This is a very odd restriction... could this be written as depends on DRM_NOUVEAU select LEDS_CLASS Or will that not flip the LEDS_CLASS from m to y in case DRM_NOUVEAU=y? If not, is there a way to cause that to happen? Separately, perhaps we should just drop this LEDS_CLASS select into DRM_NOUVEAU? We've tended to avoid adding tons of options. Cheers, -ilia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 15:46 ` Ilia Mirkin @ 2016-11-08 15:52 ` Arnd Bergmann 2016-11-08 16:07 ` [Nouveau] " Lukas Wunner 2016-11-08 16:16 ` Karol Herbst 1 sibling, 1 reply; 9+ messages in thread From: Arnd Bergmann @ 2016-11-08 15:52 UTC (permalink / raw) To: Ilia Mirkin; +Cc: Ben Skeggs, nouveau, linux-kernel, dri-devel On Tuesday, November 8, 2016 10:46:07 AM CET Ilia Mirkin wrote: > > diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig > > index 78631fb61adf..715cd6f4dc31 100644 > > --- a/drivers/gpu/drm/nouveau/Kconfig > > +++ b/drivers/gpu/drm/nouveau/Kconfig > > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG > > The paranoia and spam levels will add a lot of extra checks which > > may potentially slow down driver operation. > > > > +config DRM_NOUVEAU_LED > > + bool "Support for logo LED" > > + depends on DRM_NOUVEAU && LEDS_CLASS > > + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) > > + help > > + Say Y here to enabling controlling the brightness of the > > + LED behind NVIDIA logo on certain Titan cards. > > This is a very odd restriction... could this be written as > > depends on DRM_NOUVEAU > select LEDS_CLASS > > Or will that not flip the LEDS_CLASS from m to y in case > DRM_NOUVEAU=y? If not, is there a way to cause that to happen? Using 'select' on user-selectable symbols risks introducing circular dependencies. > Separately, perhaps we should just drop this LEDS_CLASS select into > DRM_NOUVEAU? We've tended to avoid adding tons of options. My first attempt was to add "depends on LEDS_CLASS || !LEDS_CLASS" to DRM_NOUVEAU, which led to one long circle: drivers/usb/Kconfig:39:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/usb/Kconfig:39: symbol USB is selected by MOUSE_APPLETOUCH For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/mouse/Kconfig:187: symbol MOUSE_APPLETOUCH depends on INPUT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/Kconfig:8: symbol INPUT is selected by VT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/tty/Kconfig:12: symbol VT is selected by FB_STI For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:669: symbol FB_STI depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:42: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:36: symbol DRM_KMS_HELPER is selected by DRM_NOUVEAU For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/nouveau/Kconfig:1: symbol DRM_NOUVEAU depends on LEDS_CLASS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/leds/Kconfig:16: symbol LEDS_CLASS is selected by OMAP_DEBUG_LEDS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" arch/arm/plat-omap/Kconfig:19: symbol OMAP_DEBUG_LEDS depends on NEW_LEDS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by ATH9K_HTC For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/net/wireless/ath/ath9k/Kconfig:158: symbol ATH9K_HTC depends on USB Tue, 08 Nov 2016 11:49:44 +0100 build/0x3053A542_defconfig warnings drivers/usb/Kconfig:39:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/usb/Kconfig:39: symbol USB is selected by MOUSE_APPLETOUCH For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/mouse/Kconfig:187: symbol MOUSE_APPLETOUCH depends on INPUT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/Kconfig:8: symbol INPUT is selected by VT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/tty/Kconfig:12: symbol VT is selected by FB_STI For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:669: symbol FB_STI depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:42: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:36: symbol DRM_KMS_HELPER is selected by DRM_NOUVEAU For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/nouveau/Kconfig:1: symbol DRM_NOUVEAU depends on LEDS_CLASS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/leds/Kconfig:16: symbol LEDS_CLASS is selected by OMAP_DEBUG_LEDS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" arch/arm/plat-omap/Kconfig:19: symbol OMAP_DEBUG_LEDS depends on NEW_LEDS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by ATH9K_HTC For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/net/wireless/ath/ath9k/Kconfig:158: symbol ATH9K_HTC depends on USB The underlying problem is that we already have a number of other symbols that either have "depends on LEDS_CLASS" or "select LEDS_CLASS". To clean that up properly, we should either make the symbol itself hidden and only select it from other drivers, or use "depends on LEDS_CLASS" everywhere. Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() in the header file, to stub out the calls into the new file, but that can be a bit confusing. Arnd ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 15:52 ` Arnd Bergmann @ 2016-11-08 16:07 ` Lukas Wunner 2016-11-08 16:12 ` Arnd Bergmann 0 siblings, 1 reply; 9+ messages in thread From: Lukas Wunner @ 2016-11-08 16:07 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Ilia Mirkin, nouveau, Ben Skeggs, dri-devel, linux-kernel On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: > The underlying problem is that we already have a number of other > symbols that either have "depends on LEDS_CLASS" or > "select LEDS_CLASS". To clean that up properly, we should either > make the symbol itself hidden and only select it from other drivers, > or use "depends on LEDS_CLASS" everywhere. > > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() > in the header file, to stub out the calls into the new file, but > that can be a bit confusing. Why don't you just add empty inline stubs for nouveau_led_init / _fini / _suspend / _resume? Thanks, Lukas ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 16:07 ` [Nouveau] " Lukas Wunner @ 2016-11-08 16:12 ` Arnd Bergmann 2016-11-08 16:21 ` Karol Herbst 0 siblings, 1 reply; 9+ messages in thread From: Arnd Bergmann @ 2016-11-08 16:12 UTC (permalink / raw) To: Lukas Wunner; +Cc: Ilia Mirkin, nouveau, Ben Skeggs, dri-devel, linux-kernel On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner wrote: > On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: > > The underlying problem is that we already have a number of other > > symbols that either have "depends on LEDS_CLASS" or > > "select LEDS_CLASS". To clean that up properly, we should either > > make the symbol itself hidden and only select it from other drivers, > > or use "depends on LEDS_CLASS" everywhere. > > > > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() > > in the header file, to stub out the calls into the new file, but > > that can be a bit confusing. > > Why don't you just add empty inline stubs for nouveau_led_init / _fini / > _suspend / _resume? > That's what I was suggesting: diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h index 9c9bb6ac938e..bc5f47cb516b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_led.h +++ b/drivers/gpu/drm/nouveau/nouveau_led.h @@ -35,21 +35,21 @@ struct nouveau_led { struct led_classdev led; }; static inline struct nouveau_led * nouveau_led(struct drm_device *dev) { return nouveau_drm(dev)->led; } /* nouveau_led.c */ -#if IS_ENABLED(CONFIG_LEDS_CLASS) +#if IS_REACHABLE(CONFIG_LEDS_CLASS) int nouveau_led_init(struct drm_device *dev); void nouveau_led_suspend(struct drm_device *dev); void nouveau_led_resume(struct drm_device *dev); void nouveau_led_fini(struct drm_device *dev); #else static inline int nouveau_led_init(struct drm_device *dev) { return 0; }; static inline void nouveau_led_suspend(struct drm_device *dev) { }; static inline void nouveau_led_resume(struct drm_device *dev) { }; static inline void nouveau_led_fini(struct drm_device *dev) { }; #endif The downside is that now the nouveau_led_init() just won't be called if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be surprising to users. Arnd ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 16:12 ` Arnd Bergmann @ 2016-11-08 16:21 ` Karol Herbst 2016-11-08 16:35 ` Emil Velikov 0 siblings, 1 reply; 9+ messages in thread From: Karol Herbst @ 2016-11-08 16:21 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Lukas Wunner, nouveau, dri-devel, Ben Skeggs, linux-kernel 2016-11-08 17:12 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner wrote: >> On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: >> > The underlying problem is that we already have a number of other >> > symbols that either have "depends on LEDS_CLASS" or >> > "select LEDS_CLASS". To clean that up properly, we should either >> > make the symbol itself hidden and only select it from other drivers, >> > or use "depends on LEDS_CLASS" everywhere. >> > >> > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() >> > in the header file, to stub out the calls into the new file, but >> > that can be a bit confusing. >> >> Why don't you just add empty inline stubs for nouveau_led_init / _fini / >> _suspend / _resume? >> > > That's what I was suggesting: > > diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h > index 9c9bb6ac938e..bc5f47cb516b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_led.h > +++ b/drivers/gpu/drm/nouveau/nouveau_led.h > @@ -35,21 +35,21 @@ struct nouveau_led { > struct led_classdev led; > }; > > static inline struct nouveau_led * > nouveau_led(struct drm_device *dev) > { > return nouveau_drm(dev)->led; > } > > /* nouveau_led.c */ > -#if IS_ENABLED(CONFIG_LEDS_CLASS) > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > int nouveau_led_init(struct drm_device *dev); > void nouveau_led_suspend(struct drm_device *dev); > void nouveau_led_resume(struct drm_device *dev); > void nouveau_led_fini(struct drm_device *dev); > #else > static inline int nouveau_led_init(struct drm_device *dev) { return 0; }; > static inline void nouveau_led_suspend(struct drm_device *dev) { }; > static inline void nouveau_led_resume(struct drm_device *dev) { }; > static inline void nouveau_led_fini(struct drm_device *dev) { }; > #endif > > The downside is that now the nouveau_led_init() just won't be called > if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be > surprising to users. yeah, it will. I guess it is fine to force LEDS to y if nouveau is set to y. The thinks I absolutely dislike is: 1. auto hiding of features I _want_ to have, because I would have to enable the dependencies first, which is like super annoying if there are somewhere else 2. preventing me from enabling something, cause the dependency is missing. We should clarify first if we actually want to enable those features optionally, because there isn't much of a reason not to enable the dependencies, except embedded systems. We have a lot more stuff where we could add things like that: hwmon, debugfs, acpi, compat and maybe there are even more things > > Arnd > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 16:21 ` Karol Herbst @ 2016-11-08 16:35 ` Emil Velikov 0 siblings, 0 replies; 9+ messages in thread From: Emil Velikov @ 2016-11-08 16:35 UTC (permalink / raw) To: Karol Herbst; +Cc: Arnd Bergmann, nouveau, Ben Skeggs, dri-devel, linux-kernel On 8 November 2016 at 16:21, Karol Herbst <karolherbst@gmail.com> wrote: > 2016-11-08 17:12 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: >> On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner wrote: >>> On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: >>> > The underlying problem is that we already have a number of other >>> > symbols that either have "depends on LEDS_CLASS" or >>> > "select LEDS_CLASS". To clean that up properly, we should either >>> > make the symbol itself hidden and only select it from other drivers, >>> > or use "depends on LEDS_CLASS" everywhere. >>> > >>> > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() >>> > in the header file, to stub out the calls into the new file, but >>> > that can be a bit confusing. >>> >>> Why don't you just add empty inline stubs for nouveau_led_init / _fini / >>> _suspend / _resume? >>> >> >> That's what I was suggesting: >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h >> index 9c9bb6ac938e..bc5f47cb516b 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_led.h >> +++ b/drivers/gpu/drm/nouveau/nouveau_led.h >> @@ -35,21 +35,21 @@ struct nouveau_led { >> struct led_classdev led; >> }; >> >> static inline struct nouveau_led * >> nouveau_led(struct drm_device *dev) >> { >> return nouveau_drm(dev)->led; >> } >> >> /* nouveau_led.c */ >> -#if IS_ENABLED(CONFIG_LEDS_CLASS) >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >> int nouveau_led_init(struct drm_device *dev); >> void nouveau_led_suspend(struct drm_device *dev); >> void nouveau_led_resume(struct drm_device *dev); >> void nouveau_led_fini(struct drm_device *dev); >> #else >> static inline int nouveau_led_init(struct drm_device *dev) { return 0; }; >> static inline void nouveau_led_suspend(struct drm_device *dev) { }; >> static inline void nouveau_led_resume(struct drm_device *dev) { }; >> static inline void nouveau_led_fini(struct drm_device *dev) { }; >> #endif >> >> The downside is that now the nouveau_led_init() just won't be called >> if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be >> surprising to users. > > yeah, it will. I guess it is fine to force LEDS to y if nouveau is set > to y. The thinks I absolutely dislike is: > 1. auto hiding of features I _want_ to have, because I would have to > enable the dependencies first, which is like super annoying if there > are somewhere else > 2. preventing me from enabling something, cause the dependency is missing. > > We should clarify first if we actually want to enable those features > optionally, because there isn't much of a reason not to enable the > dependencies, except embedded systems. We have a lot more stuff where > we could add things like that: hwmon, debugfs, acpi, compat and maybe > there are even more things > Sounds like people may have missed the core part: This/earlier patch are required due to select "abuse" elsewhere in the kernel. The IS_REACHABLE/DRM_NOUVEAU_LED is patch to workaround things on nouveau side, with a proper one to remove/untangle the "select LEDS_CLASS". The latter will likely be slow/pain, since devs love to use select because it's convenient (and indeed it is). Thus [temporary] workaround on nouveau side will be good in the short term. Afaict, "forcing LEDS to y if nouveau is set to y" is going the opposite direction of what one should be going ;-) Regards, Emil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 15:46 ` Ilia Mirkin 2016-11-08 15:52 ` Arnd Bergmann @ 2016-11-08 16:16 ` Karol Herbst 1 sibling, 0 replies; 9+ messages in thread From: Karol Herbst @ 2016-11-08 16:16 UTC (permalink / raw) To: Ilia Mirkin Cc: Arnd Bergmann, nouveau, Ben Skeggs, dri-devel, linux-kernel, Alexandre Courbot 2016-11-08 16:46 GMT+01:00 Ilia Mirkin <imirkin@alum.mit.edu>: > On Tue, Nov 8, 2016 at 8:56 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> The newly introduced LED handling for nouveau fails to link when the >> driver is built-in but the LED subsystem is a loadable module: >> >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': >> tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': >> tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': >> tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': >> tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' >> >> This adds a separate Kconfig symbol for the LED support that >> correctly tracks the dependencies. >> >> Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/gpu/drm/nouveau/Kbuild | 2 +- >> drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ >> drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- >> 3 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild >> index fde6e3656636..5e00e911daa6 100644 >> --- a/drivers/gpu/drm/nouveau/Kbuild >> +++ b/drivers/gpu/drm/nouveau/Kbuild >> @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o >> nouveau-y += nouveau_drm.o >> nouveau-y += nouveau_hwmon.o >> nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o >> -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o >> +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o >> nouveau-y += nouveau_nvif.o >> nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o >> nouveau-y += nouveau_usif.o # userspace <-> nvif >> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig >> index 78631fb61adf..715cd6f4dc31 100644 >> --- a/drivers/gpu/drm/nouveau/Kconfig >> +++ b/drivers/gpu/drm/nouveau/Kconfig >> @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG >> The paranoia and spam levels will add a lot of extra checks which >> may potentially slow down driver operation. >> >> +config DRM_NOUVEAU_LED >> + bool "Support for logo LED" >> + depends on DRM_NOUVEAU && LEDS_CLASS >> + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) >> + help >> + Say Y here to enabling controlling the brightness of the >> + LED behind NVIDIA logo on certain Titan cards. > > This is a very odd restriction... could this be written as > > depends on DRM_NOUVEAU > select LEDS_CLASS > > Or will that not flip the LEDS_CLASS from m to y in case > DRM_NOUVEAU=y? If not, is there a way to cause that to happen? > > Separately, perhaps we should just drop this LEDS_CLASS select into > DRM_NOUVEAU? We've tended to avoid adding tons of options. > well, that would mean that you always need the LEDS_CLASS and maybe on a tegra system you don't want to, so the led stuff should stay completely optional. Don't know though. Alex, maybe you want to clarify which dependencies should stay optional? If nobody on your side care, then we won't care as well and only add switches if users actually request it. > Cheers, > > -ilia > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration 2016-11-08 13:56 [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Arnd Bergmann 2016-11-08 15:46 ` Ilia Mirkin @ 2016-11-08 16:10 ` Martin Peres 1 sibling, 0 replies; 9+ messages in thread From: Martin Peres @ 2016-11-08 16:10 UTC (permalink / raw) To: Arnd Bergmann, Ben Skeggs; +Cc: David Airlie, dri-devel, nouveau, linux-kernel On 08/11/16 15:56, Arnd Bergmann wrote: > The newly introduced LED handling for nouveau fails to link when the > driver is built-in but the LED subsystem is a loadable module: > > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': > tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': > tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': > tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': > tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' > > This adds a separate Kconfig symbol for the LED support that > correctly tracks the dependencies. > > Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/nouveau/Kbuild | 2 +- > drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ > drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild > index fde6e3656636..5e00e911daa6 100644 > --- a/drivers/gpu/drm/nouveau/Kbuild > +++ b/drivers/gpu/drm/nouveau/Kbuild > @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o > nouveau-y += nouveau_drm.o > nouveau-y += nouveau_hwmon.o > nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o > -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o > +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o > nouveau-y += nouveau_nvif.o > nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o > nouveau-y += nouveau_usif.o # userspace <-> nvif > diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig > index 78631fb61adf..715cd6f4dc31 100644 > --- a/drivers/gpu/drm/nouveau/Kconfig > +++ b/drivers/gpu/drm/nouveau/Kconfig > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG > The paranoia and spam levels will add a lot of extra checks which > may potentially slow down driver operation. > > +config DRM_NOUVEAU_LED > + bool "Support for logo LED" > + depends on DRM_NOUVEAU && LEDS_CLASS > + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) > + help > + Say Y here to enabling controlling the brightness of the > + LED behind NVIDIA logo on certain Titan cards. Titan is a little too specific and inaccurate. How about high-end cards? As for the patch itself... I was going for the auto-magic approach but apparently failed at it :s Sorry... I guess what I wanted to do was to only enable the LED support if LEDS_CLASS=y. Anyway, your approach is cleaner because it makes it easy for the user to say if he/she wants to enable. I will have a closer look at it later. Thanks anyway for reporting the issue! Martin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-08 16:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-08 13:56 [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Arnd Bergmann 2016-11-08 15:46 ` Ilia Mirkin 2016-11-08 15:52 ` Arnd Bergmann 2016-11-08 16:07 ` [Nouveau] " Lukas Wunner 2016-11-08 16:12 ` Arnd Bergmann 2016-11-08 16:21 ` Karol Herbst 2016-11-08 16:35 ` Emil Velikov 2016-11-08 16:16 ` Karol Herbst 2016-11-08 16:10 ` Martin Peres
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).