* [PATCH] neofb: avoid resetting display config on unblank
@ 2006-02-08 20:22 Christian Trefzer
2006-02-10 11:36 ` [PATCH] neofb: add more logic to determine sensibility of register readback Christian Trefzer
0 siblings, 1 reply; 6+ messages in thread
From: Christian Trefzer @ 2006-02-08 20:22 UTC (permalink / raw)
To: Antonino A. Daplas; +Cc: LKML
[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]
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 <ctrefzer@gmx.de>
---
--- 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 */
[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] neofb: add more logic to determine sensibility of register readback
2006-02-08 20:22 [PATCH] neofb: avoid resetting display config on unblank Christian Trefzer
@ 2006-02-10 11:36 ` Christian Trefzer
2006-02-10 11:53 ` Antonino A. Daplas
0 siblings, 1 reply; 6+ messages in thread
From: Christian Trefzer @ 2006-02-10 11:36 UTC (permalink / raw)
To: Antonino A. Daplas; +Cc: LKML
[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]
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 <ctrefzer@gmx.de>
--- 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 */
[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] neofb: add more logic to determine sensibility of register readback
2006-02-10 11:36 ` [PATCH] neofb: add more logic to determine sensibility of register readback Christian Trefzer
@ 2006-02-10 11:53 ` Antonino A. Daplas
2006-02-10 13:57 ` Christian Trefzer
2006-02-10 16:41 ` Kyle Moffett
0 siblings, 2 replies; 6+ messages in thread
From: Antonino A. Daplas @ 2006-02-10 11:53 UTC (permalink / raw)
To: Christian Trefzer; +Cc: LKML
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 <ctrefzer@gmx.de>
>
>
> --- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] neofb: add more logic to determine sensibility of register readback
2006-02-10 11:53 ` Antonino A. Daplas
@ 2006-02-10 13:57 ` Christian Trefzer
2006-02-10 16:41 ` Kyle Moffett
1 sibling, 0 replies; 6+ messages in thread
From: Christian Trefzer @ 2006-02-10 13:57 UTC (permalink / raw)
To: Antonino A. Daplas; +Cc: lkml
[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]
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 <ctrefzer@gmx.de>
--- 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 */
[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] neofb: add more logic to determine sensibility of register readback
2006-02-10 11:53 ` Antonino A. Daplas
2006-02-10 13:57 ` Christian Trefzer
@ 2006-02-10 16:41 ` Kyle Moffett
2006-02-11 10:42 ` Christian Trefzer
1 sibling, 1 reply; 6+ messages in thread
From: Kyle Moffett @ 2006-02-10 16:41 UTC (permalink / raw)
To: Antonino A. Daplas; +Cc: Christian Trefzer, LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] neofb: add more logic to determine sensibility of register readback
2006-02-10 16:41 ` Kyle Moffett
@ 2006-02-11 10:42 ` Christian Trefzer
0 siblings, 0 replies; 6+ messages in thread
From: Christian Trefzer @ 2006-02-11 10:42 UTC (permalink / raw)
To: Kyle Moffett; +Cc: Antonino A. Daplas, lkml
[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]
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 <ctrefzer@gmx.de>
--- 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 */
[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-02-11 10:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-08 20:22 [PATCH] neofb: avoid resetting display config on unblank Christian Trefzer
2006-02-10 11:36 ` [PATCH] neofb: add more logic to determine sensibility of register readback Christian Trefzer
2006-02-10 11:53 ` Antonino A. Daplas
2006-02-10 13:57 ` Christian Trefzer
2006-02-10 16:41 ` Kyle Moffett
2006-02-11 10:42 ` Christian Trefzer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox