This patch fixes issues with the NeoMagic framebuffer driver.
It nicely complements my previous fix already in linus' tree. The only
thing missing now is that the external CRT will not be activated at
neofb init when external-only is selected, either by register read or
module/kernel parameter.
Testing was done on a Dell Latitude CPi-A/NM2200 chip.
Previous behaviour:
- before booting linux, set the preferred display config X via FN+F8
- boot linux, neofb stores the register values in a private
variable
- change the display config to Y via keystroke
- leave the machine in peace until display is blanked
- touching any key will result in display config X being restored
- booting up, the BIOS will acknowledge config Y, though...
Current behaviour:
At the time of unblanking, config Y is honoured because we now read back
register contents instead of just overwriting them with outdated values.
Signed-off by: Christian Trefzer <[email protected]>
---
--- linux-2.6.15-ct3/drivers/video/neofb.c.orig 2006-02-08 19:59:39.187793848 +0100
+++ linux-2.6.15-ct3/drivers/video/neofb.c 2006-02-08 20:52:28.174719064 +0100
@@ -1334,6 +1334,9 @@
struct neofb_par *par = (struct neofb_par *)info->par;
int seqflags, lcdflags, dpmsflags, reg;
+ /* Reload the value stored in the register, might have been changed via FN keystroke */
+ par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+
switch (blank_mode) {
case FB_BLANK_POWERDOWN: /* powerdown - both sync lines down */
seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
@@ -1366,7 +1369,7 @@
case FB_BLANK_NORMAL: /* just blank screen (backlight stays on) */
seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
lcdflags = par->PanelDispCntlReg1 & 0x02; /* LCD normal */
- dpmsflags = 0; /* no hsync/vsync suppression */
+ dpmsflags = 0x00; /* no hsync/vsync suppression */
break;
case FB_BLANK_UNBLANK: /* unblank */
seqflags = 0; /* Enable sequencer */
Hello everyone,
testing my latest patches through productive use I noticed that we
require a bit of a logic for the register read to not cause trouble at
the time of unblanking. Thing is, blanking is implemented through
deactivation of the LCD activation bit, so if we read back the register,
the LCD backlight won't come back on until the LID is shut and reopened,
effectively pushing the work to the BIOS.
The following patch is on top of my previous one, adding a variable to
struct neofb_par to remember if we want to read the respective register
values next time we (un)blank.
Logic depends on the assumption that we won't change display config in a
blanked state, as a keypress should already be answered by unblanking.
Essentially, we should read back the register values to variable on
blank, and ignore them on unblank. Unfortunately, only checking for
!blank_mode won't avoid the backlight remaining off, hence the
additional checks.
Signed-off-by: Christian Trefzer <[email protected]>
--- a/include/video/neomagic.h 2006-01-03 04:21:10.000000000 +0100
+++ b/include/video/neomagic.h 2006-02-09 20:59:20.164839408 +0100
@@ -159,6 +159,7 @@
unsigned char PanelDispCntlReg1;
unsigned char PanelDispCntlReg2;
unsigned char PanelDispCntlReg3;
+ unsigned char PanelDispCntlRegRead;
unsigned char PanelVertCenterReg1;
unsigned char PanelVertCenterReg2;
unsigned char PanelVertCenterReg3;
--- a/drivers/video/neofb.c 2006-02-08 21:24:05.000000000 +0100
+++ b/drivers/video/neofb.c 2006-02-09 23:21:33.489914472 +0100
@@ -1334,8 +1334,11 @@
struct neofb_par *par = (struct neofb_par *)info->par;
int seqflags, lcdflags, dpmsflags, reg;
- /* Reload the value stored in the register, might have been changed via FN keystroke */
- par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+ /* Reload the value stored in the register, if sensible. It might have been
+ * changed via FN keystroke. */
+ if (par->PanelDispCntlRegRead && !blank_mode) {
+ par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+ }
switch (blank_mode) {
case FB_BLANK_POWERDOWN: /* powerdown - both sync lines down */
@@ -1355,21 +1358,25 @@
tosh_smm(®s);
}
#endif
+ par->PanelDispCntlRegRead = 0;
break;
case FB_BLANK_HSYNC_SUSPEND: /* hsync off */
seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
lcdflags = 0; /* LCD off */
dpmsflags = NEO_GR01_SUPPRESS_HSYNC;
+ par->PanelDispCntlRegRead = 0;
break;
case FB_BLANK_VSYNC_SUSPEND: /* vsync off */
seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
lcdflags = 0; /* LCD off */
dpmsflags = NEO_GR01_SUPPRESS_VSYNC;
+ par->PanelDispCntlRegRead = 0;
break;
case FB_BLANK_NORMAL: /* just blank screen (backlight stays on) */
seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
lcdflags = par->PanelDispCntlReg1 & 0x02; /* LCD normal */
dpmsflags = 0x00; /* no hsync/vsync suppression */
+ par->PanelDispCntlRegRead = 0;
break;
case FB_BLANK_UNBLANK: /* unblank */
seqflags = 0; /* Enable sequencer */
@@ -1387,6 +1394,7 @@
tosh_smm(®s);
}
#endif
+ par->PanelDispCntlRegRead = 1;
break;
default: /* Anything else we don't understand; return 1 to tell
* fb_blank we didn't aactually do anything */
Christian Trefzer wrote:
> Hello everyone,
>
> testing my latest patches through productive use I noticed that we
> require a bit of a logic for the register read to not cause trouble at
> the time of unblanking. Thing is, blanking is implemented through
> deactivation of the LCD activation bit, so if we read back the register,
> the LCD backlight won't come back on until the LID is shut and reopened,
> effectively pushing the work to the BIOS.
>
> The following patch is on top of my previous one, adding a variable to
> struct neofb_par to remember if we want to read the respective register
> values next time we (un)blank.
>
> Logic depends on the assumption that we won't change display config in a
> blanked state, as a keypress should already be answered by unblanking.
> Essentially, we should read back the register values to variable on
> blank, and ignore them on unblank. Unfortunately, only checking for
> !blank_mode won't avoid the backlight remaining off, hence the
> additional checks.
>
> Signed-off-by: Christian Trefzer <[email protected]>
>
>
> --- a/include/video/neomagic.h 2006-01-03 04:21:10.000000000 +0100
> +++ b/include/video/neomagic.h 2006-02-09 20:59:20.164839408 +0100
> @@ -159,6 +159,7 @@
> unsigned char PanelDispCntlReg1;
> unsigned char PanelDispCntlReg2;
> unsigned char PanelDispCntlReg3;
> + unsigned char PanelDispCntlRegRead;
> unsigned char PanelVertCenterReg1;
> unsigned char PanelVertCenterReg2;
> unsigned char PanelVertCenterReg3;
> --- a/drivers/video/neofb.c 2006-02-08 21:24:05.000000000 +0100
> +++ b/drivers/video/neofb.c 2006-02-09 23:21:33.489914472 +0100
> @@ -1334,8 +1334,11 @@
> struct neofb_par *par = (struct neofb_par *)info->par;
> int seqflags, lcdflags, dpmsflags, reg;
>
> - /* Reload the value stored in the register, might have been changed via FN keystroke */
> - par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
> + /* Reload the value stored in the register, if sensible. It might have been
> + * changed via FN keystroke. */
> + if (par->PanelDispCntlRegRead && !blank_mode) {
> + par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
> + }
>
> switch (blank_mode) {
> case FB_BLANK_POWERDOWN: /* powerdown - both sync lines down */
> @@ -1355,21 +1358,25 @@
> tosh_smm(®s);
> }
> #endif
> + par->PanelDispCntlRegRead = 0;
> break;
> case FB_BLANK_HSYNC_SUSPEND: /* hsync off */
> seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
> lcdflags = 0; /* LCD off */
> dpmsflags = NEO_GR01_SUPPRESS_HSYNC;
> + par->PanelDispCntlRegRead = 0;
> break;
> case FB_BLANK_VSYNC_SUSPEND: /* vsync off */
> seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
> lcdflags = 0; /* LCD off */
> dpmsflags = NEO_GR01_SUPPRESS_VSYNC;
> + par->PanelDispCntlRegRead = 0;
> break;
> case FB_BLANK_NORMAL: /* just blank screen (backlight stays on) */
> seqflags = VGA_SR01_SCREEN_OFF; /* Disable sequencer */
> lcdflags = par->PanelDispCntlReg1 & 0x02; /* LCD normal */
> dpmsflags = 0x00; /* no hsync/vsync suppression */
> + par->PanelDispCntlRegRead = 0;
> break;
> case FB_BLANK_UNBLANK: /* unblank */
> seqflags = 0; /* Enable sequencer */
> @@ -1387,6 +1394,7 @@
> tosh_smm(®s);
> }
> #endif
> + par->PanelDispCntlRegRead = 1;
> break;
> default: /* Anything else we don't understand; return 1 to tell
> * fb_blank we didn't aactually do anything */
You can save a few lines by
par->PanelDispCntlRegRead = (blank_mode) ? 0 : 1;
Tony
Hi Tony,
On Fri, Feb 10, 2006 at 07:53:11PM +0800, Antonino A. Daplas wrote:
> You can save a few lines by
>
> par->PanelDispCntlRegRead = (blank_mode) ? 0 : 1;
>
> Tony
>
That's just beautiful. I'm a bloody n00b when it comes to C, so I lack
the feeling for such subtleties : ) Thanks a bunch! Reworked patch
below, also adding proper initialization which got missing during
back-and-forth testing.
Signed-off-by: Christian Trefzer <[email protected]>
--- a/include/video/neomagic.h 2006-01-03 04:21:10.000000000 +0100
+++ b/include/video/neomagic.h 2006-02-09 20:59:20.164839408 +0100
@@ -159,6 +159,7 @@
unsigned char PanelDispCntlReg1;
unsigned char PanelDispCntlReg2;
unsigned char PanelDispCntlReg3;
+ unsigned char PanelDispCntlRegRead;
unsigned char PanelVertCenterReg1;
unsigned char PanelVertCenterReg2;
unsigned char PanelVertCenterReg3;
--- a/drivers/video/neofb.c 2006-02-08 21:24:05.000000000 +0100
+++ b/drivers/video/neofb.c 2006-02-10 14:54:17.312841824 +0100
@@ -843,6 +843,9 @@
par->SysIfaceCntl2 = 0xc0; /* VESA Bios sets this to 0x80! */
+ /* Initialize: by default, we want display config register to be read */
+ par->PanelDispCntlRegRead = 1;
+
/* Enable any user specified display devices. */
par->PanelDispCntlReg1 = 0x00;
if (par->internal_display)
@@ -1334,8 +1337,12 @@
struct neofb_par *par = (struct neofb_par *)info->par;
int seqflags, lcdflags, dpmsflags, reg;
- /* Reload the value stored in the register, might have been changed via FN keystroke */
- par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+ /* Reload the value stored in the register, if sensible. It might have been
+ * changed via FN keystroke. */
+ if (par->PanelDispCntlRegRead && !blank_mode) {
+ par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+ }
+ par->PanelDispCntlRegRead = (blank_mode) ? 0 : 1;
switch (blank_mode) {
case FB_BLANK_POWERDOWN: /* powerdown - both sync lines down */
On Feb 10, 2006, at 06:53, Antonino A. Daplas wrote:
> Christian Trefzer wrote:
>> + par->PanelDispCntlRegRead = 0;
>> + par->PanelDispCntlRegRead = 0;
>> + par->PanelDispCntlRegRead = 0;
>> + par->PanelDispCntlRegRead = 0;
>> + par->PanelDispCntlRegRead = 1;
>
> You can save a few lines by
>
> par->PanelDispCntlRegRead = (blank_mode) ? 0 : 1;
How about the really simple so-obvious-its-impossible-to-misread
solution?
par->PanelDispCntlRegRead = !blank_mode;
Personally I tend to get ?: constructs confused a _lot_, especially
mistaking the really short ones like x?0:1 and x?1:0. Those two are
usually better represented as !x or !!x, respectively.
Cheers,
Kyle Moffett
--
I lost interest in "blade servers" when I found they didn't throw
knives at people who weren't supposed to be in your machine room.
-- Anthony de Boer
On Fri, Feb 10, 2006 at 11:41:34AM -0500, Kyle Moffett wrote:
> On Feb 10, 2006, at 06:53, Antonino A. Daplas wrote:
> >Christian Trefzer wrote:
> >>+ par->PanelDispCntlRegRead = 0;
> >>+ par->PanelDispCntlRegRead = 0;
> >>+ par->PanelDispCntlRegRead = 0;
> >>+ par->PanelDispCntlRegRead = 0;
> >>+ par->PanelDispCntlRegRead = 1;
> >
> >You can save a few lines by
> >
> >par->PanelDispCntlRegRead = (blank_mode) ? 0 : 1;
>
> How about the really simple so-obvious-its-impossible-to-misread
> solution?
>
> par->PanelDispCntlRegRead = !blank_mode;
Where is my mind..?
Sure, why not. Guess I had my head too much in the smoke clouds of not
knowing why a dead-sure-to-work implementation still would not do the
right thing, I already thought I got the C representation of true and
false wrong. Even Tony's hint would be clearer than my code, but that
can in turn be condensed into something really obvious. It's just a
negation, after all. Silly me.
Changed patch below. Should be the final one now, right?
Signed-off-by: Christian Trefzer <[email protected]>
--- a/include/video/neomagic.h 2006-01-03 04:21:10.000000000 +0100
+++ b/include/video/neomagic.h 2006-02-09 20:59:20.164839408 +0100
@@ -159,6 +159,7 @@
unsigned char PanelDispCntlReg1;
unsigned char PanelDispCntlReg2;
unsigned char PanelDispCntlReg3;
+ unsigned char PanelDispCntlRegRead;
unsigned char PanelVertCenterReg1;
unsigned char PanelVertCenterReg2;
unsigned char PanelVertCenterReg3;
--- a/drivers/video/neofb.c 2006-02-08 21:24:05.000000000 +0100
+++ b/drivers/video/neofb.c 2006-02-11 11:39:39.346365480 +0100
@@ -843,6 +843,9 @@
par->SysIfaceCntl2 = 0xc0; /* VESA Bios sets this to 0x80! */
+ /* Initialize: by default, we want display config register to be read */
+ par->PanelDispCntlRegRead = 1;
+
/* Enable any user specified display devices. */
par->PanelDispCntlReg1 = 0x00;
if (par->internal_display)
@@ -1334,8 +1337,12 @@
struct neofb_par *par = (struct neofb_par *)info->par;
int seqflags, lcdflags, dpmsflags, reg;
- /* Reload the value stored in the register, might have been changed via FN keystroke */
- par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+ /* Reload the value stored in the register, if sensible. It might have been
+ * changed via FN keystroke. */
+ if (par->PanelDispCntlRegRead && !blank_mode) {
+ par->PanelDispCntlReg1 = vga_rgfx(NULL, 0x20) & 0x03;
+ }
+ par->PanelDispCntlRegRead = !blank_mode;
switch (blank_mode) {
case FB_BLANK_POWERDOWN: /* powerdown - both sync lines down */