Bug 5059

Summary: intelfb - do not keep console resolution
Product: Drivers Reporter: Pavel Kysilka (goldenfish)
Component: Console/FramebuffersAssignee: Antonino Daplas (adaplas)
Status: CLOSED CODE_FIX    
Severity: normal CC: sylvain.meyer
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.13-rc6 Subsystem:
Regression: --- Bisected commit-id:
Attachments: info from /var/log/messages after swithing
fb_set_var debug patch
Save info->flags in a local variable
fb ioremap patch

Description Pavel Kysilka 2005-08-13 19:54:39 UTC
Distribution: Debian testing
Hardware Environment: P4 2.4GHz,1GB RAM, integrated i865 graphics card,
Intel D865GBF Desktop Board
Software Environment: text console,X
Problem Description:
intelfb driver do not keep resolution set with fbset after switching to another
console and back.

resources: bug 4738
motherboard: http://www.intel.com/design/motherbd/bf/index.htm

Steps to reproduce:
initial options: tty1,tty2 - 1024x768-60
1) tty1 - fbset after booting (1024x768-60)
2) tty1 - fbset 800x600-100
tty1: 800x600-100
3) swith to tty2, swith to tty1
tty1: 1024x768-60 (the same resolution as default from kernel booting)
Comment 1 Pavel Kysilka 2005-08-13 19:56:31 UTC
Created attachment 5631 [details]
info from /var/log/messages after swithing
Comment 2 Pavel Kysilka 2005-08-13 20:03:23 UTC
sorry, initial resolution is 1024x768-70 (PREFFERED_MODE - intelfb.h).
s/1024x768-60/1024x768-70/g
Comment 3 Antonino Daplas 2005-08-14 02:13:34 UTC
Created attachment 5632 [details]
fb_set_var debug patch

This patch is a debugging patch which should show where it failed in
fb_set_var.
To minimize extra debugging messages, disable intelfb debugging in your Kernel
config.
Comment 4 Sylvain Meyer 2005-08-14 14:28:46 UTC
I did a fbset -g 720 576 720 576 32 from a 800x600 console
Then i switch to a new console and switch back to the first. The first isn't
reverted to 720x576 and stays at the original 800x600 resolution.

Here is the log after applying your patch, Tony.

fb_set_var: 720x576
fb_set_var: new var is 720x576
fb_set_var: activating 720x576
fb_set_var: adding mode to modelist
fb_set_var: 800x600
fb_set_var: new var is 800x600
fb_set_var: activating 800x600
fb_set_var: adding mode to modelist
fb_set_var: 800x600

I think the message "fb_set_var: notifying fbcon of modechange, flag %x" is
missing, isn't it ?

Pavel, is the problem specific to the intelfb driver ? I can't see what is wrong
here. It seems to be a fbcon bug.
Comment 5 Antonino Daplas 2005-08-14 15:53:23 UTC
Created attachment 5637 [details]
Save info->flags in a local variable

The FBINFO_MISC_USEREVENT tells core fbdev to notify fbcon of a mode change.
However, this flag was destroyed by intelfb during its set_par().  This is
inintentional though, so I don't think this is an intelfb bug as other drivers
may be affected.  The fix, then, is to save info->flags in a local variable
before calling any of the driver hooks. (A more definitive fix is to separate
info->flags into two, one that is set by the driver and another that is set by
core fbdev/fbcon).

Reverse the "fb_set_var_debug patch" first, then apply this one.  If you people
can confirm that it works for you, I'll pass it to Linus and Andrew for 2.6.13.
Comment 6 Antonino Daplas 2005-08-14 15:58:44 UTC
Sylvain,

Can you check this patch
http://bugzilla.kernel.org/attachment.cgi?id=5568&action=view in BUG 4738?  I
think that intelfb will fail with its ioremap if the graphics aperture is
greater than 128 MB.  The patch will only ioremap from the beginning of the
aperture to the end of framebuffer memory.  I don't have the hardware though, so
can you confirm if the fix is correct (or if you already have a fix)?
Comment 7 Pavel Kysilka 2005-08-14 16:45:28 UTC
Hi,
with this patch "Save info->flags in a local variable" intelfb driver keep
resolution. works fine.
I retest Matrox G550 in next hours.
Comment 8 Pavel Kysilka 2005-08-14 18:19:49 UTC
MatroxG550 working perfectly with all patches (

fbdev-add-fbset-a-support.patch
Do not ioremap the entire graphics aperture
Save info->flags in a local variable

)

I found another small error with intelfb driver.
fbset -a 800x600-100 -depth 17
                           ^^^^
Driver with this parameter is working. Switch color depth to 32bpp.
With parameter depth<0 driver calculate memory (depth = -32).
Comment 9 Antonino Daplas 2005-08-14 18:49:04 UTC
>MatroxG550 working perfectly with all patches (

>fbdev-add-fbset-a-support.patch
>Do not ioremap the entire graphics aperture
>Save info->flags in a local variable

Great. I'll push them later. I'll also wait for confirmation from Sylvain who is
the author of intelfb.

>)

>I found another small error with intelfb driver.
>fbset -a 800x600-100 -depth 17
>                           ^^^^

Normal behavior is for drivers to round _up_ to the next valid value.  So 17 was
correctly rounded up by intelfb to 32.

>Driver with this parameter is working. Switch color depth to 32bpp.
>With parameter depth<0 driver calculate memory (depth = -32).

Okay, since parameter depth is an unsigned int, all negative values are
converted to positive.  So intelfb should treat this number as invalid and
refuse to set the mode.  I think intelfb also does it correctly.  What I don't
understand is how you got a negative number (-32).
Comment 10 Pavel Kysilka 2005-08-14 20:28:17 UTC
> What I don't understand is how you got a negative number (-32).
Negative number i put in fbset application. my fault, If i write commad to
console. I make a slip (error) in console. fbset aplication allow put negative
number of color depth.
Comment 11 Antonino Daplas 2005-08-14 23:18:41 UTC
It's okay for fbset to accept negative values.  But the driver must throw this
away as invalid.
Comment 12 Sylvain Meyer 2005-08-15 05:37:57 UTC
Created attachment 5641 [details]
fb ioremap patch

Tony, I corrected the ioremap patch. Your patch enlightens me on a bug left
after applying the voffset patch. This new patch should correct the two
problems.
It seems to work like a charm on my computer.
Comment 13 Antonino Daplas 2005-08-15 06:36:08 UTC
Sylvain,

Thanks.  Hopefully this will be accepted by Linus for 2.6.13.

Pavel,

I'll close this bug.  The fix is obviously correct.