[PATCH] nouveau: make backlight support non optional

[PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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__ */
-- 
2.31.1

[Nouveau] [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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__ */
-- 
2.31.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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__ */
-- 
2.31.1

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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_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

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

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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_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

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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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_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

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

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
>
> 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'
>

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
>

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
>
> 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'
>

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
>

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
>
> 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'
>

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
>

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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_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
> >
> > 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'
> >
>
> 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

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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_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
> >
> > 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'
> >
>
> 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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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_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
> >
> > 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'
> >
>
> 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

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
> > >
> > > 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'
> > >
> >
> > 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.

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
> > >
> > > 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'
> > >
> >
> > 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.

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
> > >
> > > 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'
> > >
> >
> > 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.

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
> > > >
> > > > 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'
> > > >
> > >
> > > 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.

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 :/

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
> > > >
> > > > 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'
> > > >
> > >
> > > 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.

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 :/

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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_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
> > > >
> > > > 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'
> > > >
> > >
> > > 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.

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 :/

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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.

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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.

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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 || !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.
>

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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 || !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.
>

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Karol Herbst]

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 || !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.
>

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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"

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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"
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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"

[PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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"
-- 
2.31.1

[Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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"
-- 
2.31.1

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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

Re: [Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
>

Re: [Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
>

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
>

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
> >

Re: [Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
> >

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
> >

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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

Re: [Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
>

Re: [Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
>

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Karol Herbst]

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
>

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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-8-arnd@arndb.de/

Part of the series went in (through different ways) now, but this one
never did.

      Arnd

Re: [Nouveau] [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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-8-arnd@arndb.de/

Part of the series went in (through different ways) now, but this one
never did.

      Arnd

Re: [PATCH] depend on BACKLIGHT_CLASS_DEVICE for more devices

[Arnd Bergmann]

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-8-arnd@arndb.de/

Part of the series went in (through different ways) now, but this one
never did.

      Arnd

Re: [PATCH] nouveau: make backlight support non optional

[Jani Nikula]

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 || !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"

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Jani Nikula]

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 || !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"

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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

Re: [Nouveau] [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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

Re: [PATCH] nouveau: make backlight support non optional

[Arnd Bergmann]

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