diff mbox series

i915: fix backlight configuration issue

Message ID 20200108140227.3976563-1-arnd@arndb.de
State New
Headers show
Series i915: fix backlight configuration issue | expand

Commit Message

Arnd Bergmann Jan. 8, 2020, 2:01 p.m. UTC 
The i915 driver can use the backlight subsystem as an option, and usually
selects it when CONFIG_ACPI is set. However it is possible to configure
a kernel with modular backlight classdev support and a built-in i915
driver, which leads to a linker error:

drivers/gpu/drm/i915/display/intel_panel.o: In function `intel_backlight_device_register':
intel_panel.c:(.text+0x2f58): undefined reference to `backlight_device_register'
drivers/gpu/drm/i915/display/intel_panel.o: In function `intel_backlight_device_unregister':
intel_panel.c:(.text+0x2fe4): undefined reference to `backlight_device_unregister'

Add another Kconfig option to ensure the driver only tries to use
the backlight support when it can in fact be linked that way. The
new option is on by default to keep the existing behavior.

This is roughly what other drivers like nouveau do as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
I've had this one lying around for a long time, it is still needed
but I am not sure which solution is best here. This version is
probably the least invasive, but it does not solve the bigger
problem around too many 'select' statements in drm
---
 drivers/gpu/drm/i915/Kconfig               | 11 ++++++++++-
 drivers/gpu/drm/i915/display/intel_panel.c |  4 ++--
 drivers/gpu/drm/i915/display/intel_panel.h |  6 +++---
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.20.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Comments

Jani Nikula Jan. 8, 2020, 3:32 p.m. UTC | #1 
On Wed, 08 Jan 2020, Arnd Bergmann <arnd@arndb.de> wrote:
> The i915 driver can use the backlight subsystem as an option, and usually

> selects it when CONFIG_ACPI is set. However it is possible to configure

> a kernel with modular backlight classdev support and a built-in i915

> driver, which leads to a linker error:

>

> drivers/gpu/drm/i915/display/intel_panel.o: In function `intel_backlight_device_register':

> intel_panel.c:(.text+0x2f58): undefined reference to `backlight_device_register'

> drivers/gpu/drm/i915/display/intel_panel.o: In function `intel_backlight_device_unregister':

> intel_panel.c:(.text+0x2fe4): undefined reference to `backlight_device_unregister'

>

> Add another Kconfig option to ensure the driver only tries to use

> the backlight support when it can in fact be linked that way. The

> new option is on by default to keep the existing behavior.

>

> This is roughly what other drivers like nouveau do as well.

>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> I've had this one lying around for a long time, it is still needed

> but I am not sure which solution is best here. This version is

> probably the least invasive, but it does not solve the bigger

> problem around too many 'select' statements in drm


This is just another hack that's only required because backlight is
selected instead of depended on throughout the kernel. (*)

i915 (and most drm drivers, with some variations) could easily handle
this with:

	depends on (ACPI && ACPI_VIDEO) || ACPI=n
	depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n

Those two lines express the allowed configurations. It's just that we
can't do that in i915 *alone*. The combinations of depends and selects
lead to impossible configurations. It's all or nothing.

I am not amused by adding more hacks, and I am really *not* interested
in adding another useless i915 config option to "solve" this issue.

So thanks, but no thanks. I'm not taking this patch.


BR,
Jani.


(*) The deeper issue is that people as well as the kconfig tools ignore
the warnings in Documentation/kbuild/kconfig-language.rst:

	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

I don't think we can, uh, fix the people, but it might be possible to
warn about selecting visible symbols or symbols with dependencies.

> ---

>  drivers/gpu/drm/i915/Kconfig               | 11 ++++++++++-

>  drivers/gpu/drm/i915/display/intel_panel.c |  4 ++--

>  drivers/gpu/drm/i915/display/intel_panel.h |  6 +++---

>  3 files changed, 15 insertions(+), 6 deletions(-)

>

> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig

> index ba9595960bbe..81d956040d18 100644

> --- a/drivers/gpu/drm/i915/Kconfig

> +++ b/drivers/gpu/drm/i915/Kconfig

> @@ -16,7 +16,7 @@ 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 BACKLIGHT_CLASS_DEVICE if ACPI

> +	select DRM_I915_BACKLIGHT if ACPI

>  	select INPUT if ACPI

>  	select ACPI_VIDEO if ACPI

>  	select ACPI_BUTTON if ACPI

> @@ -68,6 +68,15 @@ 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).

> +

>  config DRM_I915_CAPTURE_ERROR

>  	bool "Enable capturing GPU state following a hang"

>  	depends on DRM_I915

> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c

> index 7b3ec6eb3382..e2fe7a50dcbf 100644

> --- a/drivers/gpu/drm/i915/display/intel_panel.c

> +++ b/drivers/gpu/drm/i915/display/intel_panel.c

> @@ -1203,7 +1203,7 @@ void intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state,

>  	mutex_unlock(&dev_priv->backlight_lock);

>  }

>  

> -#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

> +#if IS_ENABLED(CONFIG_DRM_I915_BACKLIGHT)

>  static u32 intel_panel_get_backlight(struct intel_connector *connector)

>  {

>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);

> @@ -1370,7 +1370,7 @@ void intel_backlight_device_unregister(struct intel_connector *connector)

>  		panel->backlight.device = NULL;

>  	}

>  }

> -#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */

> +#endif /* CONFIG_DRM_I915_BACKLIGHT */

>  

>  /*

>   * CNP: PWM clock frequency is 19.2 MHz or 24 MHz.

> diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h

> index cedeea443336..e6e81268b7ed 100644

> --- a/drivers/gpu/drm/i915/display/intel_panel.h

> +++ b/drivers/gpu/drm/i915/display/intel_panel.h

> @@ -49,10 +49,10 @@ intel_panel_edid_fixed_mode(struct intel_connector *connector);

>  struct drm_display_mode *

>  intel_panel_vbt_fixed_mode(struct intel_connector *connector);

>  

> -#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

> +#if IS_ENABLED(CONFIG_DRM_I915_BACKLIGHT)

>  int intel_backlight_device_register(struct intel_connector *connector);

>  void intel_backlight_device_unregister(struct intel_connector *connector);

> -#else /* CONFIG_BACKLIGHT_CLASS_DEVICE */

> +#else /* CONFIG_DRM_I915_BACKLIGHT */

>  static inline int intel_backlight_device_register(struct intel_connector *connector)

>  {

>  	return 0;

> @@ -60,6 +60,6 @@ static inline int intel_backlight_device_register(struct intel_connector *connec

>  static inline void intel_backlight_device_unregister(struct intel_connector *connector)

>  {

>  }

> -#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */

> +#endif /* CONFIG_DRM_I915_BACKLIGHT */

>  

>  #endif /* __INTEL_PANEL_H__ */


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Jan. 8, 2020, 6:55 p.m. UTC | #2 
On Wed, Jan 08, 2020 at 05:32:26PM +0200, Jani Nikula wrote:
> On Wed, 08 Jan 2020, Arnd Bergmann <arnd@arndb.de> wrote:

> > The i915 driver can use the backlight subsystem as an option, and usually

> > selects it when CONFIG_ACPI is set. However it is possible to configure

> > a kernel with modular backlight classdev support and a built-in i915

> > driver, which leads to a linker error:

> >

> > drivers/gpu/drm/i915/display/intel_panel.o: In function `intel_backlight_device_register':

> > intel_panel.c:(.text+0x2f58): undefined reference to `backlight_device_register'

> > drivers/gpu/drm/i915/display/intel_panel.o: In function `intel_backlight_device_unregister':

> > intel_panel.c:(.text+0x2fe4): undefined reference to `backlight_device_unregister'

> >

> > Add another Kconfig option to ensure the driver only tries to use

> > the backlight support when it can in fact be linked that way. The

> > new option is on by default to keep the existing behavior.

> >

> > This is roughly what other drivers like nouveau do as well.

> >

> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> > ---

> > I've had this one lying around for a long time, it is still needed

> > but I am not sure which solution is best here. This version is

> > probably the least invasive, but it does not solve the bigger

> > problem around too many 'select' statements in drm

> 

> This is just another hack that's only required because backlight is

> selected instead of depended on throughout the kernel. (*)

> 

> i915 (and most drm drivers, with some variations) could easily handle

> this with:

> 

> 	depends on (ACPI && ACPI_VIDEO) || ACPI=n

> 	depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n

> 

> Those two lines express the allowed configurations. It's just that we

> can't do that in i915 *alone*. The combinations of depends and selects

> lead to impossible configurations. It's all or nothing.

> 

> I am not amused by adding more hacks, and I am really *not* interested

> in adding another useless i915 config option to "solve" this issue.

> 

> So thanks, but no thanks. I'm not taking this patch.


Yeah I'm also leaning towards that the real fix here is to convert
backlight over to be a depends on symbol, not a select symbol. It's
clearly not a simple stand-alone helper. Or someone makes select recursive
and adds a SAT solver to Kconfig :-)
-Daniel

> 

> 

> BR,

> Jani.

> 

> 

> (*) The deeper issue is that people as well as the kconfig tools ignore

> the warnings in Documentation/kbuild/kconfig-language.rst:

> 

> 	select should be used with care. select will force

> 	a symbol to a value without visiting the dependencies.

> 	By abusing select you are able to select a symbol FOO even

> 	if FOO depends on BAR that is not set.

> 	In general use select only for non-visible symbols

> 	(no prompts anywhere) and for symbols with no dependencies.

> 	That will limit the usefulness but on the other hand avoid

> 	the illegal configurations all over.

> 

> I don't think we can, uh, fix the people, but it might be possible to

> warn about selecting visible symbols or symbols with dependencies.

> 

> > ---

> >  drivers/gpu/drm/i915/Kconfig               | 11 ++++++++++-

> >  drivers/gpu/drm/i915/display/intel_panel.c |  4 ++--

> >  drivers/gpu/drm/i915/display/intel_panel.h |  6 +++---

> >  3 files changed, 15 insertions(+), 6 deletions(-)

> >

> > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig

> > index ba9595960bbe..81d956040d18 100644

> > --- a/drivers/gpu/drm/i915/Kconfig

> > +++ b/drivers/gpu/drm/i915/Kconfig

> > @@ -16,7 +16,7 @@ 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 BACKLIGHT_CLASS_DEVICE if ACPI

> > +	select DRM_I915_BACKLIGHT if ACPI

> >  	select INPUT if ACPI

> >  	select ACPI_VIDEO if ACPI

> >  	select ACPI_BUTTON if ACPI

> > @@ -68,6 +68,15 @@ 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).

> > +

> >  config DRM_I915_CAPTURE_ERROR

> >  	bool "Enable capturing GPU state following a hang"

> >  	depends on DRM_I915

> > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c

> > index 7b3ec6eb3382..e2fe7a50dcbf 100644

> > --- a/drivers/gpu/drm/i915/display/intel_panel.c

> > +++ b/drivers/gpu/drm/i915/display/intel_panel.c

> > @@ -1203,7 +1203,7 @@ void intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state,

> >  	mutex_unlock(&dev_priv->backlight_lock);

> >  }

> >  

> > -#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

> > +#if IS_ENABLED(CONFIG_DRM_I915_BACKLIGHT)

> >  static u32 intel_panel_get_backlight(struct intel_connector *connector)

> >  {

> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);

> > @@ -1370,7 +1370,7 @@ void intel_backlight_device_unregister(struct intel_connector *connector)

> >  		panel->backlight.device = NULL;

> >  	}

> >  }

> > -#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */

> > +#endif /* CONFIG_DRM_I915_BACKLIGHT */

> >  

> >  /*

> >   * CNP: PWM clock frequency is 19.2 MHz or 24 MHz.

> > diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h

> > index cedeea443336..e6e81268b7ed 100644

> > --- a/drivers/gpu/drm/i915/display/intel_panel.h

> > +++ b/drivers/gpu/drm/i915/display/intel_panel.h

> > @@ -49,10 +49,10 @@ intel_panel_edid_fixed_mode(struct intel_connector *connector);

> >  struct drm_display_mode *

> >  intel_panel_vbt_fixed_mode(struct intel_connector *connector);

> >  

> > -#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

> > +#if IS_ENABLED(CONFIG_DRM_I915_BACKLIGHT)

> >  int intel_backlight_device_register(struct intel_connector *connector);

> >  void intel_backlight_device_unregister(struct intel_connector *connector);

> > -#else /* CONFIG_BACKLIGHT_CLASS_DEVICE */

> > +#else /* CONFIG_DRM_I915_BACKLIGHT */

> >  static inline int intel_backlight_device_register(struct intel_connector *connector)

> >  {

> >  	return 0;

> > @@ -60,6 +60,6 @@ static inline int intel_backlight_device_register(struct intel_connector *connec

> >  static inline void intel_backlight_device_unregister(struct intel_connector *connector)

> >  {

> >  }

> > -#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */

> > +#endif /* CONFIG_DRM_I915_BACKLIGHT */

> >  

> >  #endif /* __INTEL_PANEL_H__ */

> 

> -- 

> Jani Nikula, Intel Open Source Graphics Center


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index ba9595960bbe..81d956040d18 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -16,7 +16,7 @@  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 BACKLIGHT_CLASS_DEVICE if ACPI
+	select DRM_I915_BACKLIGHT if ACPI
 	select INPUT if ACPI
 	select ACPI_VIDEO if ACPI
 	select ACPI_BUTTON if ACPI
@@ -68,6 +68,15 @@  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).
+
 config DRM_I915_CAPTURE_ERROR
 	bool "Enable capturing GPU state following a hang"
 	depends on DRM_I915
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index 7b3ec6eb3382..e2fe7a50dcbf 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -1203,7 +1203,7 @@  void intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state,
 	mutex_unlock(&dev_priv->backlight_lock);
 }
 
-#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
+#if IS_ENABLED(CONFIG_DRM_I915_BACKLIGHT)
 static u32 intel_panel_get_backlight(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1370,7 +1370,7 @@  void intel_backlight_device_unregister(struct intel_connector *connector)
 		panel->backlight.device = NULL;
 	}
 }
-#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */
+#endif /* CONFIG_DRM_I915_BACKLIGHT */
 
 /*
  * CNP: PWM clock frequency is 19.2 MHz or 24 MHz.
diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
index cedeea443336..e6e81268b7ed 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.h
+++ b/drivers/gpu/drm/i915/display/intel_panel.h
@@ -49,10 +49,10 @@  intel_panel_edid_fixed_mode(struct intel_connector *connector);
 struct drm_display_mode *
 intel_panel_vbt_fixed_mode(struct intel_connector *connector);
 
-#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
+#if IS_ENABLED(CONFIG_DRM_I915_BACKLIGHT)
 int intel_backlight_device_register(struct intel_connector *connector);
 void intel_backlight_device_unregister(struct intel_connector *connector);
-#else /* CONFIG_BACKLIGHT_CLASS_DEVICE */
+#else /* CONFIG_DRM_I915_BACKLIGHT */
 static inline int intel_backlight_device_register(struct intel_connector *connector)
 {
 	return 0;
@@ -60,6 +60,6 @@  static inline int intel_backlight_device_register(struct intel_connector *connec
 static inline void intel_backlight_device_unregister(struct intel_connector *connector)
 {
 }
-#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */
+#endif /* CONFIG_DRM_I915_BACKLIGHT */
 
 #endif /* __INTEL_PANEL_H__ */