diff mbox series

[RFC,5/6] drm/rcar-du: fix selection of CMM driver

Message ID 20200408202711.1198966-6-arnd@arndb.de (mailing list archive)
State New, archived
Headers show
Series Regressions for "imply" behavior change | expand

Commit Message

Arnd Bergmann April 8, 2020, 8:27 p.m. UTC
The 'imply' statement does not seem to have an effect, as it's
still possible to turn the CMM code into a loadable module
in a randconfig build, leading to a link error:

arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':

Remove the 'imply', and instead use a silent symbol that defaults
to the correct setting.

Fixes: e08e934d6c28 ("drm: rcar-du: Add support for CMM")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/rcar-du/Kconfig | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Laurent Pinchart April 14, 2020, 8:17 p.m. UTC | #1
Hi Arnd,

Thank you for the patch.

On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> The 'imply' statement does not seem to have an effect, as it's
> still possible to turn the CMM code into a loadable module
> in a randconfig build, leading to a link error:
> 
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> 
> Remove the 'imply', and instead use a silent symbol that defaults
> to the correct setting.

This will result in the CMM always being selected when DU is, increasing
the kernel size even for devices that don't need it. I believe we need a
better construct in Kconfig to fix this.

> Fixes: e08e934d6c28 ("drm: rcar-du: Add support for CMM")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/rcar-du/Kconfig | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 0919f1f159a4..5e35f5934d62 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -4,7 +4,6 @@ config DRM_RCAR_DU
>  	depends on DRM && OF
>  	depends on ARM || ARM64
>  	depends on ARCH_RENESAS || COMPILE_TEST
> -	imply DRM_RCAR_CMM
>  	imply DRM_RCAR_LVDS
>  	select DRM_KMS_HELPER
>  	select DRM_KMS_CMA_HELPER
> @@ -15,9 +14,8 @@ config DRM_RCAR_DU
>  	  If M is selected the module will be called rcar-du-drm.
>  
>  config DRM_RCAR_CMM
> -	tristate "R-Car DU Color Management Module (CMM) Support"
> +	def_tristate DRM_RCAR_DU
>  	depends on DRM && OF
> -	depends on DRM_RCAR_DU
>  	help
>  	  Enable support for R-Car Color Management Module (CMM).
>
Arnd Bergmann April 14, 2020, 8:38 p.m. UTC | #2
On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Arnd,
>
> Thank you for the patch.
>
> On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > The 'imply' statement does not seem to have an effect, as it's
> > still possible to turn the CMM code into a loadable module
> > in a randconfig build, leading to a link error:
> >
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> >
> > Remove the 'imply', and instead use a silent symbol that defaults
> > to the correct setting.
>
> This will result in the CMM always being selected when DU is, increasing
> the kernel size even for devices that don't need it. I believe we need a
> better construct in Kconfig to fix this.

I had expected this to have the same meaning that we had before the
Kconfig change: whenever the dependencies are available, turn it on,
otherwise leave it disabled.

Can you describe what behavior you actually want instead?
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> >       depends on DRM && OF
> >       depends on ARM || ARM64
> >       depends on ARCH_RENESAS || COMPILE_TEST
> > -     imply DRM_RCAR_CMM
> >       imply DRM_RCAR_LVDS
> >       select DRM_KMS_HELPER
> >       select DRM_KMS_CMA_HELPER
> > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> >         If M is selected the module will be called rcar-du-drm.
> >
> >  config DRM_RCAR_CMM
> > -     tristate "R-Car DU Color Management Module (CMM) Support"
> > +     def_tristate DRM_RCAR_DU
> >       depends on DRM && OF
> > -     depends on DRM_RCAR_DU
> >       help

It would be easy enough to make this a visible 'bool' symbol and
build it into the rcu-du-drm.ko module itself. Would that help you?

       Arnd
Laurent Pinchart April 14, 2020, 8:51 p.m. UTC | #3
Hi Arnd,

On Tue, Apr 14, 2020 at 10:38:27PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart wrote:
> > On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > > The 'imply' statement does not seem to have an effect, as it's
> > > still possible to turn the CMM code into a loadable module
> > > in a randconfig build, leading to a link error:
> > >
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> > >
> > > Remove the 'imply', and instead use a silent symbol that defaults
> > > to the correct setting.
> >
> > This will result in the CMM always being selected when DU is, increasing
> > the kernel size even for devices that don't need it. I believe we need a
> > better construct in Kconfig to fix this.
> 
> I had expected this to have the same meaning that we had before the
> Kconfig change: whenever the dependencies are available, turn it on,
> otherwise leave it disabled.
> 
> Can you describe what behavior you actually want instead?

Doesn't "imply" mean it gets selected by default but can be manually
disabled ?

> > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > >       depends on DRM && OF
> > >       depends on ARM || ARM64
> > >       depends on ARCH_RENESAS || COMPILE_TEST
> > > -     imply DRM_RCAR_CMM
> > >       imply DRM_RCAR_LVDS
> > >       select DRM_KMS_HELPER
> > >       select DRM_KMS_CMA_HELPER
> > > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > >         If M is selected the module will be called rcar-du-drm.
> > >
> > >  config DRM_RCAR_CMM
> > > -     tristate "R-Car DU Color Management Module (CMM) Support"
> > > +     def_tristate DRM_RCAR_DU
> > >       depends on DRM && OF
> > > -     depends on DRM_RCAR_DU
> > >       help
> 
> It would be easy enough to make this a visible 'bool' symbol and
> build it into the rcu-du-drm.ko module itself. Would that help you?

That could indeed simplify a few things. I wonder if it could introduce
a few small issues though (but likely nothing we can't fix). The two
that come to mind are the fact that the module would have two
MODULE_DESCRIPTION and MODULE_LICENSE entries (I have no idea if that
could cause breakages), and that it could make module unloading more
difficult as the CMM being used by the DU would increase the refcount on
the module. I think the latter could be worked around by manually
unbinding the DU device through sysfs before unloading the module (and I
can't say for sure that unloading the DU module is not broken today
*innocent and naive look* :-)).
Arnd Bergmann April 14, 2020, 9:10 p.m. UTC | #4
On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Arnd,
>
> On Tue, Apr 14, 2020 at 10:38:27PM +0200, Arnd Bergmann wrote:
> > On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart wrote:
> > > On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > > > The 'imply' statement does not seem to have an effect, as it's
> > > > still possible to turn the CMM code into a loadable module
> > > > in a randconfig build, leading to a link error:
> > > >
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > > > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > > > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > > > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> > > >
> > > > Remove the 'imply', and instead use a silent symbol that defaults
> > > > to the correct setting.
> > >
> > > This will result in the CMM always being selected when DU is, increasing
> > > the kernel size even for devices that don't need it. I believe we need a
> > > better construct in Kconfig to fix this.
> >
> > I had expected this to have the same meaning that we had before the
> > Kconfig change: whenever the dependencies are available, turn it on,
> > otherwise leave it disabled.
> >
> > Can you describe what behavior you actually want instead?
>
> Doesn't "imply" mean it gets selected by default but can be manually
> disabled ?

That may be what it means now (I still don't understand how it's defined
as of v5.7-rc1), but traditionally it was more like a 'select if all
dependencies
are met'.

> > > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > > >       depends on DRM && OF
> > > >       depends on ARM || ARM64
> > > >       depends on ARCH_RENESAS || COMPILE_TEST
> > > > -     imply DRM_RCAR_CMM
> > > >       imply DRM_RCAR_LVDS
> > > >       select DRM_KMS_HELPER
> > > >       select DRM_KMS_CMA_HELPER
> > > > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > > >         If M is selected the module will be called rcar-du-drm.
> > > >
> > > >  config DRM_RCAR_CMM
> > > > -     tristate "R-Car DU Color Management Module (CMM) Support"
> > > > +     def_tristate DRM_RCAR_DU
> > > >       depends on DRM && OF
> > > > -     depends on DRM_RCAR_DU
> > > >       help
> >
> > It would be easy enough to make this a visible 'bool' symbol and
> > build it into the rcu-du-drm.ko module itself. Would that help you?
>
> That could indeed simplify a few things. I wonder if it could introduce
> a few small issues though (but likely nothing we can't fix). The two
> that come to mind are the fact that the module would have two
> MODULE_DESCRIPTION and MODULE_LICENSE entries (I have no idea if that
> could cause breakages), and that it could make module unloading more
> difficult as the CMM being used by the DU would increase the refcount on
> the module. I think the latter could be worked around by manually
> unbinding the DU device through sysfs before unloading the module (and I
> can't say for sure that unloading the DU module is not broken today
> *innocent and naive look* :-)).

In that case, a Makefile trick could also work, doing

ifdef CONFIG_DRM_RCAR_CMM
obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o
endif

Thereby making the cmm module have the same state (y or m) as
the du module whenever the option is enabled.

       Arnd
Geert Uytterhoeven April 15, 2020, 2:13 p.m. UTC | #5
Hi Arnd,

On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Tue, Apr 14, 2020 at 10:38:27PM +0200, Arnd Bergmann wrote:
> > > On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart wrote:
> > > > On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > > > > The 'imply' statement does not seem to have an effect, as it's
> > > > > still possible to turn the CMM code into a loadable module
> > > > > in a randconfig build, leading to a link error:
> > > > >
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > > > > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > > > > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > > > > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> > > > >
> > > > > Remove the 'imply', and instead use a silent symbol that defaults
> > > > > to the correct setting.
> > > >
> > > > This will result in the CMM always being selected when DU is, increasing
> > > > the kernel size even for devices that don't need it. I believe we need a
> > > > better construct in Kconfig to fix this.
> > >
> > > I had expected this to have the same meaning that we had before the
> > > Kconfig change: whenever the dependencies are available, turn it on,
> > > otherwise leave it disabled.
> > >
> > > Can you describe what behavior you actually want instead?
> >
> > Doesn't "imply" mean it gets selected by default but can be manually
> > disabled ?
>
> That may be what it means now (I still don't understand how it's defined
> as of v5.7-rc1), but traditionally it was more like a 'select if all
> dependencies are met'.

That's still what it is supposed to mean right now ;-)
Except that now it should correctly handle the modular case, too.

> > > > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > > > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > > > >       depends on DRM && OF
> > > > >       depends on ARM || ARM64
> > > > >       depends on ARCH_RENESAS || COMPILE_TEST
> > > > > -     imply DRM_RCAR_CMM
> > > > >       imply DRM_RCAR_LVDS
> > > > >       select DRM_KMS_HELPER
> > > > >       select DRM_KMS_CMA_HELPER
> > > > > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > > > >         If M is selected the module will be called rcar-du-drm.
> > > > >
> > > > >  config DRM_RCAR_CMM
> > > > > -     tristate "R-Car DU Color Management Module (CMM) Support"
> > > > > +     def_tristate DRM_RCAR_DU
> > > > >       depends on DRM && OF
> > > > > -     depends on DRM_RCAR_DU
> > > > >       help
> > >
> > > It would be easy enough to make this a visible 'bool' symbol and
> > > build it into the rcu-du-drm.ko module itself. Would that help you?
> >
> > That could indeed simplify a few things. I wonder if it could introduce
> > a few small issues though (but likely nothing we can't fix). The two
> > that come to mind are the fact that the module would have two
> > MODULE_DESCRIPTION and MODULE_LICENSE entries (I have no idea if that
> > could cause breakages), and that it could make module unloading more
> > difficult as the CMM being used by the DU would increase the refcount on
> > the module. I think the latter could be worked around by manually
> > unbinding the DU device through sysfs before unloading the module (and I
> > can't say for sure that unloading the DU module is not broken today
> > *innocent and naive look* :-)).
>
> In that case, a Makefile trick could also work, doing
>
> ifdef CONFIG_DRM_RCAR_CMM
> obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o
> endif
>
> Thereby making the cmm module have the same state (y or m) as
> the du module whenever the option is enabled.

What about dropping the "imply DRM_RCAR_CMM", but defaulting to
enable CMM if DU is enabled?

    config DRM_RCAR_CMM
            tristate "R-Car DU Color Management Module (CMM) Support"
            depends on DRM_RCAR_DU && OF
            default DRM_RCAR_DU

Gr{oetje,eeting}s,

                        Geert
Arnd Bergmann April 15, 2020, 3:18 p.m. UTC | #6
On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > Doesn't "imply" mean it gets selected by default but can be manually
> > > disabled ?
> >
> > That may be what it means now (I still don't understand how it's defined
> > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > dependencies are met'.
>
> That's still what it is supposed to mean right now ;-)
> Except that now it should correctly handle the modular case, too.

Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
and enable CONFIG_DRM_RCAR_DU, I can set
DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
statement seems to be ignored entirely, except as reverse 'default'
setting.

> >
> > In that case, a Makefile trick could also work, doing
> >
> > ifdef CONFIG_DRM_RCAR_CMM
> > obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o
> > endif
> >
> > Thereby making the cmm module have the same state (y or m) as
> > the du module whenever the option is enabled.
>
> What about dropping the "imply DRM_RCAR_CMM", but defaulting to
> enable CMM if DU is enabled?
>
>     config DRM_RCAR_CMM
>             tristate "R-Car DU Color Management Module (CMM) Support"
>             depends on DRM_RCAR_DU && OF
>             default DRM_RCAR_DU

That doesn't work because it allows DRM_RCAR_DU=y with
DRM_RCAR_CMM=m, which causes a link failure.

         Arnd
Arnd Bergmann April 15, 2020, 7:07 p.m. UTC | #7
On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > disabled ?
> > >
> > > That may be what it means now (I still don't understand how it's defined
> > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > dependencies are met'.
> >
> > That's still what it is supposed to mean right now ;-)
> > Except that now it should correctly handle the modular case, too.
>
> Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> and enable CONFIG_DRM_RCAR_DU, I can set
> DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> statement seems to be ignored entirely, except as reverse 'default'
> setting.

Here is another version that should do what we want and is only
half-ugly. I can send that as a proper patch if it passes my testing
and nobody hates it too much.

       Arnd

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 0919f1f159a4..d2fcec807dfa 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -4,8 +4,6 @@ config DRM_RCAR_DU
        depends on DRM && OF
        depends on ARM || ARM64
        depends on ARCH_RENESAS || COMPILE_TEST
-       imply DRM_RCAR_CMM
-       imply DRM_RCAR_LVDS
        select DRM_KMS_HELPER
        select DRM_KMS_CMA_HELPER
        select DRM_GEM_CMA_HELPER
@@ -14,13 +12,17 @@ config DRM_RCAR_DU
          Choose this option if you have an R-Car chipset.
          If M is selected the module will be called rcar-du-drm.

-config DRM_RCAR_CMM
-       tristate "R-Car DU Color Management Module (CMM) Support"
-       depends on DRM && OF
+config DRM_RCAR_USE_CMM
+       bool "R-Car DU Color Management Module (CMM) Support"
        depends on DRM_RCAR_DU
+       default DRM_RCAR_DU
        help
          Enable support for R-Car Color Management Module (CMM).

+config DRM_RCAR_CMM
+       def_tristate DRM_RCAR_DU
+       depends on DRM_RCAR_USE_CMM
+
 config DRM_RCAR_DW_HDMI
        tristate "R-Car DU Gen3 HDMI Encoder Support"
        depends on DRM && OF
@@ -28,15 +30,20 @@ config DRM_RCAR_DW_HDMI
        help
          Enable support for R-Car Gen3 internal HDMI encoder.

-config DRM_RCAR_LVDS
-       tristate "R-Car DU LVDS Encoder Support"
-       depends on DRM && DRM_BRIDGE && OF
+config DRM_RCAR_USE_LVDS
+       bool "R-Car DU LVDS Encoder Support"
+       depends on DRM_BRIDGE && OF
+       default DRM_RCAR_DU
        select DRM_PANEL
        select OF_FLATTREE
        select OF_OVERLAY
        help
          Enable support for the R-Car Display Unit embedded LVDS encoders.

+config DRM_RCAR_LVDS
+       def_tristate DRM_RCAR_DU
+       depends on DRM_RCAR_USE_LVDS
+
 config DRM_RCAR_VSP
        bool "R-Car DU VSP Compositor Support" if ARM
        default y if ARM64
Laurent Pinchart April 15, 2020, 9:12 p.m. UTC | #8
Hi Arnd,

On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > disabled ?
> > > >
> > > > That may be what it means now (I still don't understand how it's defined
> > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > dependencies are met'.
> > >
> > > That's still what it is supposed to mean right now ;-)
> > > Except that now it should correctly handle the modular case, too.
> >
> > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > and enable CONFIG_DRM_RCAR_DU, I can set
> > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > statement seems to be ignored entirely, except as reverse 'default'
> > setting.
> 
> Here is another version that should do what we want and is only
> half-ugly. I can send that as a proper patch if it passes my testing
> and nobody hates it too much.

This may be a stupid question, but doesn't this really call for fixing
Kconfig ? This seems to be such a common pattern that requiring
constructs similar to the ones below will be a never-ending chase of
offenders.

> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 0919f1f159a4..d2fcec807dfa 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -4,8 +4,6 @@ config DRM_RCAR_DU
>         depends on DRM && OF
>         depends on ARM || ARM64
>         depends on ARCH_RENESAS || COMPILE_TEST
> -       imply DRM_RCAR_CMM
> -       imply DRM_RCAR_LVDS
>         select DRM_KMS_HELPER
>         select DRM_KMS_CMA_HELPER
>         select DRM_GEM_CMA_HELPER
> @@ -14,13 +12,17 @@ config DRM_RCAR_DU
>           Choose this option if you have an R-Car chipset.
>           If M is selected the module will be called rcar-du-drm.
> 
> -config DRM_RCAR_CMM
> -       tristate "R-Car DU Color Management Module (CMM) Support"
> -       depends on DRM && OF
> +config DRM_RCAR_USE_CMM
> +       bool "R-Car DU Color Management Module (CMM) Support"
>         depends on DRM_RCAR_DU
> +       default DRM_RCAR_DU
>         help
>           Enable support for R-Car Color Management Module (CMM).
> 
> +config DRM_RCAR_CMM
> +       def_tristate DRM_RCAR_DU
> +       depends on DRM_RCAR_USE_CMM
> +
>  config DRM_RCAR_DW_HDMI
>         tristate "R-Car DU Gen3 HDMI Encoder Support"
>         depends on DRM && OF
> @@ -28,15 +30,20 @@ config DRM_RCAR_DW_HDMI
>         help
>           Enable support for R-Car Gen3 internal HDMI encoder.
> 
> -config DRM_RCAR_LVDS
> -       tristate "R-Car DU LVDS Encoder Support"
> -       depends on DRM && DRM_BRIDGE && OF
> +config DRM_RCAR_USE_LVDS
> +       bool "R-Car DU LVDS Encoder Support"
> +       depends on DRM_BRIDGE && OF
> +       default DRM_RCAR_DU
>         select DRM_PANEL
>         select OF_FLATTREE
>         select OF_OVERLAY
>         help
>           Enable support for the R-Car Display Unit embedded LVDS encoders.
> 
> +config DRM_RCAR_LVDS
> +       def_tristate DRM_RCAR_DU
> +       depends on DRM_RCAR_USE_LVDS
> +
>  config DRM_RCAR_VSP
>         bool "R-Car DU VSP Compositor Support" if ARM
>         default y if ARM64
Arnd Bergmann April 15, 2020, 9:22 p.m. UTC | #9
On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> > On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > > disabled ?
> > > > >
> > > > > That may be what it means now (I still don't understand how it's defined
> > > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > > dependencies are met'.
> > > >
> > > > That's still what it is supposed to mean right now ;-)
> > > > Except that now it should correctly handle the modular case, too.
> > >
> > > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > > and enable CONFIG_DRM_RCAR_DU, I can set
> > > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > > statement seems to be ignored entirely, except as reverse 'default'
> > > setting.
> >
> > Here is another version that should do what we want and is only
> > half-ugly. I can send that as a proper patch if it passes my testing
> > and nobody hates it too much.
>
> This may be a stupid question, but doesn't this really call for fixing
> Kconfig ? This seems to be such a common pattern that requiring
> constructs similar to the ones below will be a never-ending chase of
> offenders.

Maybe, I suppose the hardest part here would be to come up with
an appropriate name for the keyword ;-)

Any suggestions?

This specific issue is fairly rare though, in most cases the dependencies
are in the right order so a Kconfig symbol 'depends on' a second one
when the corresponding loadable module uses symbols from that second
module. The problem here is that the two are mixed up.

The much more common problem is the one where one needs to
wrong

config FOO
       depends on BAR || !BAR

To ensure the dependency is either met or BAR is disabled, but
not FOO=y with BAR=m. If you have any suggestions for a keyword
for that thing, we can clean up hundreds of such instances.

        Arnd
Daniel Vetter April 16, 2020, 6:51 a.m. UTC | #10
On Wed, Apr 15, 2020 at 11:22 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> > > On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > > > disabled ?
> > > > > >
> > > > > > That may be what it means now (I still don't understand how it's defined
> > > > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > > > dependencies are met'.
> > > > >
> > > > > That's still what it is supposed to mean right now ;-)
> > > > > Except that now it should correctly handle the modular case, too.
> > > >
> > > > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > > > and enable CONFIG_DRM_RCAR_DU, I can set
> > > > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > > > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > > > statement seems to be ignored entirely, except as reverse 'default'
> > > > setting.
> > >
> > > Here is another version that should do what we want and is only
> > > half-ugly. I can send that as a proper patch if it passes my testing
> > > and nobody hates it too much.
> >
> > This may be a stupid question, but doesn't this really call for fixing
> > Kconfig ? This seems to be such a common pattern that requiring
> > constructs similar to the ones below will be a never-ending chase of
> > offenders.
>
> Maybe, I suppose the hardest part here would be to come up with
> an appropriate name for the keyword ;-)
>
> Any suggestions?
>
> This specific issue is fairly rare though, in most cases the dependencies
> are in the right order so a Kconfig symbol 'depends on' a second one
> when the corresponding loadable module uses symbols from that second
> module. The problem here is that the two are mixed up.
>
> The much more common problem is the one where one needs to
> wrong
>
> config FOO
>        depends on BAR || !BAR
>
> To ensure the dependency is either met or BAR is disabled, but
> not FOO=y with BAR=m. If you have any suggestions for a keyword
> for that thing, we can clean up hundreds of such instances.

Some ideas:

config FOO
    can use  BAR
    maybe BAR
    optional BAR

We should probably double-check that this is only ever used for when
both FOO and BAR are tri-state, since without that it doesn't make
much sense.
-Daniel
Laurent Pinchart April 16, 2020, 3:17 p.m. UTC | #11
On Thu, Apr 16, 2020 at 08:51:14AM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 11:22 PM Arnd Bergmann wrote:
> > On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart wrote:
> > > On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> > > > On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > > > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > > > > disabled ?
> > > > > > >
> > > > > > > That may be what it means now (I still don't understand how it's defined
> > > > > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > > > > dependencies are met'.
> > > > > >
> > > > > > That's still what it is supposed to mean right now ;-)
> > > > > > Except that now it should correctly handle the modular case, too.
> > > > >
> > > > > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > > > > and enable CONFIG_DRM_RCAR_DU, I can set
> > > > > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > > > > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > > > > statement seems to be ignored entirely, except as reverse 'default'
> > > > > setting.
> > > >
> > > > Here is another version that should do what we want and is only
> > > > half-ugly. I can send that as a proper patch if it passes my testing
> > > > and nobody hates it too much.
> > >
> > > This may be a stupid question, but doesn't this really call for fixing
> > > Kconfig ? This seems to be such a common pattern that requiring
> > > constructs similar to the ones below will be a never-ending chase of
> > > offenders.
> >
> > Maybe, I suppose the hardest part here would be to come up with
> > an appropriate name for the keyword ;-)
> >
> > Any suggestions?

Would it make sense to fix the imply semantics ? Or are they use cases
for the current behaviour of imply ? "recommend" could be another
keyword. I think we should try to limit the number of keywords though,
as it would otherwise become quite messy.

> > This specific issue is fairly rare though, in most cases the dependencies
> > are in the right order so a Kconfig symbol 'depends on' a second one
> > when the corresponding loadable module uses symbols from that second
> > module. The problem here is that the two are mixed up.
> >
> > The much more common problem is the one where one needs to
> > wrong
> >
> > config FOO
> >        depends on BAR || !BAR
> >
> > To ensure the dependency is either met or BAR is disabled, but
> > not FOO=y with BAR=m. If you have any suggestions for a keyword
> > for that thing, we can clean up hundreds of such instances.
> 
> Some ideas:
> 
> config FOO
>     can use  BAR
>     maybe BAR
>     optional BAR

Another idea,

	depends optionally on BAR

> We should probably double-check that this is only ever used for when
> both FOO and BAR are tri-state, since without that it doesn't make
> much sense.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 0919f1f159a4..5e35f5934d62 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -4,7 +4,6 @@  config DRM_RCAR_DU
 	depends on DRM && OF
 	depends on ARM || ARM64
 	depends on ARCH_RENESAS || COMPILE_TEST
-	imply DRM_RCAR_CMM
 	imply DRM_RCAR_LVDS
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER
@@ -15,9 +14,8 @@  config DRM_RCAR_DU
 	  If M is selected the module will be called rcar-du-drm.
 
 config DRM_RCAR_CMM
-	tristate "R-Car DU Color Management Module (CMM) Support"
+	def_tristate DRM_RCAR_DU
 	depends on DRM && OF
-	depends on DRM_RCAR_DU
 	help
 	  Enable support for R-Car Color Management Module (CMM).