| Message ID | 20200408202711.1198966-6-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Regressions for "imply" behavior change | expand |
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). >
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
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* :-)).
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
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
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
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
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
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
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
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 --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).
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(-)