Fix switching to KD_TEXT
Subsystems (2)
PATCH 1/1 Fix switching to KD_TEXT
Every framebuffer driver relies on the assumption that the set_par()
function
of the driver is called before drawing functions and other functions dependent on the hardware state are executed.
This assumption is false in two cases, and one is a genuine linux bug:
1: Whenever you switch from X to a framebuffer console for the very
first time, there is a chance that a broken X system has _not_ set the mode to KD_GRAPHICS, thus the vt and framebuffer code executes a screen redraw and several other functions before a set_par() is executed. This is believed to be not a bug of linux but a bug of X/xdm.
2: Whenever a switch from X to a framebuffer console occures,
the pan_display() function of the driver is called once before the set_par() function of the driver is called. Code path:
complete_change_console -> redraw_screen -> fbcon_switch -> bit_update_start-> fb_pan_display -> xyz_pan_display. This is clearly a bug of linux.
Although our primary goal must be to fix linux bugs and not to work around bugs of X, the patch fixes both of the cases.
The advantage and correctness of this patch should be obvious. Yes, it does add a possibly slow call to the fb_set_par() function, but at this point it is necessary.
drivers/video/console/fbcon.c +0 -0
@@ -2103,10 +2103,11 @@ static int fbcon_switch(struct vc_data *
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);
+ if (old_info != NULL && old_info != info)
fbcon_del_cursor_timer(old_info);
+
+ if (info->fbops->fb_set_par) {
+ info->fbops->fb_set_par(info);
fbcon_add_cursor_timer(info);
}Quoted 16 lines · Knut Petersen
Every framebuffer driver relies on the assumption that the
set_par()
function
of the driver is called before drawing functions and other functions dependent
on the hardware state are executed.This assumption is false in two cases, and one is a genuine linux bug:
1: Whenever you switch from X to a framebuffer console for the very
first time, there is a chance that a broken X system has _not_ set the mode toKD_GRAPHICS, thus the vt and framebuffer code executes a screen redraw and several other functions before aset_par()is executed. This is believed to be not a bug of linux but a bug of X/xdm.
Quoted 6 lines
2: Whenever a switch from X to a framebuffer console occures,
thepan_display()function of the driver is called once before theset_par()function of the driver is called. Code path:
complete_change_console->redraw_screen->fbcon_switch-> bit_update_start->fb_pan_display->xyz_pan_display. This is clearly a bug of linux.
This part, #2, can be easily fixed.
Quoted 9 lines
Although our primary goal must be to fix linux bugs and not to work around bugs of X, the patch fixes both of the cases.
The advantage and correctness of this patch should be obvious. Yes, it does add a possibly slow call to the
fb_set_par()function, but at this point it is necessary.Signed-off-by Knut Petersen <[email protected]>
Sorry, NAK for now. Unless other people agree that it is okay for them to have an unconditional call to set_par() for every console switch. Note that the set_par() in some drivers is terribly slow (several seconds at least).
Let's wait a few days, if nobody disagrees with you, so be it.
Tony
Hi Tony!
Quoted 8 lines
Sorry, NAK for now. Unless other people agree that it is okay for them to have an unconditional call to
set_par()for every console switch. Note that theset_par()in some drivers is terribly slow (several seconds at least).Let's wait a few days, if nobody disagrees with you, so be it.
Which hardware needs several seconds? I do run relatively slow hardware, but a complete set_par() needs less than 0.003 seconds with cyblafb.
If there really is hardware that needs _several_ seconds for _one_ set_par(), then we should really be more carefull when to call that function.
Let´s have a look at a switch from the framebuffer console to X and back.
Kernel is a 2.6.15-rc4-git1 with my current cyblafb + your patches needed for cyblafb
+ the patch we are talking about.
Framebuffer console is 1 with mode 1280x1024, X console is 7 and has been set to 1024x768 before X was activated.
Now I do run "chvt 7;sleep 3;chvt 1" on the vt 1.
[ 972.360841] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 972.360898] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 972.360925] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 972.360951] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 972.360989] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 972.361018] [<c02c8203>] change_console+0x43/0x90 [ 972.361043] [<c02ceebd>] console_callback+0xed/0x110 [ 972.361101] [<c0127e6a>] worker_thread+0x1ba/0x260 [ 972.361143] [<c012c115>] kthread+0x95/0xd0 [ 972.361171] [<c0100c15>] kernel_thread_helper+0x5/0x10 [ 972.361212] cyblafb: Switching to new mode: fbset -g 1024 768 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.363308] cyblafb: pixclock = 64.99 MHz, k/m/n 1 b 6e
[ 972.363916] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 972.363945] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 972.363969] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 972.363996] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 972.364023] [<c02c8203>] change_console+0x43/0x90 [ 972.364047] [<c02ceebd>] console_callback+0xed/0x110 [ 972.364093] [<c0127e6a>] worker_thread+0x1ba/0x260 [ 972.364121] [<c012c115>] kthread+0x95/0xd0 [ 972.364143] [<c0100c15>] kernel_thread_helper+0x5/0x10 [ 972.364181] cyblafb: Switching to new mode: fbset -g 1024 768 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.366271] cyblafb: pixclock = 64.99 MHz, k/m/n 1 b 6e [ 972.366325] cyblafb: Switching to KD_GRAPHICS
Not so nice. Now let´s have a look at the log entries caused switching back to the framebuffer console:
This call to set_par() is the result of my patch:
[ 975.751407] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.751437] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 975.751462] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 975.751486] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.751513] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.751540] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.751564] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.751587] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.751613] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.751640] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.751666] [<c01029a9>] syscall_call+0x7/0xb [ 975.751702] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.753795] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
Without that prior set_par() that´s the point where the enhanced cyblafb would have serious problems:
[ 975.753839] cyblafb_pan_display: entry [ 975.753853] [<c0281f7c>] cyblafb_pan_display+0x9c/0xb0 [ 975.753883] [<c027b911>] fb_pan_display+0x61/0xa0 [ 975.753925] [<c027ba4f>] fb_set_var+0xff/0x210 [ 975.753949] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 975.753973] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.754000] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.754027] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.754051] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.754074] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.754100] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.754127] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.754153] [<c01029a9>] syscall_call+0x7/0xb
Again a call to set_par:
[ 975.754744] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.754773] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 975.754796] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.754823] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.754850] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.754875] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.754917] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.754943] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.754970] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.754996] [<c01029a9>] syscall_call+0x7/0xb [ 975.755032] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.757123] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
Now the save_state() function is called:
[ 975.757745] cyblafb: Switching to KD_TEXT
and again a call to set_par():
[ 975.757759] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.757789] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 975.757813] [<c027498a>] fbcon_blank+0x11a/0x1b0 [ 975.757837] [<c02cff17>] do_unblank_screen+0x67/0x120 [ 975.757869] [<c02c8130>] complete_change_console+0x40/0xd0 [ 975.757955] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.757979] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.758002] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.758029] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.758055] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.758081] [<c01029a9>] syscall_call+0x7/0xb [ 975.758117] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.760205] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
And now, the be really shure, again, a call to set_par():
[ 975.760827] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.760856] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 975.760880] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.760923] [<c02749e6>] fbcon_blank+0x176/0x1b0 [ 975.760947] [<c02cff17>] do_unblank_screen+0x67/0x120 [ 975.760976] [<c02c8130>] complete_change_console+0x40/0xd0 [ 975.761002] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.761027] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.761050] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.761076] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.761102] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.761129] [<c01029a9>] syscall_call+0x7/0xb [ 975.761164] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.763280] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
First of all: set_par is executed too often. With and without my patch.
If we would know that vt x is nothing but the vt used for X, we could introduce a policy to completely ignore all framebuffer operations for that console. That also would result in a much nicer look of the modeswitch. Do we need a new kernel parameter or should this be implemented by detected which vt is switched to KD_GRAPHICS?
Switching from X to the console could be optimized a lot, I´m shure that you do agree. We do need _one_ call to set_par() etc, but this must be at the right place. And the right place is somewhere before any other driver operation is called, with the exception of check_var() and init functions obviously.
Think about your NAK carefully. I might submit a patch to skeletonfb.c that inserts a warning at all places that might be unexpectedly called with a hardware state different to that set by set_par(). ;-))))
And please don´t argue that X or certain releases are broken. That is true, but ordinary users will use those broken versions for years.
cu,
Knut
Quoted 3 lines · Knut Petersen
And please don´t argue that X or certain releases are broken. That is true, but ordinary
users will use those broken versions for years.
Then they should just keep using vgacon.
However if you're really going to add more uneccessary set_par() calls please make them configurable (eg. CONFIG_BROKEN_X). I'll go crazy if my CRT is going to start resyncing on every vt switch.
Signature · 3 lines
Ville Syrjälä [email protected] http://www.sci.fi/~syrjala/
Quoted 7 lines · Knut Petersen
Hi Tony!
Quoted 5 lines
Sorry, NAK for now. Unless other people agree that it is okay for them to have an unconditional call to
set_par()for every console switch. Note that theset_par()in some drivers is terribly slow (several seconds at least).
Quoted 5 lines
Quoted 4 lines
Let's wait a few days, if nobody disagrees with you, so be it.
Which hardware needs several seconds?
savagefb does it in 3 seconds. I do remember a few nvidia cards that takes longer. i810fb is okay, it's also in the millisecond range.
I do run relatively slow hardware,
Quoted 2 lines
but a complete
set_par()needs less than 0.003 seconds with cyblafb.
Good for you.
Quoted 4 lines
If there really is hardware that needs _several_ seconds for _one_
set_par(), then we should
really be more carefull when to call that function.
People accept that when switching from X to console and vice versa, it will be slow. It's when switching from a text console to another that needs to be very fast. Inserting a set_par that can take a few to several seconds will eventually become frustrating for people who predominantly work in console mode.
Quoted 62 lines
Let´s have a look at a switch from the framebuffer console to X and back.
Kernel is a 2.6.15-rc4-git1 with my current cyblafb + your patches needed for cyblafb
+ the patch we are talking about.Framebuffer console is 1 with mode 1280x1024, X console is 7 and has been set to 1024x768
before X was activated.Now I do run "chvt 7;sleep 3;chvt 1" on the vt 1.
[ 972.360841] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 972.360898] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 972.360925] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 972.360951] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 972.360989] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 972.361018] [<c02c8203>] change_console+0x43/0x90 [ 972.361043] [<c02ceebd>] console_callback+0xed/0x110 [ 972.361101] [<c0127e6a>] worker_thread+0x1ba/0x260 [ 972.361143] [<c012c115>] kthread+0x95/0xd0 [ 972.361171] [<c0100c15>] kernel_thread_helper+0x5/0x10 [ 972.361212] cyblafb: Switching to new mode: fbset -g 1024 768 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.363308] cyblafb: pixclock = 64.99 MHz, k/m/n 1 b 6e[ 972.363916] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 972.363945] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 972.363969] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 972.363996] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 972.364023] [<c02c8203>] change_console+0x43/0x90 [ 972.364047] [<c02ceebd>] console_callback+0xed/0x110 [ 972.364093] [<c0127e6a>] worker_thread+0x1ba/0x260 [ 972.364121] [<c012c115>] kthread+0x95/0xd0 [ 972.364143] [<c0100c15>] kernel_thread_helper+0x5/0x10 [ 972.364181] cyblafb: Switching to new mode: fbset -g 1024 768 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.366271] cyblafb: pixclock = 64.99 MHz, k/m/n 1 b 6e [ 972.366325] cyblafb: Switching to KD_GRAPHICSNot so nice. Now let´s have a look at the log entries caused switching back to
the framebuffer console:This call to
set_par()is the result of my patch:[ 975.751407] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.751437] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 975.751462] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 975.751486] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.751513] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.751540] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.751564] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.751587] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.751613] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.751640] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.751666] [<c01029a9>] syscall_call+0x7/0xb [ 975.751702] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.753795] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
Quoted 3 lines
Without that prior
set_par()that´s the point where the enhanced cyblafb would
have serious problems:
As I've mentioned above, this can be easily fixed.
Quoted 72 lines
[ 975.753839] cyblafb_pan_display: entry [ 975.753853] [<c0281f7c>] cyblafb_pan_display+0x9c/0xb0 [ 975.753883] [<c027b911>] fb_pan_display+0x61/0xa0 [ 975.753925] [<c027ba4f>] fb_set_var+0xff/0x210 [ 975.753949] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 975.753973] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.754000] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.754027] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.754051] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.754074] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.754100] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.754127] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.754153] [<c01029a9>] syscall_call+0x7/0xbAgain a call to
set_par:[ 975.754744] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.754773] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 975.754796] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.754823] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.754850] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.754875] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.754917] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.754943] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.754970] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.754996] [<c01029a9>] syscall_call+0x7/0xb [ 975.755032] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.757123] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3aNow the
save_state()function is called:[ 975.757745] cyblafb: Switching to KD_TEXTand again a call to
set_par():[ 975.757759] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.757789] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 975.757813] [<c027498a>] fbcon_blank+0x11a/0x1b0 [ 975.757837] [<c02cff17>] do_unblank_screen+0x67/0x120 [ 975.757869] [<c02c8130>] complete_change_console+0x40/0xd0 [ 975.757955] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.757979] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.758002] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.758029] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.758055] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.758081] [<c01029a9>] syscall_call+0x7/0xb [ 975.758117] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.760205] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3aAnd now, the be really shure, again, a call to
set_par():[ 975.760827] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.760856] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 975.760880] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.760923] [<c02749e6>] fbcon_blank+0x176/0x1b0 [ 975.760947] [<c02cff17>] do_unblank_screen+0x67/0x120 [ 975.760976] [<c02c8130>] complete_change_console+0x40/0xd0 [ 975.761002] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.761027] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.761050] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.761076] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.761102] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.761129] [<c01029a9>] syscall_call+0x7/0xb [ 975.761164] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.763280] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3aFirst of all:
set_paris executed too often. With and without my patch.
Quoted 7 lines
If we would know that vt x is nothing but the vt used for X, we could introduce a policy
to completely ignore all framebuffer operations for that console. That also would result
in a much nicer look of the modeswitch. Do we need a new kernel parameter or should
this be implemented by detected which vt is switched toKD_GRAPHICS?
I've tried this before, it is ugly.
Quoted 8 lines
Switching from X to the console could be optimized a lot, I´m shure that you do agree.
We do need _one_ call toset_par()etc, but this must be at the right place. And the right
place is somewhere before any other driver operation is called, with the exception ofcheck_var()and init functions obviously.
The problem is that the console code has so many pathways that it is difficult to find the correct moment to call a set_par. Especially since there are so many cases where it's illegal for the console to write to the framebuffer. Think KD_GRAPHICS, hardware/software suspend, hardware initialization, etc. Think also that the hardware can be change by the kernel (via stty) and by the user (via fbset, etc).
Yes, we can optimize the code pathways so we minimize calls to set_par and set_var, but remember that just a few kernel versions ago, fbcon/ fbdev was plagued with bugs, so bug fixing was the priority, not code optimization. Nowadays, core fbdev and fbcon looks pretty stable so we can redirect our effort towards this goal.
Quoted 6 lines
Think about your NAK carefully. I might submit a patch to
skeletonfb.cthat inserts
a warning at all places that might be unexpectedly called with a hardware state different
to that set byset_par(). ;-))))
Go ahead. Only pan_display, I believe, currently has this problem. And again, the fix is simple.
Quoted 4 lines
And please don´t argue that X or certain releases are broken. That is true, but ordinary
users will use those broken versions for years.
I'm not arguing that. However, we can always work around this buggy implementation. For example, one can do an
echo 1 > /sys/class/graphics/fb0/state
before kdm (the buggy one that forgets to set KD_GRAPHICS) is called.
Tony
Knut Petersen wrote:
Had time to look at your tracing closely...
Quoted 8 lines
Framebuffer console is 1 with mode 1280x1024, X console is 7 and has been set to 1024x768
before X was activated.Now I do run "chvt 7;sleep 3;chvt 1" on the vt 1.
[ 972.360841] [<c0282331>] cyblafb_set_par+0x31/0xd80
The set_par call should not happen if the var of vt7 and vt1 are the same.
Quoted 13 lines
[ 972.360898] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 972.360925] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 972.360951] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 972.360989] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 972.361018] [<c02c8203>] change_console+0x43/0x90 [ 972.361043] [<c02ceebd>] console_callback+0xed/0x110 [ 972.361101] [<c0127e6a>] worker_thread+0x1ba/0x260 [ 972.361143] [<c012c115>] kthread+0x95/0xd0 [ 972.361171] [<c0100c15>] kernel_thread_helper+0x5/0x10 [ 972.361212] cyblafb: Switching to new mode: fbset -g 1024 768 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.363308] cyblafb: pixclock = 64.99 MHz, k/m/n 1 b 6e
Quoted 1 line
[ 972.363916] [<c0282331>] cyblafb_set_par+0x31/0xd80
Okay, this particular set_par() will only be called if the info mapped to vt1 and info mapped to vt7 are different. Which means you have multiple fbdevs loaded in your system.
The other reason, of course, is you have a patched fbcon_switch (yours).
Quoted 38 lines
[ 972.363945] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 972.363969] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 972.363996] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 972.364023] [<c02c8203>] change_console+0x43/0x90 [ 972.364047] [<c02ceebd>] console_callback+0xed/0x110 [ 972.364093] [<c0127e6a>] worker_thread+0x1ba/0x260 [ 972.364121] [<c012c115>] kthread+0x95/0xd0 [ 972.364143] [<c0100c15>] kernel_thread_helper+0x5/0x10 [ 972.364181] cyblafb: Switching to new mode: fbset -g 1024 768 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.366271] cyblafb: pixclock = 64.99 MHz, k/m/n 1 b 6e [ 972.366325] cyblafb: Switching to KD_GRAPHICSNot so nice. Now let´s have a look at the log entries caused switching back to
the framebuffer console:This call to
set_par()is the result of my patch:[ 975.751407] [<c0282331>] cyblafb_set_par+0x31/0xd80 [ 975.751437] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 975.751462] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 975.751486] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.751513] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.751540] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.751564] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.751587] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.751613] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.751640] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.751666] [<c01029a9>] syscall_call+0x7/0xb [ 975.751702] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.753795] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3aWithout that prior
set_par()that´s the point where the enhanced cyblafb would
have serious problems:
Quoted 13 lines
[ 975.753839] cyblafb_pan_display: entry [ 975.753853] [<c0281f7c>] cyblafb_pan_display+0x9c/0xb0 [ 975.753883] [<c027b911>] fb_pan_display+0x61/0xa0 [ 975.753925] [<c027ba4f>] fb_set_var+0xff/0x210 [ 975.753949] [<c02742fc>] fbcon_switch+0x16c/0x690 [ 975.753973] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.754000] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.754027] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.754051] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.754074] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.754100] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.754127] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.754153] [<c01029a9>] syscall_call+0x7/0xb
Yes, this is a problem. Should be easily fixed.
Quoted 4 lines
Again a call to
set_par:[ 975.754744] [<c0282331>] cyblafb_set_par+0x31/0xd80
This either came from your patch, or you have multiple fbdevs.
Quoted 13 lines
[ 975.754773] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 975.754796] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.754823] [<c02c811a>] complete_change_console+0x2a/0xd0 [ 975.754850] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.754875] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.754917] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.754943] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.754970] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.754996] [<c01029a9>] syscall_call+0x7/0xb [ 975.755032] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.757123] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
Quoted 7 lines
Now the
save_state()function is called:[ 975.757745] cyblafb: Switching to KD_TEXTand again a call to
set_par():[ 975.757759] [<c0282331>] cyblafb_set_par+0x31/0xd80
This set_par is the one that is needed. This is the point where the vt is entering KD_TEXT mode fro KD_GRAPHICS.
Quoted 14 lines
[ 975.757789] [<c027bb38>] fb_set_var+0x1e8/0x210 [ 975.757813] [<c027498a>] fbcon_blank+0x11a/0x1b0 [ 975.757837] [<c02cff17>] do_unblank_screen+0x67/0x120 [ 975.757869] [<c02c8130>] complete_change_console+0x40/0xd0 [ 975.757955] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.757979] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.758002] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.758029] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.758055] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.758081] [<c01029a9>] syscall_call+0x7/0xb [ 975.758117] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.760205] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
Quoted 3 lines
And now, the be really shure, again, a call to
set_par():[ 975.760827] [<c0282331>] cyblafb_set_par+0x31/0xd80
Again, this set_par() is either from your patch, or you have multiple fbdev's.
Quoted 15 lines
[ 975.760856] [<c0274759>] fbcon_switch+0x5c9/0x690 [ 975.760880] [<c02cc645>] redraw_screen+0xe5/0x1c0 [ 975.760923] [<c02749e6>] fbcon_blank+0x176/0x1b0 [ 975.760947] [<c02cff17>] do_unblank_screen+0x67/0x120 [ 975.760976] [<c02c8130>] complete_change_console+0x40/0xd0 [ 975.761002] [<c02c765d>] vt_ioctl+0x103d/0x19b0 [ 975.761027] [<c02c17eb>] tty_ioctl+0x18b/0x3d0 [ 975.761050] [<c016e92a>] do_ioctl+0x5a/0x90 [ 975.761076] [<c016eaeb>] vfs_ioctl+0x5b/0x1d0 [ 975.761102] [<c016eca5>] sys_ioctl+0x45/0x70 [ 975.761129] [<c01029a9>] syscall_call+0x7/0xb [ 975.761164] cyblafb: Switching to new mode: fbset -g 1280 1024 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.763280] cyblafb: pixclock = 135.00 MHz, k/m/n 0 5 3a
Quoted 1 line
First of all:
set_paris executed too often. With and without my patch.
In the final analysis, the set_par is actually called only once on an X to console switch in a typical system. The multiple set_par() present in your trace is due to:
a. different per-display var
b. multiple fbdevs
c. your patch.
In summary. For a typical system (single fbdev, all vt's with the same var) switching from X to vt should be like this, with only a single, necessary call to set_par().
(The set_par call after fbcon_blank is only present when switching from KD_GRAPHICS. This will be absent on a KD_TEXT to KD_TEXT switch).
fbcon_blank()set_par()fbcon_switch()fb_set_var()
If you have different vars, the sequence will be like this:
fbcon_blank()set_par()fbcon_switch()
fb_set_var()->set_par()
If you have multiple fbdev's, same var:
fbcon_blank()set_par()fbcon_switch()fb_set_var()set_par()
And if you have multiple fbdev's, different var's:
fbcon_blank()set_par()fbcon_switch()
fb_set_var()->set_par()set_par()
So the unnecessary (arguable) set_par calls are only present if you have multiple fbdevs/multi-head system.
Tony
Quoted 4 lines · Ville Syrjälä
However if you're really going to add more uneccessary
set_par()calls please make them configurable (eg.CONFIG_BROKEN_X). I'll go crazy if my CRT is going to start resyncing on every vt switch.
What about a flag HW_NEEDS_EARLY_SETPAR?
I think I will all something like this tomorrow.
cu,
Knut
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files for problems? Stop! Download the new AJAX search engine that makes searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
Quoted 10 lines · Knut Petersen
Quoted 6 lines · Ville Syrjälä
However if you're really going to add more uneccessary
set_par()calls please make them configurable (eg.CONFIG_BROKEN_X). I'll go crazy if my CRT is going to start resyncing on every vt switch.What about a flag
HW_NEEDS_EARLY_SETPAR?
I think I will all something like this tomorrow.
This is more acceptable.
Tony
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files for problems? Stop! Download the new AJAX search engine that makes searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
Please do forget about that patch I sent to you some days ago. An enhanced version will follow soon.
cu,
knut
Quoted 54 lines · Knut Petersen
Every framebuffer driver relies on the assumption that the
set_par()
function
of the driver is called before drawing functions and other functions dependent on the hardware state are executed.This assumption is false in two cases, and one is a genuine linux bug:
1: Whenever you switch from X to a framebuffer console for the very
first time, there is a chance that a broken X system has _not_ set the mode toKD_GRAPHICS, thus the vt and framebuffer code executes a screen redraw and several other functions before aset_par()is executed. This is believed to be not a bug of linux but a bug of X/xdm.2: Whenever a switch from X to a framebuffer console occures,
thepan_display()function of the driver is called once before theset_par()function of the driver is called. Code path:
complete_change_console->redraw_screen->fbcon_switch-> bit_update_start->fb_pan_display->xyz_pan_display. This is clearly a bug of linux.Although our primary goal must be to fix linux bugs and not to work around bugs of X, the patch fixes both of the cases.
The advantage and correctness of this patch should be obvious. Yes, it does add a possibly slow call to the
fb_set_par()function, but at this point it is necessary.Signed-off-by Knut Petersen <[email protected]>diff -uprN -X linux/Documentation/dontdiff -x '*.bak' -x '*.ctx'
linuxorig/drivers/video/console/fbcon.clinux/drivers/video/console/fbcon.c
---linuxorig/drivers/video/console/fbcon.c2005-12-02
12:18:04.000000000 +0100
+++linux/drivers/video/console/fbcon.c2005-12-06
09:06:56.000000000 +0100(quoted diff) +4
-3@@ -2103,10 +2103,11 @@ static int fbcon_switch(struct vc_data * 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); + if (old_info != NULL && old_info != info) fbcon_del_cursor_timer(old_info); + + if (info->fbops->fb_set_par) { + info->fbops->fb_set_par(info); fbcon_add_cursor_timer(info); }