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

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'
>
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 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

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.