Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1517208 > unrolled thread

[PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

Back to article view | Back to linux.kernel

Contents

  [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Arnd Bergmann <arnd@arndb.de> - 2016-11-08 15:00 +0100
    Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Ilia Mirkin <imirkin@alum.mit.edu> - 2016-11-08 16:50 +0100
      Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Arnd Bergmann <arnd@arndb.de> - 2016-11-08 17:00 +0100
        Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Lukas Wunner <lukas@wunner.de> - 2016-11-08 17:10 +0100
          Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Arnd Bergmann <arnd@arndb.de> - 2016-11-08 17:20 +0100
            Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Karol Herbst <karolherbst@gmail.com> - 2016-11-08 17:30 +0100
              Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Emil Velikov <emil.l.velikov@gmail.com> - 2016-11-08 17:40 +0100
      Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Karol Herbst <karolherbst@gmail.com> - 2016-11-08 17:20 +0100
    Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration Martin Peres <martin.peres@free.fr> - 2016-11-08 17:20 +0100

#1517208 — [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

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

[toc] | [next] | [standalone]

#1517310

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

[toc] | [prev] | [next] | [standalone]

#1517316

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

[toc] | [prev] | [next] | [standalone]

#1517319 — Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

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

[toc] | [prev] | [next] | [standalone]

#1517342 — Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

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

[toc] | [prev] | [next] | [standalone]

#1517351 — Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

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

[toc] | [prev] | [next] | [standalone]

#1517369 — Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

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

[toc] | [prev] | [next] | [standalone]

#1517345 — Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

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

[toc] | [prev] | [next] | [standalone]

#1517335

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web