Re: [Bugme-new] [Bug 9847] New: i810fb: module parameter 'mode_option' inconsistent with other framebuffer modules

On Tue, 29 Jan 2008 21:02:39 -0800 (PST) bugme-daemon@bugzilla.kernel.org
wrote:

quoted

Problem Description: While all other framebuffer modules use the option
"mode=..." to specify a modedb-style video mode, i810fb uses the

non-standard

quoted

"mode_option=..." 

The attached patch should fix it.
--

From: Krzysztof Helt <redacted>

This patch changes the option "mode_option" into "mode".

Signed-off-by: Krzysztof Helt <redacted>

---

If the patch below is mangled by my web based email client please use the attached one.

The patch was done against 2.6.24-git9.


diff -urp linux-ref/drivers/video/i810/i810_main.c linux-pc/drivers/video/i810/i810_main.c

--- linux-ref/drivers/video/i810/i810_main.c	2008-01-24 23:58:37.000000000 +0100
+++ linux-pc/drivers/video/i810/i810_main.c	2008-01-31 20:43:56.000000000 +0100

@@ -132,7 +132,7 @@ static struct pci_driver i810fb_driver =
 	.resume   =     i810fb_resume,
 };
 
-static char *mode_option __devinitdata = NULL;
+static char *mode     __devinitdata;
 static int vram       __devinitdata = 4;
 static int bpp        __devinitdata = 8;
 static int mtrr       __devinitdata;

@@ -1891,7 +1891,7 @@ i810_allocate_pci_resource(struct i810fb
 
 static void __devinit i810fb_find_init_mode(struct fb_info *info)
 {
-	struct fb_videomode mode;
+	struct fb_videomode mode_new;
 	struct fb_var_screeninfo var;
 	struct fb_monspecs *specs = &info->monspecs;
 	int found = 0;

@@ -1902,7 +1902,7 @@ static void __devinit i810fb_find_init_m
 #endif
 
 	INIT_LIST_HEAD(&info->modelist);
-	memset(&mode, 0, sizeof(struct fb_videomode));
+	memset(&mode_new, 0, sizeof(struct fb_videomode));
 	var = info->var;
 #ifdef CONFIG_FB_I810_I2C
 	i810_create_i2c_busses(par);

@@ -1928,23 +1928,23 @@ static void __devinit i810fb_find_init_m
 
 		if (xres && yres) {
 			if ((m = fb_find_best_mode(&var, &info->modelist))) {
-				mode = *m;
+				mode_new = *m;
 				found  = 1;
 			}
 		}
 
 		if (!found) {
 			m = fb_find_best_display(&info->monspecs, &info->modelist);
-			mode = *m;
+			mode_new = *m;
 			found = 1;
 		}
 
-		fb_videomode_to_var(&var, &mode);
+		fb_videomode_to_var(&var, &mode_new);
 	}
 #endif
-	if (mode_option)
-		fb_find_mode(&var, info, mode_option, specs->modedb,
-			     specs->modedb_len, (found) ? &mode : NULL,
+	if (mode)
+		fb_find_mode(&var, info, mode, specs->modedb,
+			     specs->modedb_len, (found) ? &mode_new : NULL,
 			     info->var.bits_per_pixel);
 
 	info->var = var;

@@ -1998,7 +1998,7 @@ static int __devinit i810fb_setup(char *
 		else if (!strncmp(this_opt, "ddc3", 4))
 			ddc3 = 3;
 		else
-			mode_option = this_opt;
+			mode = this_opt;
 	}
 	return 0;
 }

@@ -2202,8 +2202,8 @@ MODULE_PARM_DESC(dcolor, "use DirectColo
 		 " (default = 0 = TrueColor)");
 module_param(ddc3, bool, 0);
 MODULE_PARM_DESC(ddc3, "Probe DDC bus 3 (default = 0 = no)");
-module_param(mode_option, charp, 0);
-MODULE_PARM_DESC(mode_option, "Specify initial video mode");
+module_param(mode, charp, 0);
+MODULE_PARM_DESC(mode, "Specify initial video mode");
 
 MODULE_AUTHOR("Tony A. Daplas");
 MODULE_DESCRIPTION("Framebuffer device for the Intel 810/815 and"


----------------------------------------------------------------------

Lezac w wannie skorzystala z ...
Kliknij >>> http://link.interia.pl/f1ce9 

On 01 Feb 2008 08:01:30 +0100 krzysztof.h1@poczta.fm wrote:

quoted

On Tue, 29 Jan 2008 21:02:39 -0800 (PST) bugme-daemon@bugzilla.kernel.org
wrote:

quoted

Problem Description: While all other framebuffer modules use the option
"mode=..." to specify a modedb-style video mode, i810fb uses the

non-standard

quoted

"mode_option=..." 

The attached patch should fix it.
--

From: Krzysztof Helt <redacted>

This patch changes the option "mode_option" into "mode".

Thanks.

But can we retain the old option as well, to avoid breaking people's
existing setups?


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

On Thu, 2008-01-31 at 23:32 -0800, Andrew Morton wrote:

On 01 Feb 2008 08:01:30 +0100 krzysztof.h1@poczta.fm wrote:

quoted

quoted

On Tue, 29 Jan 2008 21:02:39 -0800 (PST) bugme-daemon@bugzilla.kernel.org
wrote:

quoted

Problem Description: While all other framebuffer modules use the option
"mode=..." to specify a modedb-style video mode, i810fb uses the

non-standard

quoted

"mode_option=..." 

The attached patch should fix it.
--

From: Krzysztof Helt <redacted>

This patch changes the option "mode_option" into "mode".

Thanks.

quoted

-module_param(mode_option, charp, 0);
-MODULE_PARM_DESC(mode_option, "Specify initial video mode");
+module_param(mode, charp, 0);
+MODULE_PARM_DESC(mode, "Specify initial video mode");

But can we retain the old option as well, to avoid breaking people's
existing setups?

Thanks for the quick reply and patch. In the meantime, I have found out
that the option inconsistency is far more widespread among framebuffer
modules. The documentation on video mode setting
(Documentation/fb/modedb.txt) states that the mode parameter should be
called 'mode_option', but the framebuffer modules seem to be split
exactly down the middle in using 'mode' versus 'mode_option'

On a Debian lenny/sid system running a 2.6.22 kernel I get this:

Framebuffer modules accepting a 'mode' parameter:
---[cut here]---
iki@miki:~$ find /lib/modules/`uname -r`/kernel -name '*fb.ko'|while
read ko; do sudo modinfo -F parm $ko|grep -q "^mode:" && echo $ko; done
/lib/modules/2.6.22-3-686/kernel/drivers/video/s3fb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/sis/sisfb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/arkfb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/geode/gx1fb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/vt8623fb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/intelfb/intelfb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/pm2fb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/aty/atyfb.ko
---

Framebuffer modules accepting a 'mode_option' parameter:
miki@miki:~$ find /lib/modules/`uname -r`/kernel -name '*fb.ko'|while
read ko; do sudo modinfo -F parm $ko|grep -q "^mode_option:" && echo
$ko; done
/lib/modules/2.6.22-3-686/kernel/drivers/video/savage/savagefb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/sstfb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/neofb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/geode/gxfb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/i810/i810fb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/nvidia/nvidiafb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/aty/aty128fb.ko
/lib/modules/2.6.22-3-686/kernel/drivers/video/aty/radeonfb.ko

I assume the parameter was called 'mode_option' originally to indicate
that the parameter name itself is optional, "modprobe i810fb
mode=1024x768" is equivalent to "modprobe i810fb 1024x768", which at
least this module seems to support.

Should this be filed as a more general bug report?

-Alain

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

On Sat, 02 Feb 2008 05:18:10 +0100 Alain Kalker [off-list ref] wrote:

Should this be filed as a more general bug report?

It is unlikely that anyone would actually act upon such a thing, I'm afraid.
Maybe if we could come up with a single, centralised option parser
for all the drivers and then migrate individual drivers over to or
or something like that..

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

On Fri, 2008-02-01 at 20:37 -0800, Andrew Morton wrote:

On Sat, 02 Feb 2008 05:18:10 +0100 Alain Kalker [off-list ref] wrote:

quoted

Should this be filed as a more general bug report?

It is unlikely that anyone would actually act upon such a thing, I'm afraid.
Maybe if we could come up with a single, centralised option parser
for all the drivers and then migrate individual drivers over to or
or something like that..

The reason I asked is that the current situation presents a problem to
the boot scripts. Currently none of the modules support both options
simultaneously, and I'm not sure if every module supports the parameter
name being optional. The only sure ways to ensure that kernel boot
option "video=<framebuffer>:<videomode> works as intended, is either to
parse the modules' option list using modinfo (which may not be available
at boot) or to try each option name in turn using "modprobe
--first-time", which I find very kludgy.

-Alain

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/