| Message ID | 20210723224617.3088886-1-kherbst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nouveau: make backlight support non optional | expand |
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
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 >
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
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.
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 :/
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.
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. >
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"
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"
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
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__ */
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(-)