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 <[email protected]>
Cc: Lyude Paul <[email protected]>
Cc: Ben Skeggs <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD backlight support for nouveau")
Signed-off-by: Karol Herbst <[email protected]>
---
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
On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst <[email protected]> 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
On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst <[email protected]> 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
>
On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann <[email protected]> wrote:
> >
> > On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst <[email protected]> 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
On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <[email protected]> wrote:
> >
> > On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst <[email protected]> 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.
On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <[email protected]> wrote:
> >
> > On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <[email protected]> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann <[email protected]> wrote:
> > > >
> > > > On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst <[email protected]> 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 :/
On Sat, Jul 24, 2021 at 2:52 PM Karol Herbst <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst <[email protected]> wrote:
> >
> > On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <[email protected]> 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.
On Sat, Jul 24, 2021 at 4:05 PM Arnd Bergmann <[email protected]> wrote:
>
> On Sat, Jul 24, 2021 at 2:52 PM Karol Herbst <[email protected]> wrote:
> >
> > On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst <[email protected]> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <[email protected]> wrote:
> > > >
> > > > On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <[email protected]> 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.
>
On Sat, Jul 24, 2021 at 4:14 PM Karol Herbst <[email protected]> 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"
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 <[email protected]>
Cc: Lyude Paul <[email protected]>
Cc: Ben Skeggs <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
On Wed, Aug 4, 2021 at 8:59 PM Karol Herbst <[email protected]> wrote:
> On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst <[email protected]> wrote:
> > On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann <[email protected]> wrote:
> > > On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst <[email protected]> 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
On Wed, Aug 4, 2021 at 11:10 PM Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Aug 4, 2021 at 8:59 PM Karol Herbst <[email protected]> wrote:
> > On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst <[email protected]> wrote:
> > > On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann <[email protected]> wrote:
> > > > On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst <[email protected]> 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
>
On Thu, Aug 5, 2021 at 12:01 AM Karol Herbst <[email protected]> wrote:
>
> On Wed, Aug 4, 2021 at 11:10 PM Arnd Bergmann <[email protected]> wrote:
> >
> > On Wed, Aug 4, 2021 at 8:59 PM Karol Herbst <[email protected]> wrote:
> > > On Wed, Aug 4, 2021 at 4:43 PM Karol Herbst <[email protected]> wrote:
> > > > On Wed, Aug 4, 2021 at 4:19 PM Arnd Bergmann <[email protected]> wrote:
> > > > > On Wed, Aug 4, 2021 at 4:10 PM Karol Herbst <[email protected]> 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/[email protected]/
Part of the series went in (through different ways) now, but this one
never did.
Arnd
On Sat, 24 Jul 2021, Arnd Bergmann <[email protected]> wrote:
> On Sat, Jul 24, 2021 at 4:14 PM Karol Herbst <[email protected]> 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
On Mon, Aug 9, 2021 at 3:20 PM Jani Nikula <[email protected]> wrote:
>
> On Sat, 24 Jul 2021, Arnd Bergmann <[email protected]> wrote:
> > On Sat, Jul 24, 2021 at 4:14 PM Karol Herbst <[email protected]> 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