Fix switch from XFree

PATCH In review Benjamin Herrenschmidt 3 participants 7 messages Feb 10, 2004

Subsystems (2)

Framebuffer layer Console

lore

Share & tools

lore

Benjamin Herrenschmidt

author

2004-02-10 10:36 UTC #0 lore raw

PATCH 1/1 Fix switch from XFree

Hi James !

(This patch includes the one adding the lock around the notifier registration so you can drop that previous one)

Currently, there is a problem when switching from XFree to a console that became especially annoying now that we have some accelerations in radeonfb 2.6. XFree leaves the accel engine in a state that is incorrect for our accel operations. This problem is generic with most drivers, we can't expect X to leave the accel engine in the right state, that's simply unrealistic.

The approach I've taken is from fbcon_switch, to check wether we are switching from KD_GRAPHICS to a KD_TEXT console, and in this case, cause fb_set_var with an additional flag I defined, FB_ACTIVATE_FORCE, which will just bypass the comparison on the var structure, forcing set_par to be called again.

That works for normal switches. That doesn't quite deal with a console beeing switched from KD_GRAPHICS to KD_TEXT, there is no consw hook for that, so one would have to be added for this purpose I suspect... But that part of the problem is much less annoying in practice.

The patch also include a couple of leftover fixes from my tree, it's too late for me to split things up ;) Mostly dealing with the cursor when the fb is sleeping

(Oh, and that FB_PIXMAP_IO seemed wrong, I switched back to _DEFAULT, as it's not mapped into card space afaik)

Ben.

drivers/video/console/fbcon.c +48 ~~-15~~

diff -urN fbdev-2.5/drivers/video/console/fbcon.c linuxppc-2.5-benh/drivers/video/console/fbcon.c
@@ -196,7 +196,7 @@
 {
 	struct fb_info *info = (struct fb_info *) private;
 
-	if (!info)
+	if (!info || info->state != FBINFO_STATE_RUNNING)
 		return;
 	info->cursor.enable ^= 1;
 	info->fbops->fb_cursor(info, &info->cursor);

@@ -602,7 +602,6 @@
 
 	/* on which frame buffer will we open this console? */
 	info = registered_fb[(int) con2fb_map[unit]];
-	
 	if (info->var.accel_flags)
 		fb_display[unit].scrollmode = SCROLL_YNOMOVE;
 	else

@@ -1564,8 +1563,8 @@
 			height, width);
 }
 
-static int fbcon_resize(struct vc_data *vc, unsigned int width, 
-			unsigned int height)
+static int fbcon_do_resize(struct vc_data *vc, unsigned int width, 
+			   unsigned int height, int force)
 {
 	struct fb_info *info = registered_fb[(int) con2fb_map[vc->vc_num]];
 	struct display *p = &fb_display[vc->vc_num];

@@ -1573,13 +1572,14 @@
 	int err; int x_diff, y_diff;
 	int fw = vc->vc_font.width;
 	int fh = vc->vc_font.height;
+	int rc = 0;
 
 	var.xres = width * fw;
 	var.yres = height * fh;
 	x_diff = info->var.xres - var.xres;
 	y_diff = info->var.yres - var.yres;
 	if (x_diff < 0 || x_diff > fw ||
-	   (y_diff < 0 || y_diff > fh)) {
+	   (y_diff < 0 || y_diff > fh) || force) {
 		var.activate = FB_ACTIVATE_FIND;
 		err = fb_set_var(info, &var);
 		if (err || width > var.xres/fw ||

@@ -1587,6 +1587,8 @@
 			return -EINVAL;
        		DPRINTK("resize now %ix%i\n", var.xres, var.yres);
 		var.activate = FB_ACTIVATE_NOW;
+		if (force)
+			var.activate |= FB_ACTIVATE_FORCE;
 		/* This flag is enough for now as we are supposed to hold
 		 * the console semaphore at this point. I agree it's a bit
 		 * ugly but it does the job until some better solution is

@@ -1595,17 +1597,25 @@
 		console_resizing = 1;
 		fb_set_var(info, &var);
 		console_resizing = 0;
+		rc = 1;
 	}
 	p->vrows = var.yres_virtual/fh;
 	if (var.yres > (fh * (height + 1)))
 		p->vrows -= (var.yres - (fh * height)) / fh;
-	return 0;
+	return rc;
+}
+
+static int fbcon_resize(struct vc_data *vc, unsigned int width, 
+			unsigned int height)
+{
+	return fbcon_do_resize(vc, width, height, 0);
 }
 
 static int fbcon_switch(struct vc_data *vc)
 {
 	struct fb_info *info = registered_fb[(int) con2fb_map[vc->vc_num]];
 	struct display *p = &fb_display[vc->vc_num];
+	int gfx_to_text = 0;
 
 	if (softback_top) {
 		int l = fbcon_softback_size / vc->vc_size_row;

@@ -1633,12 +1643,17 @@
 	if (info)
 		info->var.yoffset = p->yscroll = 0;
 
+	if (info->currcon == -1 ||
+	    (vt_cons[info->currcon]->vc_mode != KD_TEXT &&
+	     vt_cons[vc->vc_num]->vc_mode == KD_TEXT))
+		gfx_to_text = 1;
+
 	/* Set currcon before fbcon_resize or we'll do bad things
 	 * when fbdev calls us back on mode changed notification
 	 */
 	info->currcon = vc->vc_num;
 
-        fbcon_resize(vc, vc->vc_cols, vc->vc_rows);
+        fbcon_do_resize(vc, vc->vc_cols, vc->vc_rows, gfx_to_text);
 	switch (p->scrollmode & __SCROLL_YMASK) {
 	case __SCROLL_YWRAP:
 		scrollback_phys_max = p->vrows - vc->vc_rows;

@@ -2247,24 +2262,31 @@
 
 static void fbcon_suspended(struct fb_info *info)
 {
-	/* Here, we should do something to properly erase the
-	 * cursor and synchronize with the cursor interrupt on
-	 * SMP... (may not be that critical though...)
-	 */
+	/* Clear cursor, restore saved data */
+	info->cursor.enable = 0;
+	info->fbops->fb_cursor(info, &info->cursor);
 }
 
 static void fbcon_resumed(struct fb_info *info)
 {
-	struct vc_data *vc = vc_cons[info->currcon].d;
+	struct vc_data *vc;
+
+	if (info->currcon < 0)
+		return;
+	vc = vc_cons[info->currcon].d;
 
 	update_screen(vc->vc_num);
 }
 
 static void fbcon_mode_changed(struct fb_info *info)
 {
-	struct vc_data *vc = vc_cons[info->currcon].d;
+	struct vc_data *vc;
 	int rows, cols;
 
+	if (info->currcon < 0)
+		return;
+	vc = vc_cons[info->currcon].d;
+
 	/* This isn't perfect yet. If we change one console, we
 	 * don't change them all and we switch back to the wrong
 	 * mode on next console switch. We need to either keep a

@@ -2339,6 +2361,7 @@
 	.notifier_call	= fbcon_event_notify,
 };
 
+static int fbcon_event_notifier_registered;
 
 int __init fb_console_init(void)
 {

@@ -2349,14 +2372,24 @@
 	take_over_console(&fb_con, first_fb_vc, last_fb_vc, fbcon_is_default);
 	info = registered_fb[num_registered_fb-1];	
 
-	fb_register_client(&fbcon_event_notifer);
+	acquire_console_sem();
+	if (!fbcon_event_notifier_registered) {
+		fb_register_client(&fbcon_event_notifer);
+		fbcon_event_notifier_registered = 1;
+	} 
+	release_console_sem();
 
 	return 0;
 }
 
 void __exit fb_console_exit(void)
 {
-	fb_unregister_client(&fbcon_event_notifer);
+	acquire_console_sem();
+	if (fbcon_event_notifier_registered) {
+		fb_unregister_client(&fbcon_event_notifer);
+		fbcon_event_notifier_registered = 0;
+	}
+	release_console_sem();
 	give_up_console(&fb_con);
 }

drivers/video/fbmem.c +9 -5

diff -urN fbdev-2.5/drivers/video/fbmem.c linuxppc-2.5-benh/drivers/video/fbmem.c
@@ -965,7 +965,8 @@
 {
 	int err;
 
-	if (memcmp(&info->var, var, sizeof(struct fb_var_screeninfo))) {
+	if ((var->activate & FB_ACTIVATE_FORCE) ||
+	    memcmp(&info->var, var, sizeof(struct fb_var_screeninfo))) {
 		if (!info->fbops->fb_check_var) {
 			*var = info->var;
 			return 0;

@@ -1023,7 +1024,7 @@
 	struct fb_con2fbmap con2fb;
 #endif
 	struct fb_cmap cmap;
-	int i;
+	int i, rc;
 	
 	if (!fb)
 		return -ENODEV;

@@ -1064,7 +1065,10 @@
 			return -EFAULT;
 		return 0;
 	case FBIO_CURSOR:
-		return (fb_cursor(info, (struct fb_cursor *) arg));
+		acquire_console_sem();
+		rc = fb_cursor(info, (struct fb_cursor *) arg);
+		release_console_sem();
+		return rc;
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE
 	case FBIOGET_CON2FBMAP:
 		if (copy_from_user(&con2fb, (void *)arg, sizeof(con2fb)))

@@ -1304,7 +1308,7 @@
 			fb_info->sprite.size = FBPIXMAPSIZE;
 			fb_info->sprite.buf_align = sizeof(unsigned long);
 			fb_info->sprite.scan_align = sizeof(unsigned long);
-			fb_info->sprite.flags = FB_PIXMAP_IO;
+			fb_info->sprite.flags = FB_PIXMAP_DEFAULT;
 		}
 	}	
 	fb_info->sprite.offset = 0;

@@ -1411,7 +1415,7 @@
 		printk("unable to get major %d for fb devs\n", FB_MAJOR);
 
 	class_register(&fb_class);
-	
+
 #ifdef CONFIG_FB_OF
 	if (ofonly) {
 		offb_init();

include/linux/fb.h +5 -4

diff -urN fbdev-2.5/include/linux/fb.h linuxppc-2.5-benh/include/linux/fb.h
@@ -153,6 +153,7 @@
 #define FB_ACTIVATE_VBL	       16	/* activate values on next vbl  */
 #define FB_CHANGE_CMAP_VBL     32	/* change colormap on vbl	*/
 #define FB_ACTIVATE_ALL	       64	/* change all VCs on this fb	*/
+#define FB_ACTIVATE_FORCE     128	/* force apply even when no change*/
 
 #define FB_ACCELF_TEXT		1	/* text mode acceleration */

@@ -391,10 +392,10 @@
 #include <linux/notifier.h>
 #include <asm/io.h>
 
-struct vm_area_struct;
 struct fb_info;
-struct device;
+struct vm_area_struct;
 struct file;
+struct device;
 
 /*
  * Register/unregister for framebuffer events

@@ -418,9 +419,9 @@
  * Pixmap structure definition
  *
  * The purpose of this structure is to translate data
- * from the hardware independent format of fbdev to what 
+ * from the hardware independent format of fbdev to what
  * format the hardware needs.
- */ 
+ */
 
 #define FB_PIXMAP_DEFAULT 1     /* used internally by fbcon */
 #define FB_PIXMAP_SYSTEM  2     /* memory is in system RAM  */

-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004 Premiere Conference on Open Tools Development and Integration See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Sven Luther

2004-02-10 11:06 UTC #1 lore raw

Quoted 11 lines · Benjamin Herrenschmidt

Hi James !
(This patch includes the one adding the lock around the notifier registration so you can drop that previous one)
Currently, there is a problem when switching from XFree to a console that became especially annoying now that we have some accelerations in radeonfb 2.6. XFree leaves the accel engine in a state that is incorrect for our accel operations. This problem is generic with most drivers, we can't expect X to leave the accel engine in the right state, that's simply unrealistic.

Mmm, i wonder about this. The X driver is supposed to be responsible for saving and reseting the engine when leaving/entering. This is usually done in a separate way when using or not using fbdev.

Now, if we are going to save/restore the contect too, then this would mean a risk of dual saving the engine state. Or maybe the X server doesn't really save the context, but just reinitialize it when entering.

I have the feeling that more working together would be helpfull, or maybe setup a proper policy for X/fbdev to handle this properly.

Maybe the fbdev driver could set a flag or something, which would tell the X server that it is taking charge of saving/restoring the accel engine, so the X driver would not try doing this too, at least in the UseFBDev case (but do we care about the other case ?).

Friendly,

Sven Luther

Benjamin Herrenschmidt

author

2004-02-10 11:17 UTC #2 lore raw

Quoted 4 lines

Mmm, i wonder about this. The X driver is supposed to be responsible for saving and reseting the engine when leaving/entering. This is usually done in a separate way when using or not using fbdev.

This is a nice dream. It's both _very_ impractical (you can't always _read_ the engine state) and usually buggy as hell (and go fix XFree :)

On the other hand, it's very simple for the driver to just reset & restore the engine state it expects.

Quoted 3 lines

Now, if we are going to save/restore the contect too, then this would mean a risk of dual saving the engine state. Or maybe the X server doesn't really save the context, but just reinitialize it when entering.

It tries too for the mode... but fails. It doesn't try for the engine

Quoted 2 lines

I have the feeling that more working together would be helpfull, or maybe setup a proper policy for X/fbdev to handle this properly.

I don't think it's reasonable to expect the XFree driver to save/restore the accel engine state. It's already quite broken with the mode on radeonfb (the VGA stuff seem to do bad things on PPC at least).

Quoted 4 lines

Maybe the fbdev driver could set a flag or something, which would tell the X server that it is taking charge of saving/restoring the accel engine, so the X driver would not try doing this too, at least in the UseFBDev case (but do we care about the other case ?).

No. Trying to save/restore the engine state would mean trying to save/restore dozens (or hundreds maybe ? depends if you count the 3D engine too :), it makes little sense, it's much simpler for fbdev to just reset the state it expects.

Ben.

Sven Luther

2004-02-10 11:28 UTC #3 lore raw

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 4 lines
Mmm, i wonder about this. The X driver is supposed to be responsible for saving and reseting the engine when leaving/entering. This is usually done in a separate way when using or not using fbdev.
This is a nice dream. It's both _very_ impractical (you can't always _read_ the engine state) and usually buggy as hell (and go fix XFree :)

Well, i have no problem in fixing XFree86. But then, one would have to consider the alternate or outdated servers too, altough i doubt you care about XiG or other such comercial servers which provide x86 only.

Quoted 2 lines

On the other hand, it's very simple for the driver to just reset & restore the engine state it expects.

Or to use a different pipe, if the hardware provides accelerated context switching :)).

Quoted 5 lines

Quoted 3 lines
Now, if we are going to save/restore the contect too, then this would mean a risk of dual saving the engine state. Or maybe the X server doesn't really save the context, but just reinitialize it when entering.
It tries too for the mode... but fails. It doesn't try for the engine

Ok.

Quoted 6 lines

Quoted 2 lines
I have the feeling that more working together would be helpfull, or maybe setup a proper policy for X/fbdev to handle this properly.
I don't think it's reasonable to expect the XFree driver to save/restore the accel engine state. It's already quite broken with the mode on radeonfb (the VGA stuff seem to do bad things on PPC at least).

Ok, but for the mode, if you use UseFBdev, then the server will not touch the mode, and rather use the underlying fbdev infrastructure for it. Why not do something similar for 2D, and tell X that this version of the fbdev does indeed know how to fix the accel engine, and doesn't care about getting it in broken state.

Quoted 9 lines

Quoted 4 lines
Maybe the fbdev driver could set a flag or something, which would tell the X server that it is taking charge of saving/restoring the accel engine, so the X driver would not try doing this too, at least in the UseFBDev case (but do we care about the other case ?).
No. Trying to save/restore the engine state would mean trying to save/restore dozens (or hundreds maybe ? depends if you count the 3D engine too :), it makes little sense, it's much simpler for fbdev to just reset the state it expects.

Ok, i understand, but again, maybe if the fbdev doesn't care, there is no need for X trying to save/restore it ? Or whatever it tries to do.

I think we could decide on a sane policy for both fbdev and X to follow (for example, each one is in charge of resetting their mode/accel engine state), and then submit this policy to the XFree86 people to adhere too if the UseFBDev option is used. If XFree86 drivers do not implement it, then they are broken, and its their problem, but if they do, then we result in having the most economical context swithc possible.

Friendly,

Sven Luther

Benjamin Herrenschmidt

author

2004-02-10 11:31 UTC #4 lore raw

Quoted 3 lines

Or to use a different pipe, if the hardware provides accelerated context switching :)).

Do we really care ? This is VT switch...

Quoted 5 lines

Ok, but for the mode, if you use UseFBdev, then the server will not touch the mode, and rather use the underlying fbdev infrastructure for it. Why not do something similar for 2D, and tell X that this version of the fbdev does indeed know how to fix the accel engine, and doesn't care about getting it in broken state.

It already doesn't care :) It doesn't save / restore the engine state

Quoted 2 lines

Ok, i understand, but again, maybe if the fbdev doesn't care, there is no need for X trying to save/restore it ? Or whatever it tries to do.

It only does the mode, and only without FBDev

Quoted 10 lines

I think we could decide on a sane policy for both fbdev and X to follow (for example, each one is in charge of resetting their mode/accel engine state), and then submit this policy to the XFree86 people to adhere too if the UseFBDev option is used. If XFree86 drivers do not implement it, then they are broken, and its their problem, but if they do, then we result in having the most economical context swithc possible.
Friendly,
Sven Luther

Signature · 9 lines

Benjamin Herrenschmidt <[email protected]>



-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Geert Uytterhoeven

2004-02-10 12:34 UTC #5 lore raw

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines
Or to use a different pipe, if the hardware provides accelerated context switching :)).
Do we really care ? This is VT switch...

What about resetting the accel engine when switching from KD_GRAPHICS to KD_TEXT?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Benjamin Herrenschmidt

author

2004-02-10 22:14 UTC #6 lore raw

Quoted 8 lines · Geert Uytterhoeven

Quoted 4 lines · Benjamin Herrenschmidt
Quoted 2 lines
Or to use a different pipe, if the hardware provides accelerated context switching :)).
Do we really care ? This is VT switch...
What about resetting the accel engine when switching from KD_GRAPHICS to KD_TEXT?

Did you read my original mail ? :)

With this patch, I force a set_par, which resets the mode and the engine on a switch from a KD_GRAPHICS VT to a KD_TEXT VT. I don't do anything at this point when the current console is switched from KD_GFX to KD_TEXT without involving a VT swith as, afaik, there's no fbcon hook for that at this point. (A hack would be possible, but I want to avoid it).

Ben.