* Re: [Intel-gfx] [PATCH] i915: fix backlight configuration issue
@ 2020-01-08 15:32 ` Jani Nikula
0 siblings, 0 replies; 11+ messages in thread
From: Jani Nikula @ 2020-01-08 15:32 UTC (permalink / raw)
To: Arnd Bergmann, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter
Cc: Arnd Bergmann, Alexander Shiyan, intel-gfx, linux-kernel,
Krzysztof Kozlowski, dri-devel, Dave Airlie
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] i915: fix backlight configuration issue
@ 2020-01-08 15:32 ` Jani Nikula
0 siblings, 0 replies; 11+ messages in thread
From: Jani Nikula @ 2020-01-08 15:32 UTC (permalink / raw)
To: Arnd Bergmann, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter
Cc: Arnd Bergmann, Alexander Shiyan, intel-gfx, linux-kernel,
Krzysztof Kozlowski, Hans de Goede, dri-devel, Dave Airlie
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] i915: fix backlight configuration issue
2020-01-08 15:32 ` Jani Nikula
(?)
@ 2020-01-08 18:55 ` Daniel Vetter
-1 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-01-08 18:55 UTC (permalink / raw)
To: Jani Nikula
Cc: Arnd Bergmann, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Chris Wilson, Dave Airlie, Alexander Shiyan,
Krzysztof Kozlowski, Ville Syrjälä,
Maarten Lankhorst, Hans de Goede, intel-gfx, dri-devel,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-gfx] [PATCH] i915: fix backlight configuration issue
@ 2020-01-08 18:55 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-01-08 18:55 UTC (permalink / raw)
To: Jani Nikula
Cc: Alexander Shiyan, Arnd Bergmann, David Airlie, intel-gfx,
linux-kernel, Krzysztof Kozlowski, dri-devel, Dave Airlie
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] i915: fix backlight configuration issue
@ 2020-01-08 18:55 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-01-08 18:55 UTC (permalink / raw)
To: Jani Nikula
Cc: Alexander Shiyan, Arnd Bergmann, David Airlie, intel-gfx,
linux-kernel, Krzysztof Kozlowski, Hans de Goede, dri-devel,
Rodrigo Vivi, Dave Airlie
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
^ permalink raw reply [flat|nested] 11+ messages in thread