[PATCH 0/8] drm, fbdev: rework dependencies

[PATCH 0/8] drm, fbdev: rework dependencies

[Arnd Bergmann]

I tried to fix up some dependencies after the sii8620 "imply EXTCON"
statementn broke, trying a few things but in the backing out a
change that would completely reverse the LEDS_CLASS selects into
a 'depends on'. 

However, what I got now are multiple changes that remove gratious
"selects" that lead to circular dependencies for sii8620 and others:

- Anything doing "select FB" is now gone, or becomes "depends on FB",

- DDC support depends on I2C instead of selecting it

- backlight class device support is never selected by framebuffer
  drivers but has proper dependencies

I have done thousands of randconfig build tests on this, but no
runtime tests.

Some of the 'depends on FOO || !FOO' statements could be simplified
into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
but I would for the moment treat that as a cleanup that can be done
later.

If we can agree on these changes, maybe someone can merge them
through the drm-misc tree.

Please review

       Arnd

Arnd Bergmann (8):
  fbdev: w100fb: clean up mach-pxa compile-time dependency
  fbdev/ARM: pxa: avoid selecting CONFIG_FB
  fbdev: rework FB_DDC dependencies
  drm/rcar: stop using 'imply' for dependencies
  drm/vmwgfx: make framebuffer support optional
  drm: decouple from CONFIG_FB
  fbdev: rework backlight dependencies
  drm/bridge/sii8620: fix extcon dependency

 arch/arm/configs/pxa_defconfig      |  3 ++
 arch/arm/mach-pxa/Kconfig           |  7 ---
 arch/arm/mach-pxa/eseries.c         | 14 +----
 arch/arm/mach-pxa/saar.c            |  2 +-
 arch/arm/mach-pxa/tavorevb.c        |  2 +-
 drivers/auxdisplay/Kconfig          |  1 +
 drivers/gpu/drm/Kconfig             |  5 +-
 drivers/gpu/drm/bridge/Kconfig      |  2 +-
 drivers/gpu/drm/mxsfb/Kconfig       |  1 -
 drivers/gpu/drm/rcar-du/Kconfig     | 23 +++++---
 drivers/gpu/drm/vmwgfx/Kconfig      | 17 +++---
 drivers/gpu/drm/vmwgfx/Makefile     |  4 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 35 +++++++-----
 drivers/gpu/drm/zte/Kconfig         |  1 -
 drivers/macintosh/Kconfig           |  1 +
 drivers/staging/fbtft/Kconfig       |  1 +
 drivers/staging/olpc_dcon/Kconfig   |  2 +-
 drivers/video/fbdev/Kconfig         | 31 ++++++++---
 drivers/video/fbdev/w100fb.c        | 84 +++++------------------------
 include/video/w100fb.h              |  6 +--
 20 files changed, 101 insertions(+), 141 deletions(-)

-- 
2.26.0

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: <masahiroy@kernel.org>
Cc: <Laurent.pinchart@ideasonboard.com>
Cc: <linux-renesas-soc@vger.kernel.org>,
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
Cc: <kieran.bingham+renesas@ideasonboard.com>,
Cc: <airlied@linux.ie>
Cc: daniel@zonque.org
Cc: haojian.zhuang@gmail.com
Cc: robert.jarzmik@free.fr
Cc: daniel@ffwll.ch
Cc: marex@denx.de
Cc: stefan@agner.ch
Cc: linux-graphics-maintainer@vmware.com
Cc: thellstrom@vmware.com
Cc: jfrederich@gmail.com
Cc: dsd@laptop.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Cc: geert@linux-m68k.org

[PATCH 1/8] fbdev: w100fb: clean up mach-pxa compile-time dependency

[Arnd Bergmann]

In order to get rid of the 'select FB' line in Kconfig for all platforms,
change the w100fb driver to handle the gpio lines itself rather than
using an indirect function call.

This also allows removing the remaining exported interfaces from the
driver, the only other symbol has been unused for a long time.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/configs/pxa_defconfig |  2 +
 arch/arm/mach-pxa/Kconfig      |  3 --
 arch/arm/mach-pxa/eseries.c    | 14 +-----
 drivers/video/fbdev/w100fb.c   | 84 +++++-----------------------------
 include/video/w100fb.h         |  6 +--
 5 files changed, 16 insertions(+), 93 deletions(-)

diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
index b817c57f05f1..8df88e8a3048 100644
--- a/arch/arm/configs/pxa_defconfig
+++ b/arch/arm/configs/pxa_defconfig
@@ -454,6 +454,8 @@ CONFIG_V4L_MEM2MEM_DRIVERS=y
 CONFIG_SOC_CAMERA_MT9M111=m
 CONFIG_DRM=m
 CONFIG_FIRMWARE_EDID=y
+CONFIG_FB=y
+CONFIG_FB_W100=y
 CONFIG_FB_TILEBLITTING=y
 CONFIG_FB_PXA_OVERLAY=y
 CONFIG_FB_PXA_PARAMETERS=y
diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig
index f60bc29aef68..8444d40df1b3 100644
--- a/arch/arm/mach-pxa/Kconfig
+++ b/arch/arm/mach-pxa/Kconfig
@@ -549,9 +549,6 @@ config MACH_ICONTROL
 
 config ARCH_PXA_ESERIES
 	bool "PXA based Toshiba e-series PDAs"
-	select FB_W100
-	select FB
-	select PXA25x
 
 config MACH_E330
 	bool "Toshiba e330"
diff --git a/arch/arm/mach-pxa/eseries.c b/arch/arm/mach-pxa/eseries.c
index f37c44b6139d..023ee901a7eb 100644
--- a/arch/arm/mach-pxa/eseries.c
+++ b/arch/arm/mach-pxa/eseries.c
@@ -849,20 +849,8 @@ static struct w100_mem_info e800_w100_mem_info = {
 	.size            = 0x1fffff,
 };
 
-static void e800_tg_change(struct w100fb_par *par)
-{
-	unsigned long tmp;
-
-	tmp = w100fb_gpio_read(W100_GPIO_PORT_A);
-	if (par->mode->xres = 480)
-		tmp |= 0x100;
-	else
-		tmp &= ~0x100;
-	w100fb_gpio_write(W100_GPIO_PORT_A, tmp);
-}
-
 static struct w100_tg_info e800_tg_info = {
-	.change = e800_tg_change,
+	.change_e800 = true,
 };
 
 static struct w100fb_mach_info e800_fb_info = {
diff --git a/drivers/video/fbdev/w100fb.c b/drivers/video/fbdev/w100fb.c
index 2d6e2738b792..512c4d5a73a3 100644
--- a/drivers/video/fbdev/w100fb.c
+++ b/drivers/video/fbdev/w100fb.c
@@ -48,7 +48,6 @@ static void w100_init_lcd(struct w100fb_par*);
 static void w100_set_dispregs(struct w100fb_par*);
 static void w100_update_enable(void);
 static void w100_update_disable(void);
-static void calc_hsync(struct w100fb_par *par);
 static void w100_init_graphic_engine(struct w100fb_par *par);
 struct w100_pll_info *w100_get_xtal_table(unsigned int freq);
 
@@ -101,8 +100,6 @@ static ssize_t flip_store(struct device *dev, struct device_attribute *attr, con
 	w100_set_dispregs(par);
 	w100_update_enable();
 
-	calc_hsync(par);
-
 	return count;
 }
 
@@ -157,7 +154,6 @@ static ssize_t fastpllclk_store(struct device *dev, struct device_attribute *att
 	}
 
 	w100_init_clocks(par);
-	calc_hsync(par);
 
 	return count;
 }
@@ -173,23 +169,6 @@ static struct attribute *w100fb_attrs[] = {
 };
 ATTRIBUTE_GROUPS(w100fb);
 
-/*
- * Some touchscreens need hsync information from the video driver to
- * function correctly. We export it here.
- */
-unsigned long w100fb_get_hsynclen(struct device *dev)
-{
-	struct fb_info *info = dev_get_drvdata(dev);
-	struct w100fb_par *par=info->par;
-
-	/* If display is blanked/suspended, hsync isn't active */
-	if (par->blanked)
-		return 0;
-	else
-		return par->hsync_len;
-}
-EXPORT_SYMBOL(w100fb_get_hsynclen);
-
 static void w100fb_clear_screen(struct w100fb_par *par)
 {
 	memset_io(remapped_fbuf + (W100_FB_BASE-MEM_WINDOW_BASE), 0, (par->xres * par->yres * BITS_PER_PIXEL/8));
@@ -405,6 +384,15 @@ static void w100fb_copyarea(struct fb_info *info,
 	writel((w << 16) | (h & 0xffff), remapped_regs + mmDST_WIDTH_HEIGHT);
 }
 
+static void w100fb_e800_tg_change(struct w100fb_par *par)
+{
+	u32 tmp = readl(remapped_regs + mmGPIO_DATA);
+	if (par->mode->xres = 480)
+		tmp |= 0x100;
+	else
+		tmp &= ~0x100;
+	writel(tmp, remapped_regs + mmGPIO_DATA);
+}
 
 /*
  *  Change the resolution by calling the appropriate hardware functions
@@ -425,10 +413,9 @@ static void w100fb_activate_var(struct w100fb_par *par)
 	w100_update_enable();
 	w100_init_graphic_engine(par);
 
-	calc_hsync(par);
-
-	if (!par->blanked && tg && tg->change)
-		tg->change(par);
+	if (IS_ENABLED(CONFIG_ARCH_PXA_ESERIES) &&
+	    !par->blanked && tg && tg->change_e800)
+		w100fb_e800_tg_change(par);
 }
 
 
@@ -837,28 +824,6 @@ static void w100_update_enable(void)
 	writel((u32) (disp_db_buf_wr_cntl.val), remapped_regs + mmDISP_DB_BUF_CNTL);
 }
 
-unsigned long w100fb_gpio_read(int port)
-{
-	unsigned long value;
-
-	if (port=W100_GPIO_PORT_A)
-		value = readl(remapped_regs + mmGPIO_DATA);
-	else
-		value = readl(remapped_regs + mmGPIO_DATA2);
-
-	return value;
-}
-
-void w100fb_gpio_write(int port, unsigned long value)
-{
-	if (port=W100_GPIO_PORT_A)
-		writel(value, remapped_regs + mmGPIO_DATA);
-	else
-		writel(value, remapped_regs + mmGPIO_DATA2);
-}
-EXPORT_SYMBOL(w100fb_gpio_read);
-EXPORT_SYMBOL(w100fb_gpio_write);
-
 /*
  * Initialization of critical w100 hardware
  */
@@ -1497,31 +1462,6 @@ static void w100_set_dispregs(struct w100fb_par *par)
 	writel((par->xres*BITS_PER_PIXEL/8), remapped_regs + mmGRAPHIC_PITCH);
 }
 
-
-/*
- * Work out how long the sync pulse lasts
- * Value is 1/(time in seconds)
- */
-static void calc_hsync(struct w100fb_par *par)
-{
-	unsigned long hsync;
-	struct w100_mode *mode = par->mode;
-	union crtc_ss_u crtc_ss;
-
-	if (mode->pixclk_src = CLK_SRC_XTAL)
-		hsync=par->mach->xtal_freq;
-	else
-		hsync=((par->fastpll_mode && mode->fast_pll_freq) ? mode->fast_pll_freq : mode->pll_freq)*100000;
-
-	hsync /= (w100_pwr_state.pclk_cntl.f.pclk_post_div + 1);
-
-	crtc_ss.val = readl(remapped_regs + mmCRTC_SS);
-	if (crtc_ss.val)
-		par->hsync_len = hsync / (crtc_ss.f.ss_end-crtc_ss.f.ss_start);
-	else
-		par->hsync_len = 0;
-}
-
 static void w100_suspend(u32 mode)
 {
 	u32 val;
diff --git a/include/video/w100fb.h b/include/video/w100fb.h
index a614654d8598..8f260ba4fcac 100644
--- a/include/video/w100fb.h
+++ b/include/video/w100fb.h
@@ -14,13 +14,9 @@
 
 struct w100fb_par;
 
-unsigned long w100fb_gpio_read(int port);
-void w100fb_gpio_write(int port, unsigned long value);
-unsigned long w100fb_get_hsynclen(struct device *dev);
-
 /* LCD Specific Routines and Config */
 struct w100_tg_info {
-	void (*change)(struct w100fb_par*);
+	bool change_e800;
 	void (*suspend)(struct w100fb_par*);
 	void (*resume)(struct w100fb_par*);
 };
-- 
2.26.0

Re: [PATCH 1/8] fbdev: w100fb: clean up mach-pxa compile-time dependency

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

Hi Arnd,

> diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig
> index f60bc29aef68..8444d40df1b3 100644
> --- a/arch/arm/mach-pxa/Kconfig
> +++ b/arch/arm/mach-pxa/Kconfig
> @@ -549,9 +549,6 @@ config MACH_ICONTROL
>  
>  config ARCH_PXA_ESERIES
>  	bool "PXA based Toshiba e-series PDAs"
> -	select FB_W100
> -	select FB
> -	select PXA25x
This last line puzzles me a bit in this patch. Why removing the PXA25x select ?

Cheers.

-- 
Robert

Re: [PATCH 1/8] fbdev: w100fb: clean up mach-pxa compile-time dependency

[Arnd Bergmann]

On Sat, Apr 18, 2020 at 12:11 PM Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>
> Arnd Bergmann <arnd@arndb.de> writes:
>
> Hi Arnd,
>
> > diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig
> > index f60bc29aef68..8444d40df1b3 100644
> > --- a/arch/arm/mach-pxa/Kconfig
> > +++ b/arch/arm/mach-pxa/Kconfig
> > @@ -549,9 +549,6 @@ config MACH_ICONTROL
> >
> >  config ARCH_PXA_ESERIES
> >       bool "PXA based Toshiba e-series PDAs"
> > -     select FB_W100
> > -     select FB
> > -     select PXA25x
> This last line puzzles me a bit in this patch. Why removing the PXA25x select ?

It's a mistake, fixed now. Thanks for taking a look!

      Arnd

[PATCH 2/8] fbdev/ARM: pxa: avoid selecting CONFIG_FB

[Arnd Bergmann]

In order to add more compile-time dependencies to CONFIG_FB, remove the
last few instances of selecting it from platform code.

This was originally written to allow a modular driver, but that never
worked because vmlinux cannot call exported functions from a loadable
module.

Just change the #ifdef check to what it should have been and remove
the 'select' statement.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/configs/pxa_defconfig | 1 +
 arch/arm/mach-pxa/Kconfig      | 4 ----
 arch/arm/mach-pxa/saar.c       | 2 +-
 arch/arm/mach-pxa/tavorevb.c   | 2 +-
 4 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
index 8df88e8a3048..3931184e2eb1 100644
--- a/arch/arm/configs/pxa_defconfig
+++ b/arch/arm/configs/pxa_defconfig
@@ -455,6 +455,7 @@ CONFIG_SOC_CAMERA_MT9M111=m
 CONFIG_DRM=m
 CONFIG_FIRMWARE_EDID=y
 CONFIG_FB=y
+CONFIG_FB_PXA=y
 CONFIG_FB_W100=y
 CONFIG_FB_TILEBLITTING=y
 CONFIG_FB_PXA_OVERLAY=y
diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig
index 8444d40df1b3..bea8ce447ed3 100644
--- a/arch/arm/mach-pxa/Kconfig
+++ b/arch/arm/mach-pxa/Kconfig
@@ -76,16 +76,12 @@ config MACH_TAVOREVB
 	select CPU_PXA930
 	select CPU_PXA935
 	select PXA3xx
-	select FB
-	select FB_PXA
 
 config MACH_SAAR
 	bool "PXA930 Handheld Platform (aka SAAR)"
 	select CPU_PXA930
 	select CPU_PXA935
 	select PXA3xx
-	select FB
-	select FB_PXA
 
 comment "Third Party Dev Platforms (sorted by vendor name)"
 
diff --git a/arch/arm/mach-pxa/saar.c b/arch/arm/mach-pxa/saar.c
index 3275b679792b..e35d6c57f73e 100644
--- a/arch/arm/mach-pxa/saar.c
+++ b/arch/arm/mach-pxa/saar.c
@@ -113,7 +113,7 @@ static struct platform_device smc91x_device = {
 	},
 };
 
-#if defined(CONFIG_FB_PXA) || defined(CONFIG_FB_PXA_MODULE)
+#if defined(CONFIG_FB_PXA)
 static uint16_t lcd_power_on[] = {
 	/* single frame */
 	SMART_CMD_NOOP,
diff --git a/arch/arm/mach-pxa/tavorevb.c b/arch/arm/mach-pxa/tavorevb.c
index a15eb3b9484d..dda5f10b1883 100644
--- a/arch/arm/mach-pxa/tavorevb.c
+++ b/arch/arm/mach-pxa/tavorevb.c
@@ -165,7 +165,7 @@ static void __init tavorevb_init_keypad(void)
 static inline void tavorevb_init_keypad(void) {}
 #endif /* CONFIG_KEYBOARD_PXA27x || CONFIG_KEYBOARD_PXA27x_MODULE */
 
-#if defined(CONFIG_FB_PXA) || defined(CONFIG_FB_PXA_MODULE)
+#if defined(CONFIG_FB_PXA)
 static struct pwm_lookup tavorevb_pwm_lookup[] = {
 	PWM_LOOKUP("pxa27x-pwm.0", 1, "pwm-backlight.0", NULL, 100000,
 		   PWM_POLARITY_NORMAL),
-- 
2.26.0

Re: [PATCH 2/8] fbdev/ARM: pxa: avoid selecting CONFIG_FB

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> In order to add more compile-time dependencies to CONFIG_FB, remove the
> last few instances of selecting it from platform code.
>
> This was originally written to allow a modular driver, but that never
> worked because vmlinux cannot call exported functions from a loadable
> module.
>
> Just change the #ifdef check to what it should have been and remove
> the 'select' statement.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Hi Arnd,

Ok, so if you remove the select FB_PXA, I think there will be a couple of
defconfigs which _might_ break, namely :
	corgi_defconfig
	h5000_defconfig
	hx4700_good_defconfig
	lubbock_defconfig
	mmp2_defconfig
	multi_v7_defconfig
	mvebu_v7_defconfig
	pcm027_defconfig
	pxa168_defconfig
	pxa910_defconfig
	xcep_defconfig
	zylonite_defconfig

Could you please have a look to see if they shouldn't have a CONFIG_FB_PXA=y and
CONFIG_FB=y now with this change ?

Cheers.

--
Robert

[PATCH 3/8] fbdev: rework FB_DDC dependencies

[Arnd Bergmann]

Selecting FB_DDC currently turns on CONFIG_I2C implicitly,
which is often not desired and can lead to circular dependencies.

Change this to a 'depends on' and change all drivers that
rely on FB_DDC to have an appropriate I2C dependency as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/video/fbdev/Kconfig | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 91b0a719d221..bcf7834dbdbf 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -62,9 +62,8 @@ config FIRMWARE_EDID
 
 config FB_DDC
 	tristate
-	depends on FB
+	depends on FB && I2C
 	select I2C_ALGOBIT
-	select I2C
 
 config FB_BOOT_VESA_SUPPORT
 	bool
@@ -356,6 +355,7 @@ config FB_CYBER2000
 config FB_CYBER2000_DDC
 	bool "DDC for CyberPro support"
 	depends on FB_CYBER2000
+	depends on I2C=y || I2Cû_CYBER2000
 	select FB_DDC
 	default y
 	help
@@ -892,6 +892,7 @@ config FB_NVIDIA
 config FB_NVIDIA_I2C
 	bool "Enable DDC Support"
 	depends on FB_NVIDIA
+	depends on I2C=y || I2Cû_NVIDIA
 	select FB_DDC
 	help
 	  This enables I2C support for nVidia Chipsets.  This is used
@@ -938,6 +939,7 @@ config FB_RIVA
 config FB_RIVA_I2C
 	bool "Enable DDC Support"
 	depends on FB_RIVA
+	depends on I2C=y || I2Cû_RIVA
 	select FB_DDC
 	help
 	  This enables I2C support for nVidia Chipsets.  This is used
@@ -965,7 +967,7 @@ config FB_RIVA_BACKLIGHT
 
 config FB_I740
 	tristate "Intel740 support"
-	depends on FB && PCI
+	depends on FB && PCI && I2C
 	select FB_MODE_HELPERS
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
@@ -1019,6 +1021,7 @@ config FB_I810_GTF
 config FB_I810_I2C
 	bool "Enable DDC Support"
 	depends on FB_I810 && FB_I810_GTF
+	depends on I2C=y || I2Cû_I810
 	select FB_DDC
 	help
 	  Add DDC/I2C support for i810fb.  This will allow the driver to get
@@ -1074,6 +1077,7 @@ config FB_INTEL_DEBUG
 config FB_INTEL_I2C
 	bool "DDC/I2C for Intel framebuffer support"
 	depends on FB_INTEL
+	depends on I2C=y || I2Cû_INTEL
 	select FB_DDC
 	default y
 	help
@@ -1153,6 +1157,7 @@ config FB_MATROX_G
 config FB_MATROX_I2C
 	tristate "Matrox I2C support"
 	depends on FB_MATROX
+	depends on I2C=y || I2Cû_MATROX
 	select FB_DDC
 	---help---
 	  This drivers creates I2C buses which are needed for accessing the
@@ -1218,6 +1223,7 @@ config FB_RADEON
 config FB_RADEON_I2C
 	bool "DDC/I2C for ATI Radeon support"
 	depends on FB_RADEON
+	depends on I2C=y || I2Cû_RADEON
 	select FB_DDC
 	default y
 	help
@@ -1327,6 +1333,7 @@ config FB_S3
 config FB_S3_DDC
 	bool "DDC for S3 support"
 	depends on FB_S3
+	depends on I2C=y || I2Cû_S3
 	select FB_DDC
 	default y
 	help
@@ -1352,6 +1359,7 @@ config FB_SAVAGE
 config FB_SAVAGE_I2C
 	bool "Enable DDC2 Support"
 	depends on FB_SAVAGE
+	depends on I2C=y || I2Cû_SAVAGE
 	select FB_DDC
 	help
 	  This enables I2C support for S3 Savage Chipsets.  This is used
@@ -1491,6 +1499,7 @@ config FB_3DFX_ACCEL
 config FB_3DFX_I2C
 	bool "Enable DDC/I2C support"
 	depends on FB_3DFX
+	depends on I2C=y || I2Cû_3DFX
 	select FB_DDC
 	default y
 	help
@@ -1530,7 +1539,7 @@ config FB_VT8623
 
 config FB_TRIDENT
 	tristate "Trident/CyberXXX/CyberBlade support"
-	depends on FB && PCI
+	depends on FB && PCI && I2C
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
-- 
2.26.0

[PATCH 4/8] drm/rcar: stop using 'imply' for dependencies

[Arnd Bergmann]

The meaning of the 'imply' keyword has changed recently, and neither the
old meaning (select the symbol if its dependencies are met) nor the new
meaning (enable it by default, but let the user set any other setting)
is what we want here.

Work around this by adding two more Kconfig options that lead to
the correct behavior: if DRM_RCAR_USE_CMM and DRM_RCAR_USE_LVDS
are enabled, that portion of the driver becomes usable, and no
configuration results in a link error.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/rcar-du/Kconfig | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

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

[PATCH 5/8] drm/vmwgfx: make framebuffer support optional

[Arnd Bergmann]

CONFIG_FB causes unnecessary Kconfig dependency problems when selected
from another driver. Since it is already optional in vmwgfx at runtime,
turn this into a compile-time dependency so the support can be completely
left out of the driver and enabled only if CONFIG_FB is explicitly
selected in the configuration.

When the FB support is built into the driver, it is turned on by
default, but can still be disabled when loading the module.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/vmwgfx/Kconfig      | 17 +++++++-------
 drivers/gpu/drm/vmwgfx/Makefile     |  4 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 35 +++++++++++++++++++----------
 3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/Kconfig b/drivers/gpu/drm/vmwgfx/Kconfig
index 15acdf2a7c0f..b2835636991b 100644
--- a/drivers/gpu/drm/vmwgfx/Kconfig
+++ b/drivers/gpu/drm/vmwgfx/Kconfig
@@ -2,12 +2,7 @@
 config DRM_VMWGFX
 	tristate "DRM driver for VMware Virtual GPU"
 	depends on DRM && PCI && X86 && MMU
-	select FB_DEFERRED_IO
-	select FB_CFB_FILLRECT
-	select FB_CFB_COPYAREA
-	select FB_CFB_IMAGEBLIT
 	select DRM_TTM
-	select FB
 	select MAPPING_DIRTY_HELPERS
 	# Only needed for the transitional use of drm_crtc_init - can be removed
 	# again once vmwgfx sets up the primary plane itself.
@@ -20,9 +15,15 @@ config DRM_VMWGFX
 	  The compiled module will be called "vmwgfx.ko".
 
 config DRM_VMWGFX_FBCON
-	depends on DRM_VMWGFX && FB
-	bool "Enable framebuffer console under vmwgfx by default"
+	bool "Enable framebuffer console under vmwgfx"
+	depends on DRM_VMWGFX
+	depends on FB=y || FB=DRM_VMWGFX
+	select FB_DEFERRED_IO
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
 	help
 	   Choose this option if you are shipping a new vmwgfx
-	   userspace driver that supports using the kernel driver.
+	   userspace driver that supports using the kernel driver
+	   and you have framebuffer support enabled.
 
diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
index 31f85f09f1fc..905ebabc79fc 100644
--- a/drivers/gpu/drm/vmwgfx/Makefile
+++ b/drivers/gpu/drm/vmwgfx/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
-	    vmwgfx_fb.o vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o \
+	    vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o \
 	    vmwgfx_fifo.o vmwgfx_irq.o vmwgfx_ldu.o vmwgfx_ttm_glue.o \
 	    vmwgfx_overlay.o vmwgfx_marker.o vmwgfx_gmrid_manager.o \
 	    vmwgfx_fence.o vmwgfx_bo.o vmwgfx_scrn.o vmwgfx_context.o \
@@ -11,5 +11,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
 	    vmwgfx_validation.o vmwgfx_page_dirty.o vmwgfx_streamoutput.o \
 	    ttm_object.o ttm_lock.o
 
+vmwgfx-$(CONFIG_DRM_VMWGFX_FBCON) += vmwgfx_fb.o
+
 vmwgfx-$(CONFIG_TRANSPARENT_HUGEPAGE) += vmwgfx_thp.o
 obj-$(CONFIG_DRM_VMWGFX) := vmwgfx.o
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index c2247a893ed4..a471a659b89d 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -258,7 +258,6 @@ static const struct pci_device_id vmw_pci_id_list[] = {
 };
 MODULE_DEVICE_TABLE(pci, vmw_pci_id_list);
 
-static int enable_fbdev = IS_ENABLED(CONFIG_DRM_VMWGFX_FBCON);
 static int vmw_force_iommu;
 static int vmw_restrict_iommu;
 static int vmw_force_coherent;
@@ -269,8 +268,21 @@ static int vmw_probe(struct pci_dev *, const struct pci_device_id *);
 static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,
 			      void *ptr);
 
+#ifdef CONFIG_DRM_VMWGFX_FBCON
+static int enable_fbdev = 1;
 MODULE_PARM_DESC(enable_fbdev, "Enable vmwgfx fbdev");
 module_param_named(enable_fbdev, enable_fbdev, int, 0600);
+static inline bool vmw_fb_enabled(struct vmw_private *dev_priv)
+{
+	return dev_priv->enable_fb;
+}
+#else
+#define enable_fbdev 0
+static inline bool vmw_fb_enabled(struct vmw_private *dev_priv)
+{
+	return false;
+}
+#endif
 MODULE_PARM_DESC(force_dma_api, "Force using the DMA API for TTM pages");
 module_param_named(force_dma_api, vmw_force_iommu, int, 0600);
 MODULE_PARM_DESC(restrict_iommu, "Try to limit IOMMU usage for TTM pages");
@@ -669,7 +681,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 	dev_priv->mmio_start = pci_resource_start(dev->pdev, 2);
 
 	dev_priv->assume_16bpp = !!vmw_assume_16bpp;
-
 	dev_priv->enable_fb = enable_fbdev;
 
 	vmw_write(dev_priv, SVGA_REG_ID, SVGA_ID_2);
@@ -945,7 +956,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 		VMWGFX_DRIVER_PATCHLEVEL);
 	vmw_host_log(host_log);
 
-	if (dev_priv->enable_fb) {
+	if (vmw_fb_enabled(dev_priv)) {
 		vmw_fifo_resource_inc(dev_priv);
 		vmw_svga_enable(dev_priv);
 		vmw_fb_init(dev_priv);
@@ -1001,7 +1012,7 @@ static void vmw_driver_unload(struct drm_device *dev)
 	if (dev_priv->ctx.res_ht_initialized)
 		drm_ht_remove(&dev_priv->ctx.res_ht);
 	vfree(dev_priv->ctx.cmd_bounce);
-	if (dev_priv->enable_fb) {
+	if (vmw_fb_enabled(dev_priv)) {
 		vmw_fb_off(dev_priv);
 		vmw_fb_close(dev_priv);
 		vmw_fifo_resource_dec(dev_priv);
@@ -1149,7 +1160,7 @@ static void vmw_master_drop(struct drm_device *dev,
 	struct vmw_private *dev_priv = vmw_priv(dev);
 
 	vmw_kms_legacy_hotspot_clear(dev_priv);
-	if (!dev_priv->enable_fb)
+	if (vmw_fb_enabled(dev_priv))
 		vmw_svga_disable(dev_priv);
 }
 
@@ -1347,7 +1358,7 @@ static int vmw_pm_freeze(struct device *kdev)
 		DRM_ERROR("Failed to freeze modesetting.\n");
 		return ret;
 	}
-	if (dev_priv->enable_fb)
+	if (vmw_fb_enabled(dev_priv))
 		vmw_fb_off(dev_priv);
 
 	ttm_suspend_lock(&dev_priv->reservation_sem);
@@ -1355,18 +1366,18 @@ static int vmw_pm_freeze(struct device *kdev)
 	vmw_resource_evict_all(dev_priv);
 	vmw_release_device_early(dev_priv);
 	ttm_bo_swapout_all(&dev_priv->bdev);
-	if (dev_priv->enable_fb)
+	if (vmw_fb_enabled(dev_priv))
 		vmw_fifo_resource_dec(dev_priv);
 	if (atomic_read(&dev_priv->num_fifo_resources) != 0) {
 		DRM_ERROR("Can't hibernate while 3D resources are active.\n");
-		if (dev_priv->enable_fb)
+		if (vmw_fb_enabled(dev_priv))
 			vmw_fifo_resource_inc(dev_priv);
 		WARN_ON(vmw_request_device_late(dev_priv));
 		dev_priv->suspend_locked = false;
 		ttm_suspend_unlock(&dev_priv->reservation_sem);
 		if (dev_priv->suspend_state)
 			vmw_kms_resume(dev);
-		if (dev_priv->enable_fb)
+		if (vmw_fb_enabled(dev_priv))
 			vmw_fb_on(dev_priv);
 		return -EBUSY;
 	}
@@ -1388,14 +1399,14 @@ static int vmw_pm_restore(struct device *kdev)
 	vmw_write(dev_priv, SVGA_REG_ID, SVGA_ID_2);
 	(void) vmw_read(dev_priv, SVGA_REG_ID);
 
-	if (dev_priv->enable_fb)
+	if (vmw_fb_enabled(dev_priv))
 		vmw_fifo_resource_inc(dev_priv);
 
 	ret = vmw_request_device(dev_priv);
 	if (ret)
 		return ret;
 
-	if (dev_priv->enable_fb)
+	if (vmw_fb_enabled(dev_priv))
 		__vmw_svga_enable(dev_priv);
 
 	vmw_fence_fifo_up(dev_priv->fman);
@@ -1404,7 +1415,7 @@ static int vmw_pm_restore(struct device *kdev)
 	if (dev_priv->suspend_state)
 		vmw_kms_resume(dev_priv->dev);
 
-	if (dev_priv->enable_fb)
+	if (vmw_fb_enabled(dev_priv))
 		vmw_fb_on(dev_priv);
 
 	return 0;
-- 
2.26.0

Re: [PATCH 5/8] drm/vmwgfx: make framebuffer support optional

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 8332 bytes --]

Hi

IMHO, at some point console support will require a change where there's
a single config option enables/disables fbdev emulation for all drivers.
But this patch is a step in the right direction. Thanks!

Am 17.04.20 um 17:55 schrieb Arnd Bergmann:
> CONFIG_FB causes unnecessary Kconfig dependency problems when selected
> from another driver. Since it is already optional in vmwgfx at runtime,
> turn this into a compile-time dependency so the support can be completely
> left out of the driver and enabled only if CONFIG_FB is explicitly
> selected in the configuration.
> 
> When the FB support is built into the driver, it is turned on by
> default, but can still be disabled when loading the module.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/vmwgfx/Kconfig      | 17 +++++++-------
>  drivers/gpu/drm/vmwgfx/Makefile     |  4 +++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 35 +++++++++++++++++++----------
>  3 files changed, 35 insertions(+), 21 deletions(-)

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> 
> diff --git a/drivers/gpu/drm/vmwgfx/Kconfig b/drivers/gpu/drm/vmwgfx/Kconfig
> index 15acdf2a7c0f..b2835636991b 100644
> --- a/drivers/gpu/drm/vmwgfx/Kconfig
> +++ b/drivers/gpu/drm/vmwgfx/Kconfig
> @@ -2,12 +2,7 @@
>  config DRM_VMWGFX
>  	tristate "DRM driver for VMware Virtual GPU"
>  	depends on DRM && PCI && X86 && MMU
> -	select FB_DEFERRED_IO
> -	select FB_CFB_FILLRECT
> -	select FB_CFB_COPYAREA
> -	select FB_CFB_IMAGEBLIT
>  	select DRM_TTM
> -	select FB
>  	select MAPPING_DIRTY_HELPERS
>  	# Only needed for the transitional use of drm_crtc_init - can be removed
>  	# again once vmwgfx sets up the primary plane itself.
> @@ -20,9 +15,15 @@ config DRM_VMWGFX
>  	  The compiled module will be called "vmwgfx.ko".
>  
>  config DRM_VMWGFX_FBCON
> -	depends on DRM_VMWGFX && FB
> -	bool "Enable framebuffer console under vmwgfx by default"
> +	bool "Enable framebuffer console under vmwgfx"
> +	depends on DRM_VMWGFX
> +	depends on FB=y || FB=DRM_VMWGFX
> +	select FB_DEFERRED_IO
> +	select FB_CFB_FILLRECT
> +	select FB_CFB_COPYAREA
> +	select FB_CFB_IMAGEBLIT
>  	help
>  	   Choose this option if you are shipping a new vmwgfx
> -	   userspace driver that supports using the kernel driver.
> +	   userspace driver that supports using the kernel driver
> +	   and you have framebuffer support enabled.
>  
> diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
> index 31f85f09f1fc..905ebabc79fc 100644
> --- a/drivers/gpu/drm/vmwgfx/Makefile
> +++ b/drivers/gpu/drm/vmwgfx/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
> -	    vmwgfx_fb.o vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o \
> +	    vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o \
>  	    vmwgfx_fifo.o vmwgfx_irq.o vmwgfx_ldu.o vmwgfx_ttm_glue.o \
>  	    vmwgfx_overlay.o vmwgfx_marker.o vmwgfx_gmrid_manager.o \
>  	    vmwgfx_fence.o vmwgfx_bo.o vmwgfx_scrn.o vmwgfx_context.o \
> @@ -11,5 +11,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
>  	    vmwgfx_validation.o vmwgfx_page_dirty.o vmwgfx_streamoutput.o \
>  	    ttm_object.o ttm_lock.o
>  
> +vmwgfx-$(CONFIG_DRM_VMWGFX_FBCON) += vmwgfx_fb.o
> +
>  vmwgfx-$(CONFIG_TRANSPARENT_HUGEPAGE) += vmwgfx_thp.o
>  obj-$(CONFIG_DRM_VMWGFX) := vmwgfx.o
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index c2247a893ed4..a471a659b89d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -258,7 +258,6 @@ static const struct pci_device_id vmw_pci_id_list[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, vmw_pci_id_list);
>  
> -static int enable_fbdev = IS_ENABLED(CONFIG_DRM_VMWGFX_FBCON);
>  static int vmw_force_iommu;
>  static int vmw_restrict_iommu;
>  static int vmw_force_coherent;
> @@ -269,8 +268,21 @@ static int vmw_probe(struct pci_dev *, const struct pci_device_id *);
>  static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,
>  			      void *ptr);
>  
> +#ifdef CONFIG_DRM_VMWGFX_FBCON
> +static int enable_fbdev = 1;
>  MODULE_PARM_DESC(enable_fbdev, "Enable vmwgfx fbdev");
>  module_param_named(enable_fbdev, enable_fbdev, int, 0600);
> +static inline bool vmw_fb_enabled(struct vmw_private *dev_priv)
> +{
> +	return dev_priv->enable_fb;
> +}
> +#else
> +#define enable_fbdev 0
> +static inline bool vmw_fb_enabled(struct vmw_private *dev_priv)
> +{
> +	return false;
> +}
> +#endif
>  MODULE_PARM_DESC(force_dma_api, "Force using the DMA API for TTM pages");
>  module_param_named(force_dma_api, vmw_force_iommu, int, 0600);
>  MODULE_PARM_DESC(restrict_iommu, "Try to limit IOMMU usage for TTM pages");
> @@ -669,7 +681,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
>  	dev_priv->mmio_start = pci_resource_start(dev->pdev, 2);
>  
>  	dev_priv->assume_16bpp = !!vmw_assume_16bpp;
> -
>  	dev_priv->enable_fb = enable_fbdev;
>  
>  	vmw_write(dev_priv, SVGA_REG_ID, SVGA_ID_2);
> @@ -945,7 +956,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
>  		VMWGFX_DRIVER_PATCHLEVEL);
>  	vmw_host_log(host_log);
>  
> -	if (dev_priv->enable_fb) {
> +	if (vmw_fb_enabled(dev_priv)) {
>  		vmw_fifo_resource_inc(dev_priv);
>  		vmw_svga_enable(dev_priv);
>  		vmw_fb_init(dev_priv);
> @@ -1001,7 +1012,7 @@ static void vmw_driver_unload(struct drm_device *dev)
>  	if (dev_priv->ctx.res_ht_initialized)
>  		drm_ht_remove(&dev_priv->ctx.res_ht);
>  	vfree(dev_priv->ctx.cmd_bounce);
> -	if (dev_priv->enable_fb) {
> +	if (vmw_fb_enabled(dev_priv)) {
>  		vmw_fb_off(dev_priv);
>  		vmw_fb_close(dev_priv);
>  		vmw_fifo_resource_dec(dev_priv);
> @@ -1149,7 +1160,7 @@ static void vmw_master_drop(struct drm_device *dev,
>  	struct vmw_private *dev_priv = vmw_priv(dev);
>  
>  	vmw_kms_legacy_hotspot_clear(dev_priv);
> -	if (!dev_priv->enable_fb)
> +	if (vmw_fb_enabled(dev_priv))
>  		vmw_svga_disable(dev_priv);
>  }
>  
> @@ -1347,7 +1358,7 @@ static int vmw_pm_freeze(struct device *kdev)
>  		DRM_ERROR("Failed to freeze modesetting.\n");
>  		return ret;
>  	}
> -	if (dev_priv->enable_fb)
> +	if (vmw_fb_enabled(dev_priv))
>  		vmw_fb_off(dev_priv);
>  
>  	ttm_suspend_lock(&dev_priv->reservation_sem);
> @@ -1355,18 +1366,18 @@ static int vmw_pm_freeze(struct device *kdev)
>  	vmw_resource_evict_all(dev_priv);
>  	vmw_release_device_early(dev_priv);
>  	ttm_bo_swapout_all(&dev_priv->bdev);
> -	if (dev_priv->enable_fb)
> +	if (vmw_fb_enabled(dev_priv))
>  		vmw_fifo_resource_dec(dev_priv);
>  	if (atomic_read(&dev_priv->num_fifo_resources) != 0) {
>  		DRM_ERROR("Can't hibernate while 3D resources are active.\n");
> -		if (dev_priv->enable_fb)
> +		if (vmw_fb_enabled(dev_priv))
>  			vmw_fifo_resource_inc(dev_priv);
>  		WARN_ON(vmw_request_device_late(dev_priv));
>  		dev_priv->suspend_locked = false;
>  		ttm_suspend_unlock(&dev_priv->reservation_sem);
>  		if (dev_priv->suspend_state)
>  			vmw_kms_resume(dev);
> -		if (dev_priv->enable_fb)
> +		if (vmw_fb_enabled(dev_priv))
>  			vmw_fb_on(dev_priv);
>  		return -EBUSY;
>  	}
> @@ -1388,14 +1399,14 @@ static int vmw_pm_restore(struct device *kdev)
>  	vmw_write(dev_priv, SVGA_REG_ID, SVGA_ID_2);
>  	(void) vmw_read(dev_priv, SVGA_REG_ID);
>  
> -	if (dev_priv->enable_fb)
> +	if (vmw_fb_enabled(dev_priv))
>  		vmw_fifo_resource_inc(dev_priv);
>  
>  	ret = vmw_request_device(dev_priv);
>  	if (ret)
>  		return ret;
>  
> -	if (dev_priv->enable_fb)
> +	if (vmw_fb_enabled(dev_priv))
>  		__vmw_svga_enable(dev_priv);
>  
>  	vmw_fence_fifo_up(dev_priv->fman);
> @@ -1404,7 +1415,7 @@ static int vmw_pm_restore(struct device *kdev)
>  	if (dev_priv->suspend_state)
>  		vmw_kms_resume(dev_priv->dev);
>  
> -	if (dev_priv->enable_fb)
> +	if (vmw_fb_enabled(dev_priv))
>  		vmw_fb_on(dev_priv);
>  
>  	return 0;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 6/8] drm: decouple from CONFIG_FB

[Arnd Bergmann]

CONFIG_DRM_KMS_FB_HELPER selects CONFIG_FB, which is something it
really should not, to avoid circular dependencies and accidentally
including potentially dangerous user interfaces in the kernel,
so change this into a 'depends on' check.

Two device drivers currently select CONFIG_DRM_KMS_FB_HELPER, but
as far as I can tell, they do not really need to any more, so those
selects can be removed.

This leaves DRM_FBDEV_EMULATION as the only thing that rightfully
selects CONFIG_DRM_KMS_FB_HELPER, and this now has to depend on
CONFIG_FB and its dependencies.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/Kconfig       | 5 +++--
 drivers/gpu/drm/mxsfb/Kconfig | 1 -
 drivers/gpu/drm/zte/Kconfig   | 1 -
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 43594978958e..7c3109133685 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -80,7 +80,7 @@ config DRM_KMS_HELPER
 config DRM_KMS_FB_HELPER
 	bool
 	depends on DRM_KMS_HELPER
-	select FB
+	depends on FB
 	select FRAMEBUFFER_CONSOLE if !EXPERT
 	select FRAMEBUFFER_CONSOLE_DETECT_PRIMARY if FRAMEBUFFER_CONSOLE
 	select FB_SYS_FOPS
@@ -111,7 +111,8 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
 
 config DRM_FBDEV_EMULATION
 	bool "Enable legacy fbdev support for your modesetting driver"
-	depends on DRM
+	depends on DRM && FB
+	depends on FB=y || DRM=m
 	select DRM_KMS_HELPER
 	select DRM_KMS_FB_HELPER
 	default y
diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
index 0dca8f27169e..33916b7b2c50 100644
--- a/drivers/gpu/drm/mxsfb/Kconfig
+++ b/drivers/gpu/drm/mxsfb/Kconfig
@@ -10,7 +10,6 @@ config DRM_MXSFB
 	depends on COMMON_CLK
 	select DRM_MXS
 	select DRM_KMS_HELPER
-	select DRM_KMS_FB_HELPER
 	select DRM_KMS_CMA_HELPER
 	select DRM_PANEL
 	help
diff --git a/drivers/gpu/drm/zte/Kconfig b/drivers/gpu/drm/zte/Kconfig
index 90ebaedc11fd..aa8594190b50 100644
--- a/drivers/gpu/drm/zte/Kconfig
+++ b/drivers/gpu/drm/zte/Kconfig
@@ -3,7 +3,6 @@ config DRM_ZTE
 	tristate "DRM Support for ZTE SoCs"
 	depends on DRM && ARCH_ZX
 	select DRM_KMS_CMA_HELPER
-	select DRM_KMS_FB_HELPER
 	select DRM_KMS_HELPER
 	select SND_SOC_HDMI_CODEC if SND_SOC
 	select VIDEOMODE_HELPERS
-- 
2.26.0

Re: [PATCH 6/8] drm: decouple from CONFIG_FB

[Sam Ravnborg]

Hi Arnd.

On Fri, Apr 17, 2020 at 05:55:51PM +0200, Arnd Bergmann wrote:
> CONFIG_DRM_KMS_FB_HELPER selects CONFIG_FB, which is something it
> really should not, to avoid circular dependencies and accidentally
> including potentially dangerous user interfaces in the kernel,
> so change this into a 'depends on' check.
> 
> Two device drivers currently select CONFIG_DRM_KMS_FB_HELPER, but
> as far as I can tell, they do not really need to any more, so those
> selects can be removed.
> 
> This leaves DRM_FBDEV_EMULATION as the only thing that rightfully
> selects CONFIG_DRM_KMS_FB_HELPER, and this now has to depend on
> CONFIG_FB and its dependencies.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/Kconfig       | 5 +++--
>  drivers/gpu/drm/mxsfb/Kconfig | 1 -
>  drivers/gpu/drm/zte/Kconfig   | 1 -
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 43594978958e..7c3109133685 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -80,7 +80,7 @@ config DRM_KMS_HELPER
>  config DRM_KMS_FB_HELPER
>  	bool
>  	depends on DRM_KMS_HELPER
> -	select FB
> +	depends on FB
>  	select FRAMEBUFFER_CONSOLE if !EXPERT
>  	select FRAMEBUFFER_CONSOLE_DETECT_PRIMARY if FRAMEBUFFER_CONSOLE
>  	select FB_SYS_FOPS
> @@ -111,7 +111,8 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
>  
>  config DRM_FBDEV_EMULATION
>  	bool "Enable legacy fbdev support for your modesetting driver"
> -	depends on DRM
> +	depends on DRM && FB
> +	depends on FB=y || DRM=m
>  	select DRM_KMS_HELPER
>  	select DRM_KMS_FB_HELPER
>  	default y
This statement:

	depends on DRM && FB

tell us that both symbols must be either y or m. Any combination of y
and m will do the trick


Then we have this statement:

	depends on FB=y || DRM=m

It tells us that either FB equals y or DRM equals m.

So we have following table

	FB	DRM	Result
	n	n	n
	n	y	n
	n	m	n
	y	n	n
	y	y	y
	y	m	y
	m	n	n
	m	y	N
	m	m	y

So what this try to say is that we cannot have FB a module while DRM is
built-in (marked N in the above).

Could you explain in the changelog why this combination is not good.
(Or tell me if my analysis was flawed).

With this fixed (assuming I am right):
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

> diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
> index 0dca8f27169e..33916b7b2c50 100644
> --- a/drivers/gpu/drm/mxsfb/Kconfig
> +++ b/drivers/gpu/drm/mxsfb/Kconfig
> @@ -10,7 +10,6 @@ config DRM_MXSFB
>  	depends on COMMON_CLK
>  	select DRM_MXS
>  	select DRM_KMS_HELPER
> -	select DRM_KMS_FB_HELPER
>  	select DRM_KMS_CMA_HELPER
>  	select DRM_PANEL
>  	help
> diff --git a/drivers/gpu/drm/zte/Kconfig b/drivers/gpu/drm/zte/Kconfig
> index 90ebaedc11fd..aa8594190b50 100644
> --- a/drivers/gpu/drm/zte/Kconfig
> +++ b/drivers/gpu/drm/zte/Kconfig
> @@ -3,7 +3,6 @@ config DRM_ZTE
>  	tristate "DRM Support for ZTE SoCs"
>  	depends on DRM && ARCH_ZX
>  	select DRM_KMS_CMA_HELPER
> -	select DRM_KMS_FB_HELPER
>  	select DRM_KMS_HELPER
>  	select SND_SOC_HDMI_CODEC if SND_SOC
>  	select VIDEOMODE_HELPERS
> -- 
> 2.26.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm: decouple from CONFIG_FB

[Arnd Bergmann]

On Fri, Apr 17, 2020 at 6:50 PM Sam Ravnborg <sam@ravnborg.org> wrote:

>
> So what this try to say is that we cannot have FB a module while DRM is
> built-in (marked N in the above).

Correct

>
> Could you explain in the changelog why this combination is not good.
> (Or tell me if my analysis was flawed).

I agree in hindsight this was less obvious than I thought ;-)

Added this text to the changelog:

|  When CONFIG_FB is a loadable module, DRM_KMS_FB_HELPER cannot be
| part of the built-in subsystem, so add dependency to ensure this
| can only be enabled if the DRM module can successfully be linked.

and this comment in the Kconfig file:

@@ -112,7 +112,7 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
 config DRM_FBDEV_EMULATION
        bool "Enable legacy fbdev support for your modesetting driver"
        depends on DRM && FB
-       depends on FB=y || DRM=m
+       depends on FB=y || DRM=m # DRM_KMS_FB_HELPER links against FB
        select DRM_KMS_HELPER
        select DRM_KMS_FB_HELPER
        default y

Let me know if you think those changes are sufficient

> With this fixed (assuming I am right):
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Thanks!

        Arnd

Re: [PATCH 6/8] drm: decouple from CONFIG_FB

[Sam Ravnborg]

Hi Arnd.

On Fri, Apr 17, 2020 at 10:03:23PM +0200, Arnd Bergmann wrote:
> On Fri, Apr 17, 2020 at 6:50 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> >
> > So what this try to say is that we cannot have FB a module while DRM is
> > built-in (marked N in the above).
> 
> Correct
> 
> >
> > Could you explain in the changelog why this combination is not good.
> > (Or tell me if my analysis was flawed).
> 
> I agree in hindsight this was less obvious than I thought ;-)
> 
> Added this text to the changelog:
> 
> |  When CONFIG_FB is a loadable module, DRM_KMS_FB_HELPER cannot be
> | part of the built-in subsystem, so add dependency to ensure this
> | can only be enabled if the DRM module can successfully be linked.
> 
> and this comment in the Kconfig file:
> 
> @@ -112,7 +112,7 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
>  config DRM_FBDEV_EMULATION
>         bool "Enable legacy fbdev support for your modesetting driver"
>         depends on DRM && FB
> -       depends on FB=y || DRM=m
> +       depends on FB=y || DRM=m # DRM_KMS_FB_HELPER links against FB
>         select DRM_KMS_HELPER
>         select DRM_KMS_FB_HELPER
>         default y
> 
> Let me know if you think those changes are sufficient
> 
> > With this fixed (assuming I am right):
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Yes, that explains it nicely - thanks.
But unless Daniel decides otherwise we do not get it applied.
He was not too happy with it.

	Sam

[PATCH 7/8] fbdev: rework backlight dependencies

[Arnd Bergmann]

Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE,
make any driver that needs it have a dependency on the class device
being available, to prevent circular dependencies.

This is the same way that the backlight is already treated for the DRM
subsystem.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/auxdisplay/Kconfig        |  1 +
 drivers/macintosh/Kconfig         |  1 +
 drivers/staging/fbtft/Kconfig     |  1 +
 drivers/staging/olpc_dcon/Kconfig |  2 +-
 drivers/video/fbdev/Kconfig       | 14 +++++++++++---
 5 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 48efa7a047f3..f5751b5b0e88 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -144,6 +144,7 @@ config IMG_ASCII_LCD
 config HT16K33
 	tristate "Holtek Ht16K33 LED controller with keyscan"
 	depends on FB && OF && I2C && INPUT
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_SYS_FOPS
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
index cbd46c1c5bf7..a1c6677c7043 100644
--- a/drivers/macintosh/Kconfig
+++ b/drivers/macintosh/Kconfig
@@ -113,6 +113,7 @@ config PMAC_MEDIABAY
 config PMAC_BACKLIGHT
 	bool "Backlight control for LCD screens"
 	depends on PPC_PMAC && ADB_PMU && FB = y && (BROKEN || !PPC64)
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_BACKLIGHT
 	help
 	  Say Y here to enable Macintosh specific extensions of the generic
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index dad1ddcd7b0c..c4f2f01cd798 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -3,6 +3,7 @@ menuconfig FB_TFT
 	tristate "Support for small TFT LCD display modules"
 	depends on FB && SPI
 	depends on GPIOLIB || COMPILE_TEST
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
 	select FB_SYS_IMAGEBLIT
diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig
index d1a0dea09ef0..a9f36538d7ab 100644
--- a/drivers/staging/olpc_dcon/Kconfig
+++ b/drivers/staging/olpc_dcon/Kconfig
@@ -4,7 +4,7 @@ config FB_OLPC_DCON
 	depends on OLPC && FB
 	depends on I2C
 	depends on GPIO_CS5535 && ACPI
-	select BACKLIGHT_CLASS_DEVICE
+	depends on BACKLIGHT_CLASS_DEVICE
 	help
 	  In order to support very low power operation, the XO laptop uses a
 	  secondary Display CONtroller, or DCON.  This secondary controller
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index bcf7834dbdbf..47e1b65276f4 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -186,7 +186,7 @@ config FB_MACMODES
 config FB_BACKLIGHT
 	tristate
 	depends on FB
-	select BACKLIGHT_CLASS_DEVICE
+	depends on BACKLIGHT_CLASS_DEVICE
 
 config FB_MODE_HELPERS
 	bool "Enable Video Mode Handling Helpers"
@@ -275,12 +275,12 @@ config FB_ARMCLCD
 	tristate "ARM PrimeCell PL110 support"
 	depends on ARM || ARM64 || COMPILE_TEST
 	depends on FB && ARM_AMBA && HAS_IOMEM
+	depends on BACKLIGHT_CLASS_DEVICE || !OF
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
 	select FB_MODE_HELPERS if OF
 	select VIDEOMODE_HELPERS if OF
-	select BACKLIGHT_CLASS_DEVICE if OF
 	help
 	  This framebuffer device driver is for the ARM PrimeCell PL110
 	  Colour LCD controller.  ARM PrimeCells provide the building
@@ -861,6 +861,7 @@ config FB_ATMEL
 	tristate "AT91 LCD Controller support"
 	depends on FB && OF && HAVE_CLK && HAS_IOMEM
 	depends on HAVE_FB_ATMEL || COMPILE_TEST
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_BACKLIGHT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
@@ -914,6 +915,7 @@ config FB_NVIDIA_DEBUG
 config FB_NVIDIA_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_NVIDIA
+	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_NVIDIA
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -961,6 +963,7 @@ config FB_RIVA_DEBUG
 config FB_RIVA_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_RIVA
+	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_RIVA
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1232,6 +1235,7 @@ config FB_RADEON_I2C
 config FB_RADEON_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_RADEON
+	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_RADEON
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1263,6 +1267,7 @@ config FB_ATY128
 config FB_ATY128_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_ATY128
+	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_ATY128
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1312,6 +1317,7 @@ config FB_ATY_GX
 
 config FB_ATY_BACKLIGHT
 	bool "Support for backlight control"
+	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_ATY
 	depends on FB_ATY
 	default y
 	help
@@ -1855,6 +1861,7 @@ config FB_SH_MOBILE_LCDC
 	tristate "SuperH Mobile LCDC framebuffer support"
 	depends on FB && HAVE_CLK && HAS_IOMEM
 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
 	select FB_SYS_IMAGEBLIT
@@ -2183,7 +2190,7 @@ config FB_PRE_INIT_FB
 config FB_MX3
 	tristate "MX3 Framebuffer support"
 	depends on FB && MX3_IPU
-	select BACKLIGHT_CLASS_DEVICE
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -2250,6 +2257,7 @@ config FB_SSD1307
 	depends on FB && I2C
 	depends on OF
 	depends on GPIOLIB || COMPILE_TEST
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_SYS_FOPS
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
-- 
2.26.0

Re: [PATCH 7/8] fbdev: rework backlight dependencies

[Sam Ravnborg]

Hi Arnd.

On Fri, Apr 17, 2020 at 05:55:52PM +0200, Arnd Bergmann wrote:
> Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE,
> make any driver that needs it have a dependency on the class device
> being available, to prevent circular dependencies.
> 
> This is the same way that the backlight is already treated for the DRM
> subsystem.

I am not happy with the direction of this patch.
It is not easy to understand that one has to enable backlight to
be allowed to select a display or an fbdev driver.


How about somthing like this:

config BACKLIGHT_CLASS_DEVICE
	tristate
	# Will enable backlight module
	# Has no dependencies

config	BACKLIGHT
	bool "Backlight drivers"
	# Will make the backlight drivers visible - a visibility option
	# only

if BACKLIGHT

config BACKLIGHT_ATMEL_LCDC
	...
config BACKLIGHT_EP93XX

endif


All drivers outside video/backlight/ can then select
BACKLIGHT_CLASS_DEVICE to get the backlight core
module.
Or in other words BACKLIGHT_CLASS_DEVICE is used as a sort
of library symbol that gices us access to backlight functionality.

I tried something like this some time ago - but it did not work out for me.
But maybe I just missed something obviously in the maze of Kconfig.

Loking at your patch it is obviously that the approach I suggest above
would require all relevant drivers to explicit add a
select BACKLIGHT_CLASS_DEVICE.

But that is not a bad thing IMO.


	Sam


> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/auxdisplay/Kconfig        |  1 +
>  drivers/macintosh/Kconfig         |  1 +
>  drivers/staging/fbtft/Kconfig     |  1 +
>  drivers/staging/olpc_dcon/Kconfig |  2 +-
>  drivers/video/fbdev/Kconfig       | 14 +++++++++++---
>  5 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index 48efa7a047f3..f5751b5b0e88 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -144,6 +144,7 @@ config IMG_ASCII_LCD
>  config HT16K33
>  	tristate "Holtek Ht16K33 LED controller with keyscan"
>  	depends on FB && OF && I2C && INPUT
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_SYS_FOPS
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
> diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
> index cbd46c1c5bf7..a1c6677c7043 100644
> --- a/drivers/macintosh/Kconfig
> +++ b/drivers/macintosh/Kconfig
> @@ -113,6 +113,7 @@ config PMAC_MEDIABAY
>  config PMAC_BACKLIGHT
>  	bool "Backlight control for LCD screens"
>  	depends on PPC_PMAC && ADB_PMU && FB = y && (BROKEN || !PPC64)
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_BACKLIGHT
>  	help
>  	  Say Y here to enable Macintosh specific extensions of the generic
> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
> index dad1ddcd7b0c..c4f2f01cd798 100644
> --- a/drivers/staging/fbtft/Kconfig
> +++ b/drivers/staging/fbtft/Kconfig
> @@ -3,6 +3,7 @@ menuconfig FB_TFT
>  	tristate "Support for small TFT LCD display modules"
>  	depends on FB && SPI
>  	depends on GPIOLIB || COMPILE_TEST
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
>  	select FB_SYS_IMAGEBLIT
> diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig
> index d1a0dea09ef0..a9f36538d7ab 100644
> --- a/drivers/staging/olpc_dcon/Kconfig
> +++ b/drivers/staging/olpc_dcon/Kconfig
> @@ -4,7 +4,7 @@ config FB_OLPC_DCON
>  	depends on OLPC && FB
>  	depends on I2C
>  	depends on GPIO_CS5535 && ACPI
> -	select BACKLIGHT_CLASS_DEVICE
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	help
>  	  In order to support very low power operation, the XO laptop uses a
>  	  secondary Display CONtroller, or DCON.  This secondary controller
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index bcf7834dbdbf..47e1b65276f4 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -186,7 +186,7 @@ config FB_MACMODES
>  config FB_BACKLIGHT
>  	tristate
>  	depends on FB
> -	select BACKLIGHT_CLASS_DEVICE
> +	depends on BACKLIGHT_CLASS_DEVICE
>  
>  config FB_MODE_HELPERS
>  	bool "Enable Video Mode Handling Helpers"
> @@ -275,12 +275,12 @@ config FB_ARMCLCD
>  	tristate "ARM PrimeCell PL110 support"
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	depends on FB && ARM_AMBA && HAS_IOMEM
> +	depends on BACKLIGHT_CLASS_DEVICE || !OF
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
>  	select FB_MODE_HELPERS if OF
>  	select VIDEOMODE_HELPERS if OF
> -	select BACKLIGHT_CLASS_DEVICE if OF
>  	help
>  	  This framebuffer device driver is for the ARM PrimeCell PL110
>  	  Colour LCD controller.  ARM PrimeCells provide the building
> @@ -861,6 +861,7 @@ config FB_ATMEL
>  	tristate "AT91 LCD Controller support"
>  	depends on FB && OF && HAVE_CLK && HAS_IOMEM
>  	depends on HAVE_FB_ATMEL || COMPILE_TEST
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_BACKLIGHT
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
> @@ -914,6 +915,7 @@ config FB_NVIDIA_DEBUG
>  config FB_NVIDIA_BACKLIGHT
>  	bool "Support for backlight control"
>  	depends on FB_NVIDIA
> +	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_NVIDIA
>  	default y
>  	help
>  	  Say Y here if you want to control the backlight of your display.
> @@ -961,6 +963,7 @@ config FB_RIVA_DEBUG
>  config FB_RIVA_BACKLIGHT
>  	bool "Support for backlight control"
>  	depends on FB_RIVA
> +	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_RIVA
>  	default y
>  	help
>  	  Say Y here if you want to control the backlight of your display.
> @@ -1232,6 +1235,7 @@ config FB_RADEON_I2C
>  config FB_RADEON_BACKLIGHT
>  	bool "Support for backlight control"
>  	depends on FB_RADEON
> +	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_RADEON
>  	default y
>  	help
>  	  Say Y here if you want to control the backlight of your display.
> @@ -1263,6 +1267,7 @@ config FB_ATY128
>  config FB_ATY128_BACKLIGHT
>  	bool "Support for backlight control"
>  	depends on FB_ATY128
> +	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_ATY128
>  	default y
>  	help
>  	  Say Y here if you want to control the backlight of your display.
> @@ -1312,6 +1317,7 @@ config FB_ATY_GX
>  
>  config FB_ATY_BACKLIGHT
>  	bool "Support for backlight control"
> +	depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICEû_ATY
>  	depends on FB_ATY
>  	default y
>  	help
> @@ -1855,6 +1861,7 @@ config FB_SH_MOBILE_LCDC
>  	tristate "SuperH Mobile LCDC framebuffer support"
>  	depends on FB && HAVE_CLK && HAS_IOMEM
>  	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
>  	select FB_SYS_IMAGEBLIT
> @@ -2183,7 +2190,7 @@ config FB_PRE_INIT_FB
>  config FB_MX3
>  	tristate "MX3 Framebuffer support"
>  	depends on FB && MX3_IPU
> -	select BACKLIGHT_CLASS_DEVICE
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> @@ -2250,6 +2257,7 @@ config FB_SSD1307
>  	depends on FB && I2C
>  	depends on OF
>  	depends on GPIOLIB || COMPILE_TEST
> +	depends on BACKLIGHT_CLASS_DEVICE
>  	select FB_SYS_FOPS
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
> -- 
> 2.26.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] fbdev: rework backlight dependencies

[Arnd Bergmann]

On Fri, Apr 17, 2020 at 7:04 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Arnd.
>
> On Fri, Apr 17, 2020 at 05:55:52PM +0200, Arnd Bergmann wrote:
> > Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE,
> > make any driver that needs it have a dependency on the class device
> > being available, to prevent circular dependencies.
> >
> > This is the same way that the backlight is already treated for the DRM
> > subsystem.
>
> I am not happy with the direction of this patch.
> It is not easy to understand that one has to enable backlight to
> be allowed to select a display or an fbdev driver.
>
>
> How about somthing like this:
>
> config BACKLIGHT_CLASS_DEVICE
>         tristate
>         # Will enable backlight module
>         # Has no dependencies
>
> config  BACKLIGHT
>         bool "Backlight drivers"
>         # Will make the backlight drivers visible - a visibility option
>         # only
>
> All drivers outside video/backlight/ can then select
> BACKLIGHT_CLASS_DEVICE to get the backlight core
> module.
> Or in other words BACKLIGHT_CLASS_DEVICE is used as a sort
> of library symbol that gices us access to backlight functionality.

Right, this could work as long as nobody puts back any "depends on
BACKLIGHT_CLASS_DEVICE, but it does cause another problem: There
are a couple of drivers that can optionally use backlight support
or just leave it out depending on CONFIG_BACKLIGHT_CLASS_DEVICE.

Changing them to select the class device support unconditionally
would make it impossible to build those drivers without it.
Instead we could all need an individual Kconfig symbol, or use
have "select BACKLIGHT_CLASS_DEVICE if BACKLIGHT"
in each case, but I'm not sure if that's still a win over having a
simple 'depends on BACKLIGHT_CLASS_DEVICE'.

On a related note, the NEW_LEDS/LEDS_CLASS option probably
falls into the same category, and could also be done as you suggest.
At the moment, this has the problem that both symbols are user
visible and also frequently selected by device drivers (but not others),
which is causing another set of dependency issues.

      Arnd

Re: [PATCH 7/8] fbdev: rework backlight dependencies

[Jani Nikula]

On Fri, 17 Apr 2020, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Arnd.
>
> On Fri, Apr 17, 2020 at 05:55:52PM +0200, Arnd Bergmann wrote:
>> Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE,
>> make any driver that needs it have a dependency on the class device
>> being available, to prevent circular dependencies.
>> 
>> This is the same way that the backlight is already treated for the DRM
>> subsystem.
>
> I am not happy with the direction of this patch.
> It is not easy to understand that one has to enable backlight to
> be allowed to select a display or an fbdev driver.

Arguably that is a problem in Kconfig, and that applies to *all*
dependencies everywhere. It isn't something new to this patch.

For example, in the context of this patch you have:

  config HT16K33
	 tristate "Holtek Ht16K33 LED controller with keyscan"
	 depends on FB && OF && I2C && INPUT
 +	depends on BACKLIGHT_CLASS_DEVICE

The same thing could be said about FB and OF and IC2 and INPUT for
HT16K33, right? Why would *backlight* be the tipping point that makes
this difficult to understand?

Yeah, I agree it's not straightforward, but I think depends is the right
choice, not select.

The effort for making this easier to understand should be directed at
the various menuconfig tools to better highlight the dependencies that
should be enabled to let you enable other options.

> How about somthing like this:

I think this is a hack in Kconfig files that should really be fixed in
the Kconfig tooling instead. IMHO Kconfig should be as simple a
description of the dependencies as possible, not so much a UI language.

FWIW the patch is

Acked-by: Jani Nikula <jani.nikula@intel.com>

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH 8/8] drm/bridge/sii8620: fix extcon dependency

[Arnd Bergmann]

Using 'imply' does not work here, it still cause the same build
failure:

arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'

Now that the framebuffer configuration is cleaned up, we can use the
usual 'depends on EXTCON || !EXTCON' trick.

Fixes: 7a109673899b ("drm/bridge/sii8620: add Kconfig dependency on extcon")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/bridge/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index aaed2347ace9..29c837625f03 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -90,7 +90,7 @@ config DRM_SIL_SII8620
 	tristate "Silicon Image SII8620 HDMI/MHL bridge"
 	depends on OF
 	select DRM_KMS_HELPER
-	imply EXTCON
+	depends on EXTCON || !EXTCON
 	depends on RC_CORE || !RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
-- 
2.26.0

Re: [PATCH 8/8] drm/bridge/sii8620: fix extcon dependency

[Andrzej Hajda]

On 17.04.2020 17:55, Arnd Bergmann wrote:
> Using 'imply' does not work here, it still cause the same build
> failure:
>
> arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
> sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
> arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
> sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
> arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
> arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
> sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'
>
> Now that the framebuffer configuration is cleaned up, we can use the
> usual 'depends on EXTCON || !EXTCON' trick.
>
> Fixes: 7a109673899b ("drm/bridge/sii8620: add Kconfig dependency on extcon")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks for taking care of it.

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


> ---
>  drivers/gpu/drm/bridge/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index aaed2347ace9..29c837625f03 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -90,7 +90,7 @@ config DRM_SIL_SII8620
>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
>  	depends on OF
>  	select DRM_KMS_HELPER
> -	imply EXTCON
> +	depends on EXTCON || !EXTCON
>  	depends on RC_CORE || !RC_CORE
>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Daniel Vetter]

On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> I tried to fix up some dependencies after the sii8620 "imply EXTCON"
> statementn broke, trying a few things but in the backing out a
> change that would completely reverse the LEDS_CLASS selects into
> a 'depends on'. 
> 
> However, what I got now are multiple changes that remove gratious
> "selects" that lead to circular dependencies for sii8620 and others:
> 
> - Anything doing "select FB" is now gone, or becomes "depends on FB",
> 
> - DDC support depends on I2C instead of selecting it
> 
> - backlight class device support is never selected by framebuffer
>   drivers but has proper dependencies
> 
> I have done thousands of randconfig build tests on this, but no
> runtime tests.
> 
> Some of the 'depends on FOO || !FOO' statements could be simplified
> into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
> but I would for the moment treat that as a cleanup that can be done
> later.
> 
> If we can agree on these changes, maybe someone can merge them
> through the drm-misc tree.
> 
> Please review

Biggest concern I have is that usability of make menuconfig is horrible,
and it's very hard to find options that are hidden by depends on. You can
use the search interface, if you happen to know the option.

Once you've surmounted that bar, the next one is trying to find what
exactly you need to enable. Which again means endless of recursive
screaming at Kconfig files, since make menuconfig doesn't help you at all.

That's pretty much why we've never pushed this in, and instead done the
selects. I'm vary applying all this, since after after there'll be all the
screaming again and we have to back it out.

I think to embrace this without regrets what we need is:
- some way to list the hidden options
- some way to browse the depedencies of those hidden options

menuconfig cant do that, gconfig I cant build here (it's some old gtk2
thing, where do you even get the deps for that). xconfig also cant do
this easily, dependencies aren't linked.

So yeah not sure this is a good idea at all, until at least menuconfig can
cope.
-Daniel

> 
>        Arnd
> 
> Arnd Bergmann (8):
>   fbdev: w100fb: clean up mach-pxa compile-time dependency
>   fbdev/ARM: pxa: avoid selecting CONFIG_FB
>   fbdev: rework FB_DDC dependencies
>   drm/rcar: stop using 'imply' for dependencies
>   drm/vmwgfx: make framebuffer support optional
>   drm: decouple from CONFIG_FB
>   fbdev: rework backlight dependencies
>   drm/bridge/sii8620: fix extcon dependency
> 
>  arch/arm/configs/pxa_defconfig      |  3 ++
>  arch/arm/mach-pxa/Kconfig           |  7 ---
>  arch/arm/mach-pxa/eseries.c         | 14 +----
>  arch/arm/mach-pxa/saar.c            |  2 +-
>  arch/arm/mach-pxa/tavorevb.c        |  2 +-
>  drivers/auxdisplay/Kconfig          |  1 +
>  drivers/gpu/drm/Kconfig             |  5 +-
>  drivers/gpu/drm/bridge/Kconfig      |  2 +-
>  drivers/gpu/drm/mxsfb/Kconfig       |  1 -
>  drivers/gpu/drm/rcar-du/Kconfig     | 23 +++++---
>  drivers/gpu/drm/vmwgfx/Kconfig      | 17 +++---
>  drivers/gpu/drm/vmwgfx/Makefile     |  4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 35 +++++++-----
>  drivers/gpu/drm/zte/Kconfig         |  1 -
>  drivers/macintosh/Kconfig           |  1 +
>  drivers/staging/fbtft/Kconfig       |  1 +
>  drivers/staging/olpc_dcon/Kconfig   |  2 +-
>  drivers/video/fbdev/Kconfig         | 31 ++++++++---
>  drivers/video/fbdev/w100fb.c        | 84 +++++------------------------
>  include/video/w100fb.h              |  6 +--
>  20 files changed, 101 insertions(+), 141 deletions(-)
> 
> -- 
> 2.26.0
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: <masahiroy@kernel.org>
> Cc: <Laurent.pinchart@ideasonboard.com>
> Cc: <linux-renesas-soc@vger.kernel.org>,
> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
> Cc: <kieran.bingham+renesas@ideasonboard.com>,
> Cc: <airlied@linux.ie>
> Cc: daniel@zonque.org
> Cc: haojian.zhuang@gmail.com
> Cc: robert.jarzmik@free.fr
> Cc: daniel@ffwll.ch
> Cc: marex@denx.de
> Cc: stefan@agner.ch
> Cc: linux-graphics-maintainer@vmware.com
> Cc: thellstrom@vmware.com
> Cc: jfrederich@gmail.com
> Cc: dsd@laptop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Cc: geert@linux-m68k.org

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Jason Gunthorpe]

On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> > I tried to fix up some dependencies after the sii8620 "imply EXTCON"
> > statementn broke, trying a few things but in the backing out a
> > change that would completely reverse the LEDS_CLASS selects into
> > a 'depends on'. 
> > 
> > However, what I got now are multiple changes that remove gratious
> > "selects" that lead to circular dependencies for sii8620 and others:
> > 
> > - Anything doing "select FB" is now gone, or becomes "depends on FB",
> > 
> > - DDC support depends on I2C instead of selecting it
> > 
> > - backlight class device support is never selected by framebuffer
> >   drivers but has proper dependencies
> > 
> > I have done thousands of randconfig build tests on this, but no
> > runtime tests.
> > 
> > Some of the 'depends on FOO || !FOO' statements could be simplified
> > into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
> > but I would for the moment treat that as a cleanup that can be done
> > later.
> > 
> > If we can agree on these changes, maybe someone can merge them
> > through the drm-misc tree.
> > 
> > Please review
> 
> Biggest concern I have is that usability of make menuconfig is horrible,
> and it's very hard to find options that are hidden by depends on. You can
> use the search interface, if you happen to know the option.
> 
> Once you've surmounted that bar, the next one is trying to find what
> exactly you need to enable. Which again means endless of recursive
> screaming at Kconfig files, since make menuconfig doesn't help you at all.

+1 on this. But this is a general kconfig problem, and not unique to
DRM, I've done this screaming for many different things now.. eg to
turn on every single RDMA driver.

I hackily delt with it by creating this rather insane script based on
the python kconfiglib to try and sort things out mostly automatically:

https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_kconfig.py

It would be great if menuconfig had a key to say 'hey, really, turn
this on and everything it depends on, recursively'

Jason

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Jani Nikula]

On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
>> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
>> > I tried to fix up some dependencies after the sii8620 "imply EXTCON"
>> > statementn broke, trying a few things but in the backing out a
>> > change that would completely reverse the LEDS_CLASS selects into
>> > a 'depends on'. 
>> > 
>> > However, what I got now are multiple changes that remove gratious
>> > "selects" that lead to circular dependencies for sii8620 and others:
>> > 
>> > - Anything doing "select FB" is now gone, or becomes "depends on FB",
>> > 
>> > - DDC support depends on I2C instead of selecting it
>> > 
>> > - backlight class device support is never selected by framebuffer
>> >   drivers but has proper dependencies
>> > 
>> > I have done thousands of randconfig build tests on this, but no
>> > runtime tests.
>> > 
>> > Some of the 'depends on FOO || !FOO' statements could be simplified
>> > into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
>> > but I would for the moment treat that as a cleanup that can be done
>> > later.
>> > 
>> > If we can agree on these changes, maybe someone can merge them
>> > through the drm-misc tree.
>> > 
>> > Please review
>> 
>> Biggest concern I have is that usability of make menuconfig is horrible,
>> and it's very hard to find options that are hidden by depends on. You can
>> use the search interface, if you happen to know the option.
>> 
>> Once you've surmounted that bar, the next one is trying to find what
>> exactly you need to enable. Which again means endless of recursive
>> screaming at Kconfig files, since make menuconfig doesn't help you at all.
>
> +1 on this. But this is a general kconfig problem, and not unique to
> DRM, I've done this screaming for many different things now.. eg to
> turn on every single RDMA driver.
>
> I hackily delt with it by creating this rather insane script based on
> the python kconfiglib to try and sort things out mostly automatically:
>
> https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_kconfig.py
>
> It would be great if menuconfig had a key to say 'hey, really, turn
> this on and everything it depends on, recursively'

I'm really all for switching to using depends when that is the
semantically right thing to do. In many places using select is a hack to
make the UI simpler, and that's just plain wrong. We'll be doomed to
perpetual randconfig build failures and duct tape fixes.

I'm pretty tired of this, and I regularly ignore those duct tape fixes
to i915 backlight build issues on some bizarre configs that nobody will
ever use, and would not exist if depends were used throughout.

I'm fine with select but only when it's restricted to symbols that have
no dependencies of their own and have no UI. This is in line with
Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
Kconfig tool shortcoming.

See also my reply to Sam [1].

BR,
Jani.


[1] https://lore.kernel.org/dri-devel/871roi37qe.fsf@intel.com/


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> >> >
> >> > If we can agree on these changes, maybe someone can merge them
> >> > through the drm-misc tree.
> >> >
> >> > Please review
> >>
> >> Biggest concern I have is that usability of make menuconfig is horrible,

No doubt about that, but that seems to be unrelated to the cleanup.

> >> and it's very hard to find options that are hidden by depends on. You can
> >> use the search interface, if you happen to know the option.
> >>
> >> Once you've surmounted that bar, the next one is trying to find what
> >> exactly you need to enable. Which again means endless of recursive
> >> screaming at Kconfig files, since make menuconfig doesn't help you at all.

The changes I'm doing are mostly for fbdev, which is currently the
odd one out. Most kernel subsystems today follow the documented
recommendations and only use 'depends on' for things they
depend on.

Having fbdev be the exception causes two problems:

- It does not make kconfig any easier to use overall, just less consistent
  when it is the only thing that implicitly turns on dependencies and
  for everything else one still has to look up what the dependencies are.

- Most of the problems with circular dependencies come from mixing
  the two methods, and most of the cases where they have caused
  problems in the past involve fbdev in some way.

I also doubt switching lots of 'depends on' to 'select' all over Kconfig
would improve the situation on a global level. It would simplify the
problem of turning something on without understanding the what it
does, but in turn it makes it harder to turn off something else.

E.g. today it is hard to turn off fbdev because that is selected by a
number of (partly unrelated) options, but there was a recent discussion
about getting distros to stop enabling fbdev out of security concerns.

> I'm really all for switching to using depends when that is the
> semantically right thing to do. In many places using select is a hack to
> make the UI simpler, and that's just plain wrong. We'll be doomed to
> perpetual randconfig build failures and duct tape fixes.
>
> I'm pretty tired of this, and I regularly ignore those duct tape fixes
> to i915 backlight build issues on some bizarre configs that nobody will
> ever use, and would not exist if depends were used throughout.
>
> I'm fine with select but only when it's restricted to symbols that have
> no dependencies of their own and have no UI. This is in line with
> Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
> Kconfig tool shortcoming.

Agreed, that is generally a good rule.

      Arnd

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Daniel Vetter]

On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> > >> >
> > >> > If we can agree on these changes, maybe someone can merge them
> > >> > through the drm-misc tree.
> > >> >
> > >> > Please review
> > >>
> > >> Biggest concern I have is that usability of make menuconfig is horrible,
> 
> No doubt about that, but that seems to be unrelated to the cleanup.
> 
> > >> and it's very hard to find options that are hidden by depends on. You can
> > >> use the search interface, if you happen to know the option.
> > >>
> > >> Once you've surmounted that bar, the next one is trying to find what
> > >> exactly you need to enable. Which again means endless of recursive
> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
> 
> The changes I'm doing are mostly for fbdev, which is currently the
> odd one out. Most kernel subsystems today follow the documented
> recommendations and only use 'depends on' for things they
> depend on.
> 
> Having fbdev be the exception causes two problems:
> 
> - It does not make kconfig any easier to use overall, just less consistent
>   when it is the only thing that implicitly turns on dependencies and
>   for everything else one still has to look up what the dependencies are.
> 
> - Most of the problems with circular dependencies come from mixing
>   the two methods, and most of the cases where they have caused
>   problems in the past involve fbdev in some way.
> 
> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
> would improve the situation on a global level. It would simplify the
> problem of turning something on without understanding the what it
> does, but in turn it makes it harder to turn off something else.
> 
> E.g. today it is hard to turn off fbdev because that is selected by a
> number of (partly unrelated) options, but there was a recent discussion
> about getting distros to stop enabling fbdev out of security concerns.

I've done some history digging, this is the patch that started this all:

commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Feb 5 16:03:34 2009 +0100

    drm/i915: select framebuffer support automatically

I.e. driver gets disabled because a new config is added which isn't
enabled. System doesn't boot, maintainer gets angry regression report,
select hack gets added.

Note on the specific example the code has been reworked enough that even
if you'd upgrade the kernel all that would get disabled now is the fbdev
emulation on top of drm drivers, not any of the drm drivers.

The above says we should have an automatic system for at least oldconfig
(but would be nice in menuconfig too), since "break user's kernel on
upgrade" isn't an option. And without that select is going to come back
somewhere and make a huge nasty mess: We're definitely not going to
fix Kconfig when fixing a regression in -rc kernels.

So in theory no need to convince me that select is terrible. Practice
disagrees unfortunately.
-Daniel

> 
> > I'm really all for switching to using depends when that is the
> > semantically right thing to do. In many places using select is a hack to
> > make the UI simpler, and that's just plain wrong. We'll be doomed to
> > perpetual randconfig build failures and duct tape fixes.
> >
> > I'm pretty tired of this, and I regularly ignore those duct tape fixes
> > to i915 backlight build issues on some bizarre configs that nobody will
> > ever use, and would not exist if depends were used throughout.
> >
> > I'm fine with select but only when it's restricted to symbols that have
> > no dependencies of their own and have no UI. This is in line with
> > Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
> > Kconfig tool shortcoming.
> 
> Agreed, that is generally a good rule.
> 
>       Arnd

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Jani Nikula]

On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
>> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
>> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
>> > >> >
>> > >> > If we can agree on these changes, maybe someone can merge them
>> > >> > through the drm-misc tree.
>> > >> >
>> > >> > Please review
>> > >>
>> > >> Biggest concern I have is that usability of make menuconfig is horrible,
>> 
>> No doubt about that, but that seems to be unrelated to the cleanup.
>> 
>> > >> and it's very hard to find options that are hidden by depends on. You can
>> > >> use the search interface, if you happen to know the option.
>> > >>
>> > >> Once you've surmounted that bar, the next one is trying to find what
>> > >> exactly you need to enable. Which again means endless of recursive
>> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
>> 
>> The changes I'm doing are mostly for fbdev, which is currently the
>> odd one out. Most kernel subsystems today follow the documented
>> recommendations and only use 'depends on' for things they
>> depend on.
>> 
>> Having fbdev be the exception causes two problems:
>> 
>> - It does not make kconfig any easier to use overall, just less consistent
>>   when it is the only thing that implicitly turns on dependencies and
>>   for everything else one still has to look up what the dependencies are.
>> 
>> - Most of the problems with circular dependencies come from mixing
>>   the two methods, and most of the cases where they have caused
>>   problems in the past involve fbdev in some way.
>> 
>> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
>> would improve the situation on a global level. It would simplify the
>> problem of turning something on without understanding the what it
>> does, but in turn it makes it harder to turn off something else.
>> 
>> E.g. today it is hard to turn off fbdev because that is selected by a
>> number of (partly unrelated) options, but there was a recent discussion
>> about getting distros to stop enabling fbdev out of security concerns.
>
> I've done some history digging, this is the patch that started this all:
>
> commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Thu Feb 5 16:03:34 2009 +0100
>
>     drm/i915: select framebuffer support automatically
>
> I.e. driver gets disabled because a new config is added which isn't
> enabled. System doesn't boot, maintainer gets angry regression report,
> select hack gets added.

Gotta love a good commit message from a decade ago.

First, it says it's a migration helper. And that the problem
specifically is that the user has a working config *without* FB enabled
as a starting point.

Now, if the starting point for a new config *now* is less than ten years
old, and it had i915 enabled, it'll also have FB enabled. Because
select. The migration part has done its job, and I think we should be
good to make some progress.

The commit message also notes the problems of select.

BR,
Jani.


> Note on the specific example the code has been reworked enough that even
> if you'd upgrade the kernel all that would get disabled now is the fbdev
> emulation on top of drm drivers, not any of the drm drivers.
>
> The above says we should have an automatic system for at least oldconfig
> (but would be nice in menuconfig too), since "break user's kernel on
> upgrade" isn't an option. And without that select is going to come back
> somewhere and make a huge nasty mess: We're definitely not going to
> fix Kconfig when fixing a regression in -rc kernels.
>
> So in theory no need to convince me that select is terrible. Practice
> disagrees unfortunately.
> -Daniel
>
>> 
>> > I'm really all for switching to using depends when that is the
>> > semantically right thing to do. In many places using select is a hack to
>> > make the UI simpler, and that's just plain wrong. We'll be doomed to
>> > perpetual randconfig build failures and duct tape fixes.
>> >
>> > I'm pretty tired of this, and I regularly ignore those duct tape fixes
>> > to i915 backlight build issues on some bizarre configs that nobody will
>> > ever use, and would not exist if depends were used throughout.
>> >
>> > I'm fine with select but only when it's restricted to symbols that have
>> > no dependencies of their own and have no UI. This is in line with
>> > Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
>> > Kconfig tool shortcoming.
>> 
>> Agreed, that is generally a good rule.
>> 
>>       Arnd

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Geert Uytterhoeven]

Hi Jani,

On Tue, Apr 21, 2020 at 2:58 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
> >> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
> >> <jani.nikula@linux.intel.com> wrote:
> >> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> >> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> >> > >> >
> >> > >> > If we can agree on these changes, maybe someone can merge them
> >> > >> > through the drm-misc tree.
> >> > >> >
> >> > >> > Please review
> >> > >>
> >> > >> Biggest concern I have is that usability of make menuconfig is horrible,
> >>
> >> No doubt about that, but that seems to be unrelated to the cleanup.
> >>
> >> > >> and it's very hard to find options that are hidden by depends on. You can
> >> > >> use the search interface, if you happen to know the option.
> >> > >>
> >> > >> Once you've surmounted that bar, the next one is trying to find what
> >> > >> exactly you need to enable. Which again means endless of recursive
> >> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
> >>
> >> The changes I'm doing are mostly for fbdev, which is currently the
> >> odd one out. Most kernel subsystems today follow the documented
> >> recommendations and only use 'depends on' for things they
> >> depend on.
> >>
> >> Having fbdev be the exception causes two problems:
> >>
> >> - It does not make kconfig any easier to use overall, just less consistent
> >>   when it is the only thing that implicitly turns on dependencies and
> >>   for everything else one still has to look up what the dependencies are.
> >>
> >> - Most of the problems with circular dependencies come from mixing
> >>   the two methods, and most of the cases where they have caused
> >>   problems in the past involve fbdev in some way.
> >>
> >> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
> >> would improve the situation on a global level. It would simplify the
> >> problem of turning something on without understanding the what it
> >> does, but in turn it makes it harder to turn off something else.
> >>
> >> E.g. today it is hard to turn off fbdev because that is selected by a
> >> number of (partly unrelated) options, but there was a recent discussion
> >> about getting distros to stop enabling fbdev out of security concerns.
> >
> > I've done some history digging, this is the patch that started this all:
> >
> > commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
> > Author: Ingo Molnar <mingo@elte.hu>
> > Date:   Thu Feb 5 16:03:34 2009 +0100
> >
> >     drm/i915: select framebuffer support automatically
> >
> > I.e. driver gets disabled because a new config is added which isn't
> > enabled. System doesn't boot, maintainer gets angry regression report,
> > select hack gets added.
>
> Gotta love a good commit message from a decade ago.
>
> First, it says it's a migration helper. And that the problem
> specifically is that the user has a working config *without* FB enabled
> as a starting point.
>
> Now, if the starting point for a new config *now* is less than ten years
> old, and it had i915 enabled, it'll also have FB enabled. Because
> select. The migration part has done its job, and I think we should be
> good to make some progress.

It will indeed work with "make oldconfig", as an old config with
CONFIG_DRM_I915 enabled will have CONFIG_FB set.

However, that is not true when starting with a defconfig that has
CONFIG_DRM_I915 enabled: such a defconfig will not have CONFIG_FB set,
due to the trimming process when creating a minimal defconfig.

Hence when making the change from "select FB" to "depends on FB", you
have to make sure to update the affected defconfigs, too:

$ git grep CONFIG_DRM_I915 -- "*defconfig*"
arch/x86/configs/i386_defconfig:CONFIG_DRM_I915=y
arch/x86/configs/x86_64_defconfig:CONFIG_DRM_I915=y

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Daniel Vetter]

On Tue, Apr 21, 2020 at 3:05 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Jani,
>
> On Tue, Apr 21, 2020 at 2:58 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
> > >> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
> > >> <jani.nikula@linux.intel.com> wrote:
> > >> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> > >> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> > >> > >> >
> > >> > >> > If we can agree on these changes, maybe someone can merge them
> > >> > >> > through the drm-misc tree.
> > >> > >> >
> > >> > >> > Please review
> > >> > >>
> > >> > >> Biggest concern I have is that usability of make menuconfig is horrible,
> > >>
> > >> No doubt about that, but that seems to be unrelated to the cleanup.
> > >>
> > >> > >> and it's very hard to find options that are hidden by depends on. You can
> > >> > >> use the search interface, if you happen to know the option.
> > >> > >>
> > >> > >> Once you've surmounted that bar, the next one is trying to find what
> > >> > >> exactly you need to enable. Which again means endless of recursive
> > >> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
> > >>
> > >> The changes I'm doing are mostly for fbdev, which is currently the
> > >> odd one out. Most kernel subsystems today follow the documented
> > >> recommendations and only use 'depends on' for things they
> > >> depend on.
> > >>
> > >> Having fbdev be the exception causes two problems:
> > >>
> > >> - It does not make kconfig any easier to use overall, just less consistent
> > >>   when it is the only thing that implicitly turns on dependencies and
> > >>   for everything else one still has to look up what the dependencies are.
> > >>
> > >> - Most of the problems with circular dependencies come from mixing
> > >>   the two methods, and most of the cases where they have caused
> > >>   problems in the past involve fbdev in some way.
> > >>
> > >> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
> > >> would improve the situation on a global level. It would simplify the
> > >> problem of turning something on without understanding the what it
> > >> does, but in turn it makes it harder to turn off something else.
> > >>
> > >> E.g. today it is hard to turn off fbdev because that is selected by a
> > >> number of (partly unrelated) options, but there was a recent discussion
> > >> about getting distros to stop enabling fbdev out of security concerns.
> > >
> > > I've done some history digging, this is the patch that started this all:
> > >
> > > commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
> > > Author: Ingo Molnar <mingo@elte.hu>
> > > Date:   Thu Feb 5 16:03:34 2009 +0100
> > >
> > >     drm/i915: select framebuffer support automatically
> > >
> > > I.e. driver gets disabled because a new config is added which isn't
> > > enabled. System doesn't boot, maintainer gets angry regression report,
> > > select hack gets added.
> >
> > Gotta love a good commit message from a decade ago.
> >
> > First, it says it's a migration helper. And that the problem
> > specifically is that the user has a working config *without* FB enabled
> > as a starting point.
> >
> > Now, if the starting point for a new config *now* is less than ten years
> > old, and it had i915 enabled, it'll also have FB enabled. Because
> > select. The migration part has done its job, and I think we should be
> > good to make some progress.
>
> It will indeed work with "make oldconfig", as an old config with
> CONFIG_DRM_I915 enabled will have CONFIG_FB set.
>
> However, that is not true when starting with a defconfig that has
> CONFIG_DRM_I915 enabled: such a defconfig will not have CONFIG_FB set,
> due to the trimming process when creating a minimal defconfig.
>
> Hence when making the change from "select FB" to "depends on FB", you
> have to make sure to update the affected defconfigs, too:
>
> $ git grep CONFIG_DRM_I915 -- "*defconfig*"
> arch/x86/configs/i386_defconfig:CONFIG_DRM_I915=y
> arch/x86/configs/x86_64_defconfig:CONFIG_DRM_I915=y

To clarify what I was aiming for with my mail: I'm not worried about
fbdev here, I'm just worried that this will come back, and we'll grow
select somewhere else until it's become a big & totally horrible mess.
I think a lot of the backlight selects have also grown because of
this, so this isn't just a one-off I think.

If Arnd is happy to play "Kconfig select" whack-a-mole ever once in a
while (and deal with the intermediate compile horrors while everyone
upgrades) I'm ok with this landing. Just not terribly happy if the
underlying issue isn't fixed.
-Daniel

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/8] drm, fbdev: rework dependencies

[Jani Nikula]

On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> To clarify what I was aiming for with my mail: I'm not worried about
> fbdev here, I'm just worried that this will come back, and we'll grow
> select somewhere else until it's become a big & totally horrible mess.
> I think a lot of the backlight selects have also grown because of
> this, so this isn't just a one-off I think.
>
> If Arnd is happy to play "Kconfig select" whack-a-mole ever once in a
> while (and deal with the intermediate compile horrors while everyone
> upgrades) I'm ok with this landing. Just not terribly happy if the
> underlying issue isn't fixed.

And I'll keep ignoring the IS_REACHABLE() patches that make i915 build
with a config that should not exist. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center