Patch to framebuffer

Patch to framebuffer

[Nathan Cline]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

Hello, this is my first time posting to this list so please forgive me
if I'm violating protocol in some way. :)  I've written a patch to the
framebuffer code to modify its behavior a bit. I am running on a
dual-headed system and I noticed when I was working in one console on
one monitor, the console on the other monitor was "frozen", not
updating itself. After some digging through the code I realized this
is because the two framebuffer drivers share the same framebuffer code
which stores a single pointer to the "current" virtual console. If a
VC is not current it is considered invisible and is not updated. So I
patched the code to store a pointer for each framebuffer to the
"foreground" VC on each one. It seems to work well but I'd like to get
others' input as this is my first time writing any kernel code, and to
be honest there is so much code it's difficult to get a clear picture
in my head of how the whole system works.
I've attached the patches to this message, a small one to
drivers/video/console/fbcon.h, and a larger one to fbcon.c. I would
appreciate it if some of you guys could look over this code and look
for any obvious errors, or better yet, hopefully someone else has a
multihead system they can try it on. The patches are against the
latest GIT source tree (torvalds, main branch, 2.6.15rc2) as of last
night.
Any replies to this message should be CC'ed directly to my e-mail
account (nathan dot cline .. gmail dot com) as I am not currently
subscribed to this list. I look forward to getting this patch of high
enough quality to submit to Linus. Thanks!

[-- Attachment #2: fbcon.c.patch --]
[-- Type: text/plain, Size: 4184 bytes --]

--- fbcon.c	2005-11-23 23:49:10.000000000 -0600
+++ fbcon.c.mine	2005-11-23 23:12:16.000000000 -0600
@@ -390,12 +390,15 @@
 	int mode;
 
 	if (ops->currcon != -1)
-		vc = vc_cons[ops->currcon].d;
+		vc = ops->currcon_ptr; 
 
-	if (!vc || !CON_IS_VISIBLE(vc) ||
+	if (!vc)
+		return;
+
+	if (!CON_IS_VISIBLE(vc) ||
 	    fbcon_is_inactive(vc, info) ||
  	    registered_fb[con2fb_map[vc->vc_num]] != info ||
-	    vc_cons[ops->currcon].d->vc_deccm != 1)
+	    vc->vc_deccm != 1)
 		return;
 	acquire_console_sem();
 	p = &fb_display[vc->vc_num];
@@ -753,6 +756,7 @@
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	ops->currcon = fg_console;
+	ops->currcon_ptr = vc_cons[fg_console].d;
 
 	if (info->fbops->fb_set_par && !(ops->flags & FBCON_FLAGS_INIT))
 		info->fbops->fb_set_par(info);
@@ -766,7 +770,7 @@
 		fbcon_preset_disp(info, &info->var, unit);
 
 	if (show_logo) {
-		struct vc_data *fg_vc = vc_cons[fg_console].d;
+		struct vc_data *fg_vc = ops->currcon_ptr;
 		struct fb_info *fg_info =
 			registered_fb[con2fb_map[fg_console]];
 
@@ -775,7 +779,7 @@
 				   fg_vc->vc_rows);
 	}
 
-	update_screen(vc_cons[fg_console].d);
+	update_screen(ops->currcon_ptr);
 }
 
 /**
@@ -929,6 +933,7 @@
 
 	memset(ops, 0, sizeof(struct fbcon_ops));
 	ops->currcon = -1;
+	ops->currcon_ptr = NULL;
 	ops->graphics = 1;
 	ops->cur_rotate = -1;
 	info->fbcon_par = ops;
@@ -1055,6 +1060,15 @@
 	    return;
 
 	cap = info->flags;
+	ops = info->fbcon_par;
+
+	if (ops->currcon == -1)
+	{
+		ops->currcon = vc->vc_num;
+		ops->currcon_ptr = vc;
+	}
+
+	vc->vc_display_fg = &(ops->currcon_ptr);
 
 	if (vc != svc || logo_shown == FBCON_LOGO_DONTSHOW ||
 	    (info->fix.type == FB_TYPE_TEXT))
@@ -1091,7 +1105,6 @@
 	if (!*vc->vc_uni_pagedir_loc)
 		con_copy_unimap(vc, svc);
 
-	ops = info->fbcon_par;
 	p->con_rotate = rotate;
 	set_blitting_type(vc, info, NULL);
 
@@ -1296,6 +1309,8 @@
 	struct fbcon_ops *ops = info->fbcon_par;
 	int rows, cols, charcnt = 256;
 
+	vc->vc_display_fg = &(ops->currcon_ptr);
+	
 	if (var_to_display(p, var, info))
 		return;
 	t = &fb_display[svc->vc_num];
@@ -2048,7 +2063,7 @@
 	struct fbcon_ops *ops;
 	struct display *p = &fb_display[vc->vc_num];
 	struct fb_var_screeninfo var;
-	int i, prev_console;
+	int prev_console;
 
 	info = registered_fb[con2fb_map[vc->vc_num]];
 	ops = info->fbcon_par;
@@ -2073,21 +2088,10 @@
 	prev_console = ops->currcon;
 	if (prev_console != -1)
 		old_info = registered_fb[con2fb_map[prev_console]];
-	/*
-	 * FIXME: If we have multiple fbdev's loaded, we need to
-	 * update all info->currcon.  Perhaps, we can place this
-	 * in a centralized structure, but this might break some
-	 * drivers.
-	 *
-	 * info->currcon = vc->vc_num;
-	 */
-	for (i = 0; i < FB_MAX; i++) {
-		if (registered_fb[i] != NULL && registered_fb[i]->fbcon_par) {
-			struct fbcon_ops *o = registered_fb[i]->fbcon_par;
 
-			o->currcon = vc->vc_num;
-		}
-	}
+	ops->currcon = vc->vc_num;
+	ops->currcon_ptr = vc;
+	
 	memset(&var, 0, sizeof(struct fb_var_screeninfo));
 	display_to_var(&var, p);
 	var.activate = FB_ACTIVATE_NOW;
@@ -2103,13 +2107,6 @@
 	fb_set_var(info, &var);
 	ops->var = info->var;
 
-	if (old_info != NULL && old_info != info) {
-		if (info->fbops->fb_set_par)
-			info->fbops->fb_set_par(info);
-		fbcon_del_cursor_timer(old_info);
-		fbcon_add_cursor_timer(info);
-	}
-
 	set_blitting_type(vc, info, p);
 	ops->cursor_reset = 1;
 
@@ -2691,7 +2688,7 @@
 
 	if (!ops || ops->currcon < 0)
 		return;
-	vc = vc_cons[ops->currcon].d;
+	vc = ops->currcon_ptr;
 
 	/* Clear cursor, restore saved data */
 	fbcon_cursor(vc, CM_ERASE);
@@ -2704,7 +2701,7 @@
 
 	if (!ops || ops->currcon < 0)
 		return;
-	vc = vc_cons[ops->currcon].d;
+	vc = ops->currcon_ptr; 
 
 	update_screen(vc);
 }
@@ -2718,7 +2715,7 @@
 
 	if (!ops || ops->currcon < 0)
 		return;
-	vc = vc_cons[ops->currcon].d;
+	vc = ops->currcon_ptr; 
 	if (vc->vc_mode != KD_TEXT ||
 	    registered_fb[con2fb_map[ops->currcon]] != info)
 		return;
@@ -2841,7 +2838,7 @@
 	if (!ops || ops->currcon < 0)
 		return;
 
-	vc = vc_cons[ops->currcon].d;
+	vc = ops->currcon_ptr; 
 	if (vc->vc_mode != KD_TEXT ||
 			registered_fb[con2fb_map[ops->currcon]] != info)
 		return;

[-- Attachment #3: fbcon.h.patch --]
[-- Type: text/plain, Size: 331 bytes --]

--- fbcon.h	2005-11-23 23:49:10.000000000 -0600
+++ fbcon.h.mine	2005-11-23 23:46:07.000000000 -0600
@@ -73,6 +73,7 @@
 	struct fb_cursor cursor_state;
 	struct display *p;
         int    currcon;	                /* Current VC. */
+	struct vc_data *currcon_ptr;
 	int    cursor_flash;
 	int    cursor_reset;
 	int    blank_state;

Re: Patch to framebuffer

[Antonino A. Daplas]

Nathan Cline wrote:
> Hello, this is my first time posting to this list so please forgive me
> if I'm violating protocol in some way. :)  I've written a patch to the
> framebuffer code to modify its behavior a bit. I am running on a
> dual-headed system and I noticed when I was working in one console on
> one monitor, the console on the other monitor was "frozen", not
> updating itself. After some digging through the code I realized this
> is because the two framebuffer drivers share the same framebuffer code
> which stores a single pointer to the "current" virtual console. If a
> VC is not current it is considered invisible and is not updated. 

Yes, there is only 1 active console at one time.  And many multi-head
users has been asking (and confused) about this for some time now.

> So I
> patched the code to store a pointer for each framebuffer to the
> "foreground" VC on each one.

Well, you really don't need to store currcon_ptr in fbcon_ops.  Using
vc_cons[ops->currcon].d should be enough to get the current vc attached
to the framebuffer.  It will also make your patch smaller, and hopefully
easier to spot regressions.

> It seems to work well but I'd like to get
> others' input as this is my first time writing any kernel code, and to
> be honest there is so much code it's difficult to get a clear picture
> in my head of how the whole system works.

Yes, the console code is very confusing.  I have been looking at it for
a long time and I still don't know every code pathway it's going to take.

Anyway, with single-head systems, I can't really see your patch causing
regressions, so that's good.  With multi-head, I don't know.

Anyway, maybe Andrew can give this a whirl in the -mm tree?  Just resubmit
the patch without the currcon_ptr from fbcon_ops. And add a
Signed-off-by: line.  See:

http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

Tony 

Re: Patch to framebuffer

[Antonino A. Daplas]

Nathan Cline wrote:
> --- fbcon.c	2005-11-23 23:49:10.000000000 -0600
> +++ fbcon.c.mine	2005-11-23 23:12:16.000000000 -0600
> @@ -390,12 +390,15 @@
>  	int mode;
>  
>  	if (ops->currcon != -1)
> -		vc = vc_cons[ops->currcon].d;
> +		vc = ops->currcon_ptr; 

As mentioned in the other thread, you don't need ops->currcon_ptr.
vc_cons[ops->currcon].d should give you the same results.

>  
> -	if (!vc || !CON_IS_VISIBLE(vc) ||
> +	if (!vc)
> +		return;
> +
> +	if (!CON_IS_VISIBLE(vc) ||
>  	    fbcon_is_inactive(vc, info) ||
>   	    registered_fb[con2fb_map[vc->vc_num]] != info ||
> -	    vc_cons[ops->currcon].d->vc_deccm != 1)
> +	    vc->vc_deccm != 1)
>  		return;
>  	acquire_console_sem();
>  	p = &fb_display[vc->vc_num];
> @@ -753,6 +756,7 @@
>  	struct fbcon_ops *ops = info->fbcon_par;
>  
>  	ops->currcon = fg_console;
> +	ops->currcon_ptr = vc_cons[fg_console].d;

no need

>  
>  	if (info->fbops->fb_set_par && !(ops->flags & FBCON_FLAGS_INIT))
>  		info->fbops->fb_set_par(info);
> @@ -766,7 +770,7 @@
>  		fbcon_preset_disp(info, &info->var, unit);
>  
>  	if (show_logo) {
> -		struct vc_data *fg_vc = vc_cons[fg_console].d;
> +		struct vc_data *fg_vc = ops->currcon_ptr;

or *fg_vc = vc_cons[ops->currcon].d

>  		struct fb_info *fg_info =
>  			registered_fb[con2fb_map[fg_console]];
>  
> @@ -775,7 +779,7 @@
>  				   fg_vc->vc_rows);
>  	}
>  
> -	update_screen(vc_cons[fg_console].d);
> +	update_screen(ops->currcon_ptr);

similarly, update_screen(vc_cons[ops->currcon].d);

>  }
>  
>  /**
> @@ -929,6 +933,7 @@
>  
>  	memset(ops, 0, sizeof(struct fbcon_ops));
>  	ops->currcon = -1;
> +	ops->currcon_ptr = NULL;

no need.

>  	ops->graphics = 1;
>  	ops->cur_rotate = -1;
>  	info->fbcon_par = ops;
> @@ -1055,6 +1060,15 @@
>  	    return;
>  
>  	cap = info->flags;
> +	ops = info->fbcon_par;
> +
> +	if (ops->currcon == -1)
> +	{
> +		ops->currcon = vc->vc_num;
> +		ops->currcon_ptr = vc;

no need for the last line.

> +	}
> +
> +	vc->vc_display_fg = &(ops->currcon_ptr);
>  
>  	if (vc != svc || logo_shown == FBCON_LOGO_DONTSHOW ||
>  	    (info->fix.type == FB_TYPE_TEXT))
> @@ -1091,7 +1105,6 @@
>  	if (!*vc->vc_uni_pagedir_loc)
>  		con_copy_unimap(vc, svc);
>  
> -	ops = info->fbcon_par;
>  	p->con_rotate = rotate;
>  	set_blitting_type(vc, info, NULL);
>  
> @@ -1296,6 +1309,8 @@
>  	struct fbcon_ops *ops = info->fbcon_par;
>  	int rows, cols, charcnt = 256;
>  
> +	vc->vc_display_fg = &(ops->currcon_ptr);
> +	

or vc->vc_display_fg = &vc_cons[ops->currcon].d;

>  	if (var_to_display(p, var, info))
>  		return;
>  	t = &fb_display[svc->vc_num];
> @@ -2048,7 +2063,7 @@
>  	struct fbcon_ops *ops;
>  	struct display *p = &fb_display[vc->vc_num];
>  	struct fb_var_screeninfo var;
> -	int i, prev_console;
> +	int prev_console;
>  
>  	info = registered_fb[con2fb_map[vc->vc_num]];
>  	ops = info->fbcon_par;
> @@ -2073,21 +2088,10 @@
>  	prev_console = ops->currcon;
>  	if (prev_console != -1)
>  		old_info = registered_fb[con2fb_map[prev_console]];
> -	/*
> -	 * FIXME: If we have multiple fbdev's loaded, we need to
> -	 * update all info->currcon.  Perhaps, we can place this
> -	 * in a centralized structure, but this might break some
> -	 * drivers.
> -	 *
> -	 * info->currcon = vc->vc_num;
> -	 */
> -	for (i = 0; i < FB_MAX; i++) {
> -		if (registered_fb[i] != NULL && registered_fb[i]->fbcon_par) {
> -			struct fbcon_ops *o = registered_fb[i]->fbcon_par;
>  
> -			o->currcon = vc->vc_num;
> -		}
> -	}

This hunk is the actual culprit why fbcon is behaving like it is :-)

> +	ops->currcon = vc->vc_num;
> +	ops->currcon_ptr = vc;
> +	
>  	memset(&var, 0, sizeof(struct fb_var_screeninfo));
>  	display_to_var(&var, p);
>  	var.activate = FB_ACTIVATE_NOW;
> @@ -2103,13 +2107,6 @@
>  	fb_set_var(info, &var);
>  	ops->var = info->var;
>  
> -	if (old_info != NULL && old_info != info) {
> -		if (info->fbops->fb_set_par)
> -			info->fbops->fb_set_par(info);

Don't remove the above hunk.  It's possible to have multiple fb_info's
(ie vga16fb, vesafb, xxxfb) driving the same hardware.  This gives
the driver a chance to initialize the hardware when it's their turn
to take over.
 
> -		fbcon_del_cursor_timer(old_info);
> -		fbcon_add_cursor_timer(info);
> -	}
> -

The above is safe to remove.

>  	set_blitting_type(vc, info, p);
>  	ops->cursor_reset = 1;
>  
> @@ -2691,7 +2688,7 @@
>  
>  	if (!ops || ops->currcon < 0)
>  		return;
> -	vc = vc_cons[ops->currcon].d;
> +	vc = ops->currcon_ptr;

Again, equivalent.

>  
>  	/* Clear cursor, restore saved data */
>  	fbcon_cursor(vc, CM_ERASE);
> @@ -2704,7 +2701,7 @@
>  
>  	if (!ops || ops->currcon < 0)
>  		return;
> -	vc = vc_cons[ops->currcon].d;
> +	vc = ops->currcon_ptr; 

equivalent

>  
>  	update_screen(vc);
>  }
> @@ -2718,7 +2715,7 @@
>  
>  	if (!ops || ops->currcon < 0)
>  		return;
> -	vc = vc_cons[ops->currcon].d;
> +	vc = ops->currcon_ptr; 

equivalent

>  	if (vc->vc_mode != KD_TEXT ||
>  	    registered_fb[con2fb_map[ops->currcon]] != info)
>  		return;
> @@ -2841,7 +2838,7 @@
>  	if (!ops || ops->currcon < 0)
>  		return;
>  
> -	vc = vc_cons[ops->currcon].d;
> +	vc = ops->currcon_ptr; 

equivalent

>  	if (vc->vc_mode != KD_TEXT ||
>  			registered_fb[con2fb_map[ops->currcon]] != info)
>  		return;
> 
> 
> ------------------------------------------------------------------------
> 
> --- fbcon.h	2005-11-23 23:49:10.000000000 -0600
> +++ fbcon.h.mine	2005-11-23 23:46:07.000000000 -0600
> @@ -73,6 +73,7 @@
>  	struct fb_cursor cursor_state;
>  	struct display *p;
>          int    currcon;	                /* Current VC. */
> +	struct vc_data *currcon_ptr;

No need for currcon_ptr.

Tony

Re: Patch to framebuffer

[Matt Keenan]

Nathan Cline wrote:

>Hello, this is my first time posting to this list so please forgive me
>if I'm violating protocol in some way. :)  I've written a patch to the
>framebuffer code to modify its behavior a bit. I am running on a
>dual-headed system and I noticed when I was working in one console on
>one monitor, the console on the other monitor was "frozen", not
>updating itself. After some digging through the code I realized this
>is because the two framebuffer drivers share the same framebuffer code
>which stores a single pointer to the "current" virtual console. If a
>VC is not current it is considered invisible and is not updated. So I
>patched the code to store a pointer for each framebuffer to the
>"foreground" VC on each one. It seems to work well but I'd like to get
>others' input as this is my first time writing any kernel code, and to
>be honest there is so much code it's difficult to get a clear picture
>in my head of how the whole system works.
>I've attached the patches to this message, a small one to
>drivers/video/console/fbcon.h, and a larger one to fbcon.c. I would
>appreciate it if some of you guys could look over this code and look
>for any obvious errors, or better yet, hopefully someone else has a
>multihead system they can try it on. The patches are against the
>latest GIT source tree (torvalds, main branch, 2.6.15rc2) as of last
>night.
>Any replies to this message should be CC'ed directly to my e-mail
>account (nathan dot cline .. gmail dot com) as I am not currently
>subscribed to this list. I look forward to getting this patch of high
>enough quality to submit to Linus. Thanks!
>  
>
[PATCH SNIPPED]

Nathan you may also want to send a copy of your cleaned up patch to the 
folks at linuxconsole-dev@lists.sourceforge.net as that is list that 
deals with multihead development / issues. We also have patches for 
getting multiheaded X working as well.

Matt Keenan

Re: Patch to framebuffer

[Pekka Enberg]

On 11/24/05, Nathan Cline <nathan.cline@gmail.com> wrote:
> Hello, this is my first time posting to this list so please forgive me
> if I'm violating protocol in some way. :)

Please read Documentation/SubmittingPatches before you resend.

                              Pekka

Patch to framebuffer

[Knut Petersen]

Hi Nathan,

Updating only one framebuffer console at a time is a real feature for me,
it allows a really quick developement cycle:

            compile vesafb into the kernel, cyblafb as a module
            start with vesafb
loop:    load cyblafb
            switch to cyblafb, test it
            correct bugs, compile new cyblafb module
            switch to vesafb
            unload vesafb
            goto loop
           
Although both vesafb and cyblafb control the same hardware, this is
possible because of the "feature" you want to remove.

On the other hand I do understand that your patch is valuable, if I had
two monitors attached I certainly would like to be able to use both at the
same time.

I believe that you should introduce your code either as a compile time
option or even better it should be possible to disable it via sysfs:

     /sys/class/graphics/fb?/update_mode

0 could be the default and would activate your new code
1 would restore the old "one active" mode.

Please  consider my arguments and join us at

linux-fbdev-devel@lists.sourceforge.net

cu,
 Knut