Nouveau - Jul 2021 - [PATCH] nouveau: make backlight support non optional

On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst <kherbst at redhat.com>
wrote:
>
> On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <arnd at kernel.org>
wrote:
> >
> > On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <kherbst at
redhat.com> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 8:55 AM Arnd Bergmann <arnd at
kernel.org> wrote:
> > > >
> > > > On Sat, Jul 24, 2021 at 12:47 AM Karol Herbst <kherbst at
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 at redhat.com>
wrote:
>
> On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <arnd at
kernel.org> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <kherbst at
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 at kernel.org>
wrote:
>
> On Sat, Jul 24, 2021 at 2:52 PM Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > On Sat, Jul 24, 2021 at 2:10 PM Karol Herbst <kherbst at
redhat.com> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 1:56 PM Arnd Bergmann <arnd at
kernel.org> wrote:
> > > >
> > > > On Sat, Jul 24, 2021 at 11:55 AM Karol Herbst <kherbst at
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.
>