From mboxrd@z Thu Jan 1 00:00:00 1970 From: Knut Petersen Subject: Re: [PATCH 1/1 2.6.15-rc4-git1] Fix switching to KD_TEXT Date: Fri, 09 Dec 2005 12:53:27 +0100 Message-ID: <43997037.4020206@t-online.de> References: <4398B888.50005@t-online.de> <4398CEAF.9050303@gmail.com> Mime-Version: 1.0 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <4398CEAF.9050303@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: Cc: linux-kernel@vger.kernel.org, linux-fbdev-devel@lists.sourceforge.net, Andrew Morton Hi Tony! >Sorry, NAK for now. Unless other people agree that it is okay for the= m >to have an unconditional call to set_par() for every console switch. N= ote >that the set_par() in some drivers is terribly slow (several seconds a= t least). > >Let's wait a few days, if nobody disagrees with you, so be it. > > =20 > Which hardware needs several seconds? I do run relatively slow hardware= ,=20 but a complete set_par() needs less than 0.003 seconds with cyblafb. If there really is hardware that needs _several_ seconds for _one_=20 set_par(), then we should really be more carefull when to call that function. Let=B4s have a look at a switch from the framebuffer console to X and b= ack. Kernel is a 2.6.15-rc4-git1 with my current cyblafb + your patches=20 needed for cyblafb + the patch we are talking about. =46ramebuffer console is 1 with mode 1280x1024, X console is 7 and has=20 been set to 1024x768 before X was activated. Now I do run "chvt 7;sleep 3;chvt 1" on the vt 1. [ 972.360841] [] cyblafb_set_par+0x31/0xd80 [ 972.360898] [] fb_set_var+0x1e8/0x210 [ 972.360925] [] fbcon_switch+0x16c/0x690 [ 972.360951] [] redraw_screen+0xe5/0x1c0 [ 972.360989] [] complete_change_console+0x2a/0xd0 [ 972.361018] [] change_console+0x43/0x90 [ 972.361043] [] console_callback+0xed/0x110 [ 972.361101] [] worker_thread+0x1ba/0x260 [ 972.361143] [] kthread+0x95/0xd0 [ 972.361171] [] kernel_thread_helper+0x5/0x10 [ 972.361212] cyblafb: Switching to new mode: fbset -g 1024 768=20 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.363308] cyblafb: pixclock =3D 64.99 MHz, k/m/n 1 b 6e [ 972.363916] [] cyblafb_set_par+0x31/0xd80 [ 972.363945] [] fbcon_switch+0x5c9/0x690 [ 972.363969] [] redraw_screen+0xe5/0x1c0 [ 972.363996] [] complete_change_console+0x2a/0xd0 [ 972.364023] [] change_console+0x43/0x90 [ 972.364047] [] console_callback+0xed/0x110 [ 972.364093] [] worker_thread+0x1ba/0x260 [ 972.364121] [] kthread+0x95/0xd0 [ 972.364143] [] kernel_thread_helper+0x5/0x10 [ 972.364181] cyblafb: Switching to new mode: fbset -g 1024 768=20 1024 768 8 -t 15385 160 24 29 3 136 6 [ 972.366271] cyblafb: pixclock =3D 64.99 MHz, k/m/n 1 b 6e [ 972.366325] cyblafb: Switching to KD_GRAPHICS Not so nice. Now let=B4s have a look at the log entries caused switchin= g=20 back to the framebuffer console: This call to set_par() is the result of my patch: [ 975.751407] [] cyblafb_set_par+0x31/0xd80 [ 975.751437] [] fb_set_var+0x1e8/0x210 [ 975.751462] [] fbcon_switch+0x16c/0x690 [ 975.751486] [] redraw_screen+0xe5/0x1c0 [ 975.751513] [] complete_change_console+0x2a/0xd0 [ 975.751540] [] vt_ioctl+0x103d/0x19b0 [ 975.751564] [] tty_ioctl+0x18b/0x3d0 [ 975.751587] [] do_ioctl+0x5a/0x90 [ 975.751613] [] vfs_ioctl+0x5b/0x1d0 [ 975.751640] [] sys_ioctl+0x45/0x70 [ 975.751666] [] syscall_call+0x7/0xb [ 975.751702] cyblafb: Switching to new mode: fbset -g 1280 1024=20 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.753795] cyblafb: pixclock =3D 135.00 MHz, k/m/n 0 5 3a Without that prior set_par() that=B4s the point where the enhanced cybl= afb=20 would have serious problems: [ 975.753839] cyblafb_pan_display: entry [ 975.753853] [] cyblafb_pan_display+0x9c/0xb0 [ 975.753883] [] fb_pan_display+0x61/0xa0 [ 975.753925] [] fb_set_var+0xff/0x210 [ 975.753949] [] fbcon_switch+0x16c/0x690 [ 975.753973] [] redraw_screen+0xe5/0x1c0 [ 975.754000] [] complete_change_console+0x2a/0xd0 [ 975.754027] [] vt_ioctl+0x103d/0x19b0 [ 975.754051] [] tty_ioctl+0x18b/0x3d0 [ 975.754074] [] do_ioctl+0x5a/0x90 [ 975.754100] [] vfs_ioctl+0x5b/0x1d0 [ 975.754127] [] sys_ioctl+0x45/0x70 [ 975.754153] [] syscall_call+0x7/0xb Again a call to set_par: [ 975.754744] [] cyblafb_set_par+0x31/0xd80 [ 975.754773] [] fbcon_switch+0x5c9/0x690 [ 975.754796] [] redraw_screen+0xe5/0x1c0 [ 975.754823] [] complete_change_console+0x2a/0xd0 [ 975.754850] [] vt_ioctl+0x103d/0x19b0 [ 975.754875] [] tty_ioctl+0x18b/0x3d0 [ 975.754917] [] do_ioctl+0x5a/0x90 [ 975.754943] [] vfs_ioctl+0x5b/0x1d0 [ 975.754970] [] sys_ioctl+0x45/0x70 [ 975.754996] [] syscall_call+0x7/0xb [ 975.755032] cyblafb: Switching to new mode: fbset -g 1280 1024=20 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.757123] cyblafb: pixclock =3D 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] [] cyblafb_set_par+0x31/0xd80 [ 975.757789] [] fb_set_var+0x1e8/0x210 [ 975.757813] [] fbcon_blank+0x11a/0x1b0 [ 975.757837] [] do_unblank_screen+0x67/0x120 [ 975.757869] [] complete_change_console+0x40/0xd0 [ 975.757955] [] vt_ioctl+0x103d/0x19b0 [ 975.757979] [] tty_ioctl+0x18b/0x3d0 [ 975.758002] [] do_ioctl+0x5a/0x90 [ 975.758029] [] vfs_ioctl+0x5b/0x1d0 [ 975.758055] [] sys_ioctl+0x45/0x70 [ 975.758081] [] syscall_call+0x7/0xb [ 975.758117] cyblafb: Switching to new mode: fbset -g 1280 1024=20 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.760205] cyblafb: pixclock =3D 135.00 MHz, k/m/n 0 5 3a And now, the be really shure, again, a call to set_par(): [ 975.760827] [] cyblafb_set_par+0x31/0xd80 [ 975.760856] [] fbcon_switch+0x5c9/0x690 [ 975.760880] [] redraw_screen+0xe5/0x1c0 [ 975.760923] [] fbcon_blank+0x176/0x1b0 [ 975.760947] [] do_unblank_screen+0x67/0x120 [ 975.760976] [] complete_change_console+0x40/0xd0 [ 975.761002] [] vt_ioctl+0x103d/0x19b0 [ 975.761027] [] tty_ioctl+0x18b/0x3d0 [ 975.761050] [] do_ioctl+0x5a/0x90 [ 975.761076] [] vfs_ioctl+0x5b/0x1d0 [ 975.761102] [] sys_ioctl+0x45/0x70 [ 975.761129] [] syscall_call+0x7/0xb [ 975.761164] cyblafb: Switching to new mode: fbset -g 1280 1024=20 2048 4096 8 -t 7407 232 16 39 0 160 3 [ 975.763280] cyblafb: pixclock =3D 135.00 MHz, k/m/n 0 5 3a =46irst of all: set_par is executed too often. With and without my patc= h. If we would know that vt x is nothing but the vt used for X, we could=20 introduce a policy to completely ignore all framebuffer operations for that console. That=20 also would result in a much nicer look of the modeswitch. Do we need a new kernel=20 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=B4m shure t= hat=20 you do agree. We do need _one_ call to set_par() etc, but this must be at the right=20 place. And the right place is somewhere before any other driver operation is called, with th= e=20 exception of check_var() and init functions obviously. Think about your NAK carefully. I might submit a patch to skeletonfb.c=20 that inserts a warning at all places that might be unexpectedly called with a=20 hardware state different to that set by set_par(). ;-)))) And please don=B4t argue that X or certain releases are broken. That is= =20 true, but ordinary users will use those broken versions for years. cu, Knut