Video driver bug

Certain 2.4 video drivers (aty128, aty, platinum, tdfx, iga)
appear to be buggy. More specifically, the problem is the
following:

In the set_disp function, info->dispsw is initialized and disp->dispsw
is given the address of info->dispsw:

	static void aty128_set_disp(..)
	{
	  switch(bpp) {
	    case 8:
	        info->dispsw = accel ? fbcon_aty128_8 : fbcon_cfb8;
	        disp->dispsw = &info->dispsw;
	        break;
	   ...
	}

The problem is that the info struct is shared by all virtual consoles.
Thus if the video mode is set on a console which is not active, the
active console will be affected too. This typically results in a kernel
panic (the wrong set of console output functions is used).

This problem is observable if one starts MOL from the console. MOL
changes the video mode on an inactive console in order to extract
certain video mode parameters (like rowbytes).

One way to fix the bug is changing set_disp to the following:

	static void aty128_set_disp(..)
	{
	  switch(bpp) {
	    case 8:
	        disp->dispsw = accel ? &fbcon_aty128_8 : &fbcon_cfb8;
	        break;
	   ...
	}

This is how the code used to look like (before 2.3.43-pre5).
Does anyone know why this was changed?


Cheers,

/Samuel



----------------------------------------------------------
 E-mail [off-list ref]  WWW: <http://www.ibrium.se>
  Phone/fax: (home) +46 8 4418431, (work) +46 8 7908470
----------------------------------------------------------

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Sat, 7 Oct 2000, Samuel Rydh wrote:

Certain 2.4 video drivers (aty128, aty, platinum, tdfx, iga)
appear to be buggy. More specifically, the problem is the
following:

In the set_disp function, info->dispsw is initialized and disp->dispsw
is given the address of info->dispsw:

	static void aty128_set_disp(..)
	{
	  switch(bpp) {
	    case 8:
	        info->dispsw = accel ? fbcon_aty128_8 : fbcon_cfb8;
	        disp->dispsw = &info->dispsw;
	        break;
	   ...
	}

The problem is that the info struct is shared by all virtual consoles.
Thus if the video mode is set on a console which is not active, the
active console will be affected too. This typically results in a kernel
panic (the wrong set of console output functions is used).

You're right. *_set_disp() may be called for non-active VTs, changing
info->dispsw for the active VT.

This problem is observable if one starts MOL from the console. MOL
changes the video mode on an inactive console in order to extract
certain video mode parameters (like rowbytes).

One way to fix the bug is changing set_disp to the following:

	static void aty128_set_disp(..)
	{
	  switch(bpp) {
	    case 8:
	        disp->dispsw = accel ? &fbcon_aty128_8 : &fbcon_cfb8;
	        break;
	   ...
	}

This is how the code used to look like (before 2.3.43-pre5).
Does anyone know why this was changed?

The 2.3.44 patch shows that this line was added to struct fb_info_aty128:

+    struct display_switch dispsw;       /* for cursor and font */

While that comment isn't applicable to aty128fb, it is to atyfb (where it is
no longer present, probably it was removed in 2.1.x). When using a hardware
cursor, display_switch.{cursor,set_font} needs to be overridden, cfr.

            if (info->cursor) {
                info->dispsw.cursor = atyfb_cursor;
                info->dispsw.set_font = atyfb_set_font;
            }

in atyfb_set_disp().

I remember the original version changed the cursor and set_font fields of
disp->dispsw directly. Since this affected multiple ATI cards in your machine
(fbcon_aty* is shared), or even other video cards (disp->dispsw == fbcon_cfb*
if acceleration is disabled), I added the per-card copy of struct
display_switch to struct fb_info_aty.

Well, for aty128fb (which doesn't support a hardware cursor yet), the fix is
what you propose. However, for atyfb I see no simple solution, apart from
disabling the hardware cursor :-(

Any other takers?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Sat, 7 Oct 2000, Geert Uytterhoeven wrote:

quoted

This problem is observable if one starts MOL from the console. MOL
changes the video mode on an inactive console in order to extract
certain video mode parameters (like rowbytes).

Is there anything wrong with using FB_ACTIVATE_TEST flag for this usage?
With the flag, you get a legal var and active fb is untouched.


Takashi Oe


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

quoted

The problem is that the info struct is shared by all virtual consoles.
Thus if the video mode is set on a console which is not active, the
active console will be affected too. This typically results in a kernel
panic (the wrong set of console output functions is used).

   Oh the problems of console code interwoven with fbdev drivers. The
problem is the way mode setting happens. It goes from the fbdev layer to
the console layer when it should go in the opposite direction.

   You have to think changing mode of non visible VC means we store data
in fb_display and not set any hardware. For a visible VC we store data in
fb_info and set the hardware. It is durning VC switching that data is
moved into fb_display and new data is moved into info.

   What really needs to be done is that all handling of non visible VCs be
moved into fbcon.c and fbdev drivers deal only with the visible VC. For
mode setting of a single VC this can't be done with the current console
system :-(. For palette setting this is a different story. In fact it
is a simple patch. This does mean that color palette handling could be
moved from the fbdev layer to the console layer.

Well, for aty128fb (which doesn't support a hardware cursor yet), the fix is
what you propose. However, for atyfb I see no simple solution, apart from
disabling the hardware cursor :-(

I have some ideas but it would requires some changes to the fbcon layer.
Also it would move palette handling from fbdev to the console layer.
Would these changes be included with the 2.4.0-testX kernels?

P.S

  BTW we really should be using display_fg in fb_info more. Using currcon
has it problems in multihead enviroments and drivers that support multiple
instantances of the same card. Moving currcon into fb_info is one solution
but why not use display_fg since it already is there.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Mon, 9 Oct 2000, James Simmons wrote:

quoted

Well, for aty128fb (which doesn't support a hardware cursor yet), the fix is
what you propose. However, for atyfb I see no simple solution, apart from
disabling the hardware cursor :-(

I have some ideas but it would requires some changes to the fbcon layer.
Also it would move palette handling from fbdev to the console layer.
Would these changes be included with the 2.4.0-testX kernels?

I think we must do something in 2.4.0-testX, since you can abuse the bug to
panic the box.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Tue, 10 Oct 2000, Geert Uytterhoeven wrote:

On Mon, 9 Oct 2000, James Simmons wrote:

quoted

quoted

Well, for aty128fb (which doesn't support a hardware cursor yet), the fix is
what you propose. However, for atyfb I see no simple solution, apart from
disabling the hardware cursor :-(

I have some ideas but it would requires some changes to the fbcon layer.
Also it would move palette handling from fbdev to the console layer.

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

quoted

Would these changes be included with the 2.4.0-testX kernels?

I think we must do something in 2.4.0-testX, since you can abuse the bug to
panic the box.

Does this fix the problem on atyfb? I'm not able to test it myself.

I renamed atyfb_set_disp() to atyfb_set_dispsw() as well, since it no longer
sets up the whole display struct, but only the dispsw part.

--- linux-2.4.0-test10-pre1/drivers/video/atyfb.c.orig	Sun Sep 17 20:04:17 2000
+++ linux-2.4.0-test10-pre1/drivers/video/atyfb.c	Tue Oct 10 15:40:45 2000

@@ -466,8 +466,8 @@
 static int encode_fix(struct fb_fix_screeninfo *fix,
 		      const struct atyfb_par *par,
 		      const struct fb_info_aty *info);
-static void atyfb_set_disp(struct display *disp, struct fb_info_aty *info,
-			   int bpp, int accel);
+static void atyfb_set_dispsw(struct display *disp, struct fb_info_aty *info,
+			     int bpp, int accel);
 static int atyfb_getcolreg(u_int regno, u_int *red, u_int *green, u_int *blue,
 			 u_int *transp, struct fb_info *fb);
 static int atyfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,

@@ -2826,8 +2826,8 @@
 }


-static void atyfb_set_disp(struct display *disp, struct fb_info_aty *info,
-			   int bpp, int accel)
+static void atyfb_set_dispsw(struct display *disp, struct fb_info_aty *info,
+			     int bpp, int accel)
 {
 	    switch (bpp) {
 #ifdef FBCON_HAS_CFB8

@@ -2914,7 +2914,6 @@
 	    display->can_soft_blank = 1;
 	    display->inverse = 0;
 	    accel = var->accel_flags & FB_ACCELF_TEXT;
-	    atyfb_set_disp(display, info, par.crtc.bpp, accel);
 	    if (accel)
 	    	display->scrollmode = (info->bus_type == PCI) ? SCROLL_YNOMOVE : 0;
 	    else

@@ -2923,8 +2922,10 @@
 		(*info->fb_info.changevar)(con);
 	}
 	if (!info->fb_info.display_fg ||
-	    info->fb_info.display_fg->vc_num == con)
+	    info->fb_info.display_fg->vc_num == con) {
 	    atyfb_set_par(&par, info);
+	    atyfb_set_dispsw(display, info, par.crtc.bpp, accel);
+	}
 	if (oldbpp != var->bits_per_pixel) {
 	    if ((err = fb_alloc_cmap(&display->cmap, 0, 0)))
 		return err;

@@ -4241,8 +4242,8 @@

     atyfb_decode_var(&fb_display[con].var, &par, info);
     atyfb_set_par(&par, info);
-    atyfb_set_disp(&fb_display[con], info, par.crtc.bpp,
-		   par.accel_flags & FB_ACCELF_TEXT);
+    atyfb_set_dispsw(&fb_display[con], info, par.crtc.bpp,
+		     par.accel_flags & FB_ACCELF_TEXT);

     /* Install new colormap */
     do_install_cmap(con, fb);

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Tue, 10 Oct 2000, James Simmons wrote:

quoted

quoted

I have some ideas but it would requires some changes to the fbcon layer.
Also it would move palette handling from fbdev to the console layer.
Would these changes be included with the 2.4.0-testX kernels?

I think we must do something in 2.4.0-testX, since you can abuse the bug to
panic the box.

We really should design a cursor api. A api independent of the console so
X servers can take advantage of it. IMHO fbcon-cfbX and freinds should
go away. What I purpose is it be replaced by 3 standard functions:

fillrect
copyarea
drawimage

This with a clean cursor api woudl allow display_switch to go away.

100% agreed. But that should wait for 2.5.0. Now we have to hunt for
release-critical bugs in the current version.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

quoted

I have some ideas but it would requires some changes to the fbcon layer.
Also it would move palette handling from fbdev to the console layer.
Would these changes be included with the 2.4.0-testX kernels?

I think we must do something in 2.4.0-testX, since you can abuse the bug to
panic the box.

We really should design a cursor api. A api independent of the console so
X servers can take advantage of it. IMHO fbcon-cfbX and freinds should
go away. What I purpose is it be replaced by 3 standard functions:

fillrect
copyarea
drawimage

This with a clean cursor api woudl allow display_switch to go away.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

Yep. I still like the to see the current drivers move closer to the new
api.

quoted

I think we must do something in 2.4.0-testX, since you can abuse the bug to
panic the box.

Does this fix the problem on atyfb? I'm not able to test it myself.

I haven't tested it yet. I don't have ATI card for myself yet. I will take
a close look at what we can do. Give a few days. Right now I'm working on
a rewrite of vgacon and vga16fb. I hope to get it so we can go from vgacon
to fbcon and back again. This will help alot in driver developement. VC
switch and rrmod a driver. Regain the VC back.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

quoted

We really should design a cursor api. A api independent of the console so
X servers can take advantage of it. IMHO fbcon-cfbX and freinds should
go away. What I purpose is it be replaced by 3 standard functions:

fillrect
copyarea
drawimage

This with a clean cursor api woudl allow display_switch to go away.

100% agreed. But that should wait for 2.5.0. Now we have to hunt for
release-critical bugs in the current version.

Okay. What to get a list together of know bugs?


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

Well, my understanding is that MOL needs to be able to "probe" for all
supported mode. Doing so on a visible console would definitely be hell.
It does that in order to setup the MacOS-side driver mode list.

Ben.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:

quoted

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

Well, my understanding is that MOL needs to be able to "probe" for all
supported mode. Doing so on a visible console would definitely be hell.
It does that in order to setup the MacOS-side driver mode list.

According to Samuel's last post, MOL needs both var and fix for probing.
If there is something like:

struct fb_probe_info {
	struct fb_var_screeninfo var;
	struct fb_fix_screeninfo fix;
};

int fb_probe_mode (struct fb_probe_info *pi) {
	int err;

	pi->var.activate = FB_ACTIVATE_TEST;
	if ((err = set_var(&pi->var))<0)
		return err;
	var_to_fix(&pi->var, &pi->fix);

	return 0;
}

MOL would be happy?


Takashi Oe

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

quoted

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

Well, my understanding is that MOL needs to be able to "probe" for all
supported mode. Doing so on a visible console would definitely be hell.
It does that in order to setup the MacOS-side driver mode list.

Call ioctl FBIOPUT_VSCREENINFO with fb_var_screeninfo.activate with
FB_ACTIVATE_TEST set. This will test the mode without actually setting the
hardware.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:

quoted

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

Well, my understanding is that MOL needs to be able to "probe" for all
supported mode. Doing so on a visible console would definitely be hell.
It does that in order to setup the MacOS-side driver mode list.

Wouldn't it be sufficient to run MOL in only one mode, cfr. the X server?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Sat, 7 Oct 2000, Geert Uytterhoeven wrote:

On Sat, 7 Oct 2000, Samuel Rydh wrote:

quoted

Certain 2.4 video drivers (aty128, aty, platinum, tdfx, iga)
appear to be buggy. More specifically, the problem is the
following:

In the set_disp function, info->dispsw is initialized and disp->dispsw
is given the address of info->dispsw:

	static void aty128_set_disp(..)
	{
	  switch(bpp) {
	    case 8:
	        info->dispsw = accel ? fbcon_aty128_8 : fbcon_cfb8;
	        disp->dispsw = &info->dispsw;
	        break;
	   ...
	}

The problem is that the info struct is shared by all virtual consoles.
Thus if the video mode is set on a console which is not active, the
active console will be affected too. This typically results in a kernel
panic (the wrong set of console output functions is used).

You're right. *_set_disp() may be called for non-active VTs, changing
info->dispsw for the active VT.

Here are my fixes for aty128fb, atyfb, platinumfb, and tdfxfb.

Igafb is not affected since it sets disp during initialization only.

Can someone please test these patches so I can send them to Linus? I'm unable
to test any of them (besides a simple compile test). Thanks!

--- linux-dispsw-fix-2.4.0-test10-pre2/drivers/video/atyfb.c	Sun Sep 17 20:04:17 2000
+++ geert-dispsw-fix-2.4.0-test10-pre2/drivers/video/atyfb.c	Sat Oct 14 18:17:40 2000

@@ -466,8 +466,8 @@
 static int encode_fix(struct fb_fix_screeninfo *fix,
 		      const struct atyfb_par *par,
 		      const struct fb_info_aty *info);
-static void atyfb_set_disp(struct display *disp, struct fb_info_aty *info,
-			   int bpp, int accel);
+static void atyfb_set_dispsw(struct display *disp, struct fb_info_aty *info,
+			     int bpp, int accel);
 static int atyfb_getcolreg(u_int regno, u_int *red, u_int *green, u_int *blue,
 			 u_int *transp, struct fb_info *fb);
 static int atyfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,

@@ -2826,8 +2826,8 @@
 }


-static void atyfb_set_disp(struct display *disp, struct fb_info_aty *info,
-			   int bpp, int accel)
+static void atyfb_set_dispsw(struct display *disp, struct fb_info_aty *info,
+			     int bpp, int accel)
 {
 	    switch (bpp) {
 #ifdef FBCON_HAS_CFB8

@@ -2898,6 +2898,7 @@
 	oldbpp = display->var.bits_per_pixel;
 	oldaccel = display->var.accel_flags;
 	display->var = *var;
+	accel = var->accel_flags & FB_ACCELF_TEXT;
 	if (oldxres != var->xres || oldyres != var->yres ||
 	    oldvxres != var->xres_virtual || oldvyres != var->yres_virtual ||
 	    oldbpp != var->bits_per_pixel || oldaccel != var->accel_flags) {

@@ -2913,8 +2914,6 @@
 	    display->line_length = fix.line_length;
 	    display->can_soft_blank = 1;
 	    display->inverse = 0;
-	    accel = var->accel_flags & FB_ACCELF_TEXT;
-	    atyfb_set_disp(display, info, par.crtc.bpp, accel);
 	    if (accel)
 	    	display->scrollmode = (info->bus_type == PCI) ? SCROLL_YNOMOVE : 0;
 	    else

@@ -2923,8 +2922,10 @@
 		(*info->fb_info.changevar)(con);
 	}
 	if (!info->fb_info.display_fg ||
-	    info->fb_info.display_fg->vc_num == con)
+	    info->fb_info.display_fg->vc_num == con) {
 	    atyfb_set_par(&par, info);
+	    atyfb_set_dispsw(display, info, par.crtc.bpp, accel);
+	}
 	if (oldbpp != var->bits_per_pixel) {
 	    if ((err = fb_alloc_cmap(&display->cmap, 0, 0)))
 		return err;

@@ -4241,8 +4242,8 @@

     atyfb_decode_var(&fb_display[con].var, &par, info);
     atyfb_set_par(&par, info);
-    atyfb_set_disp(&fb_display[con], info, par.crtc.bpp,
-		   par.accel_flags & FB_ACCELF_TEXT);
+    atyfb_set_dispsw(&fb_display[con], info, par.crtc.bpp,
+		     par.accel_flags & FB_ACCELF_TEXT);

     /* Install new colormap */
     do_install_cmap(con, fb);
--- linux-dispsw-fix-2.4.0-test10-pre2/drivers/video/tdfxfb.c	Fri Jul 28 21:19:20 2000
+++ geert-dispsw-fix-2.4.0-test10-pre2/drivers/video/tdfxfb.c	Sat Oct 14 17:29:21 2000

@@ -327,7 +327,6 @@
   struct tdfxfb_par default_par;
   struct tdfxfb_par current_par;
   struct display disp;
-  struct display_switch dispsw;
 #if defined(FBCON_HAS_CFB16) || defined(FBCON_HAS_CFB24) || defined(FBCON_HAS_CFB32)
   union {
 #ifdef FBCON_HAS_CFB16

@@ -412,10 +411,10 @@
 static int  tdfxfb_encode_fix(struct fb_fix_screeninfo* fix,
 			      const struct tdfxfb_par* par,
 			      const struct fb_info_tdfx* info);
-static void tdfxfb_set_disp(struct display* disp,
-			    struct fb_info_tdfx* info,
-			    int bpp,
-			    int accel);
+static void tdfxfb_set_dispsw(struct display* disp,
+			      struct fb_info_tdfx* info,
+			      int bpp,
+			      int accel);
 static int  tdfxfb_getcolreg(u_int regno,
 			     u_int* red,
 			     u_int* green,

@@ -1640,48 +1639,43 @@
   return 0;
 }

-static void tdfxfb_set_disp(struct display *disp,
-			    struct fb_info_tdfx *info,
-			    int bpp,
-			    int accel) {
+static void tdfxfb_set_dispsw(struct display *disp,
+			      struct fb_info_tdfx *info,
+			      int bpp,
+			      int accel) {

   if (disp->dispsw && disp->conp)
      fb_con.con_cursor(disp->conp, CM_ERASE);
   switch(bpp) {
 #ifdef FBCON_HAS_CFB8
   case 8:
-    info->dispsw = noaccel ? fbcon_cfb8 : fbcon_banshee8;
-    disp->dispsw = &info->dispsw;
+    disp->dispsw = noaccel ? &fbcon_cfb8 : &fbcon_banshee8;
     if (nohwcursor) fbcon_banshee8.cursor = NULL;
     break;
 #endif
 #ifdef FBCON_HAS_CFB16
   case 16:
-    info->dispsw = noaccel ? fbcon_cfb16 : fbcon_banshee16;
-    disp->dispsw = &info->dispsw;
+    disp->dispsw = noaccel ? &fbcon_cfb16 : &fbcon_banshee16;
     disp->dispsw_data = info->fbcon_cmap.cfb16;
     if (nohwcursor) fbcon_banshee16.cursor = NULL;
     break;
 #endif
 #ifdef FBCON_HAS_CFB24
   case 24:
-    info->dispsw = noaccel ? fbcon_cfb24 : fbcon_banshee24;
-    disp->dispsw = &info->dispsw;
+    disp->dispsw = noaccel ? &fbcon_cfb24 : &fbcon_banshee24;
     disp->dispsw_data = info->fbcon_cmap.cfb24;
     if (nohwcursor) fbcon_banshee24.cursor = NULL;
     break;
 #endif
 #ifdef FBCON_HAS_CFB32
   case 32:
-    info->dispsw = noaccel ? fbcon_cfb32 : fbcon_banshee32;
-    disp->dispsw = &info->dispsw;
+    disp->dispsw = noaccel ? &fbcon_cfb32 : &fbcon_banshee32;
     disp->dispsw_data = info->fbcon_cmap.cfb32;
     if (nohwcursor) fbcon_banshee32.cursor = NULL;
     break;
 #endif
   default:
-    info->dispsw = fbcon_dummy;
-    disp->dispsw = &info->dispsw;
+    disp->dispsw = &fbcon_dummy;
   }

 }

@@ -1735,7 +1729,7 @@
 	 display->can_soft_blank = 1;
 	 display->inverse        = inverse;
 	 accel = var->accel_flags & FB_ACCELF_TEXT;
-	 tdfxfb_set_disp(display, info, par.bpp, accel);
+	 tdfxfb_set_dispsw(display, info, par.bpp, accel);

 	 if(nopan) display->scrollmode = SCROLL_YREDRAW;

@@ -2083,10 +2077,10 @@

    info->cursor.redraw=1;

-   tdfxfb_set_disp(&fb_display[con],
-		   info,
-		   par.bpp,
-		   par.accel_flags & FB_ACCELF_TEXT);
+   tdfxfb_set_dispsw(&fb_display[con],
+		     info,
+		     par.bpp,
+		     par.accel_flags & FB_ACCELF_TEXT);

    tdfxfb_install_cmap(&fb_display[con], fb);
    tdfxfb_updatevar(con, fb);
--- linux-dispsw-fix-2.4.0-test10-pre2/drivers/video/aty128fb.c	Sun Sep 17 20:04:17 2000
+++ geert-dispsw-fix-2.4.0-test10-pre2/drivers/video/aty128fb.c	Sat Oct 14 17:32:10 2000

@@ -283,7 +283,6 @@
     const struct aty128_meminfo *mem;   /* onboard mem info    */
     struct aty128fb_par default_par, current_par;
     struct display disp;
-    struct display_switch dispsw;       /* for cursor and font */
     struct { u8 red, green, blue, pad; } palette[256];
     union {
 #ifdef FBCON_HAS_CFB16

@@ -347,7 +346,7 @@
 static void aty128_encode_fix(struct fb_fix_screeninfo *fix,
 				struct aty128fb_par *par,
 				const struct fb_info_aty128 *info);
-static void aty128_set_disp(struct display *disp,
+static void aty128_set_dispsw(struct display *disp,
 			struct fb_info_aty128 *info, int bpp, int accel);
 static int aty128_getcolreg(u_int regno, u_int *red, u_int *green, u_int *blue,
 				u_int *transp, struct fb_info *info);

@@ -1392,7 +1391,7 @@
 	display->inverse = 0;

 	accel = var->accel_flags & FB_ACCELF_TEXT;
-        aty128_set_disp(display, info, par.crtc.bpp, accel);
+        aty128_set_dispsw(display, info, par.crtc.bpp, accel);

 	if (accel)
 	    display->scrollmode = SCROLL_YNOMOVE;

@@ -1417,35 +1416,31 @@


 static void
-aty128_set_disp(struct display *disp,
+aty128_set_dispsw(struct display *disp,
 			struct fb_info_aty128 *info, int bpp, int accel)
 {
     switch (bpp) {
 #ifdef FBCON_HAS_CFB8
     case 8:
-	info->dispsw = accel ? fbcon_aty128_8 : fbcon_cfb8;
-        disp->dispsw = &info->dispsw;
+	disp->dispsw = accel ? &fbcon_aty128_8 : &fbcon_cfb8;
 	break;
 #endif
 #ifdef FBCON_HAS_CFB16
     case 15:
     case 16:
-	info->dispsw = accel ? fbcon_aty128_16 : fbcon_cfb16;
-        disp->dispsw = &info->dispsw;
+	disp->dispsw = accel ? &fbcon_aty128_16 : &fbcon_cfb16;
 	disp->dispsw_data = info->fbcon_cmap.cfb16;
 	break;
 #endif
 #ifdef FBCON_HAS_CFB24
     case 24:
-	info->dispsw = accel ? fbcon_aty128_24 : fbcon_cfb24;
-        disp->dispsw = &info->dispsw;
+	disp->dispsw = accel ? &fbcon_aty128_24 : &fbcon_cfb24;
 	disp->dispsw_data = info->fbcon_cmap.cfb24;
 	break;
 #endif
 #ifdef FBCON_HAS_CFB32
     case 32:
-	info->dispsw = accel ? fbcon_aty128_32 : fbcon_cfb32;
-        disp->dispsw = &info->dispsw;
+	disp->dispsw = accel ? &fbcon_aty128_32 : &fbcon_cfb32;
 	disp->dispsw_data = info->fbcon_cmap.cfb32;
 	break;
 #endif

@@ -2135,7 +2130,7 @@
     aty128_decode_var(&fb_display[con].var, &par, info);
     aty128_set_par(&par, info);

-    aty128_set_disp(&fb_display[con], info, par.crtc.bpp,
+    aty128_set_dispsw(&fb_display[con], info, par.crtc.bpp,
         par.accel_flags & FB_ACCELF_TEXT);

     do_install_cmap(con, fb);
--- linux-dispsw-fix-2.4.0-test10-pre2/drivers/video/platinumfb.c	Sat Aug  5 14:20:03 2000
+++ geert-dispsw-fix-2.4.0-test10-pre2/drivers/video/platinumfb.c	Sat Oct 14 17:30:22 2000

@@ -65,7 +65,6 @@
 struct fb_info_platinum {
 	struct fb_info			fb_info;
 	struct display			disp;
-	struct display_switch		dispsw;
 	struct fb_par_platinum		default_par;
 	struct fb_par_platinum		current_par;

@@ -140,8 +139,9 @@
 static int platinum_encode_fix(struct fb_fix_screeninfo *fix,
 			       const struct fb_par_platinum *par,
 			       const struct fb_info_platinum *info);
-static void platinum_set_disp(struct display *disp, struct fb_info_platinum *info,
-			      int cmode, int accel);
+static void platinum_set_dispsw(struct display *disp,
+				struct fb_info_platinum *info, int cmode,
+				int accel);
 static int platinum_getcolreg(u_int regno, u_int *red, u_int *green, u_int *blue,
 			      u_int *transp, struct fb_info *fb);
 static int platinum_setcolreg(u_int regno, u_int red, u_int green, u_int blue,

@@ -193,27 +193,25 @@
 	return 0;
 }

-static void platinum_set_disp(struct display *disp, struct fb_info_platinum *info,
-			      int cmode, int accel)
+static void platinum_set_dispsw(struct display *disp,
+				struct fb_info_platinum *info, int cmode,
+				int accel)
 {
 	switch(cmode) {
 #ifdef FBCON_HAS_CFB8
 	    case CMODE_8:
-		info->dispsw = fbcon_cfb8;
-		disp->dispsw = &info->dispsw;
+		disp->dispsw = &fbcon_cfb8;
 		break;
 #endif
 #ifdef FBCON_HAS_CFB16
 	    case CMODE_16:
-		info->dispsw = fbcon_cfb16;
-		disp->dispsw = &info->dispsw;
+		disp->dispsw = &fbcon_cfb16;
 		disp->dispsw_data = info->fbcon_cmap.cfb16;
 		break;
 #endif
 #ifdef FBCON_HAS_CFB32
 	    case CMODE_32:
-		info->dispsw = fbcon_cfb32;
-		disp->dispsw = &info->dispsw;
+		disp->dispsw = &fbcon_cfb32;
 		disp->dispsw_data = info->fbcon_cmap.cfb32;
 		break;
 #endif

@@ -271,7 +269,7 @@
 	    display->line_length = fix.line_length;
 	    display->can_soft_blank = 1;
 	    display->inverse = 0;
-	    platinum_set_disp(display, info, par.cmode, 0);
+	    platinum_set_dispsw(display, info, par.cmode, 0);
 	    display->scrollmode = SCROLL_YREDRAW;
 	    if (info->fb_info.changevar)
 	      (*info->fb_info.changevar)(con);

@@ -341,7 +339,7 @@

 	platinum_var_to_par(&fb_display[con].var, &par, info);
 	platinum_set_par(&par, info);
-	platinum_set_disp(&fb_display[con], info, par.cmode, 0);
+	platinum_set_dispsw(&fb_display[con], info, par.cmode, 0);
 	do_install_cmap(con, fb);

 	return 1;

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Fri, 13 Oct 2000, Takashi Oe wrote:

On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:

quoted

quoted

That's for 2.5.0. I think the first thing we need to do in 2.5.0 is to remove
all `mess with non-visible VCs' support, so /dev/fb* works on the _current_
video mode.

Well, my understanding is that MOL needs to be able to "probe" for all
supported mode. Doing so on a visible console would definitely be hell.
It does that in order to setup the MacOS-side driver mode list.

According to Samuel's last post, MOL needs both var and fix for probing.
If there is something like:

struct fb_probe_info {
	struct fb_var_screeninfo var;
	struct fb_fix_screeninfo fix;
};

Why does it need fix? Relying on data from fix for a mode that is not yet
really set is not wise: there's no guarantee that fix will be the same when the
mode is really set later.

Example: dual-head cards, where the resolution on one head is changed so the
card's memory management is changed.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Here are my fixes for aty128fb, atyfb, platinumfb, and tdfxfb.

Igafb is not affected since it sets disp during initialization only.

Can someone please test these patches so I can send them to Linus? I'm unable
to test any of them (besides a simple compile test). Thanks!

While we are at it, I beleive we should also change atyfb.c so that
currcon is no longer a global. My understanding is that can break multihead.

Any objection ?

Ben.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Sat, 14 Oct 2000, Benjamin Herrenschmidt wrote:

quoted

Here are my fixes for aty128fb, atyfb, platinumfb, and tdfxfb.

Igafb is not affected since it sets disp during initialization only.

Can someone please test these patches so I can send them to Linus? I'm unable
to test any of them (besides a simple compile test). Thanks!

While we are at it, I beleive we should also change atyfb.c so that
currcon is no longer a global. My understanding is that can break multihead.

Any objection ?

No.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

While we are at it, I beleive we should also change atyfb.c so that
currcon is no longer a global. My understanding is that can break multihead.

That is not the only thing broken for multihead support. The softback code
is severly broken. Also why don't we use the display_fg field in fb_info
instead. display_fg->vc_num is already their is it's more multihead
friendly. IMO real multihead support shoudl wait until 2.5.X since it
needs a pretty big cleanup.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

quoted

quoted

Well, my understanding is that MOL needs to be able to "probe" for all
supported mode. Doing so on a visible console would definitely be hell.
It does that in order to setup the MacOS-side driver mode list.

According to Samuel's last post, MOL needs both var and fix for probing.
If there is something like:

struct fb_probe_info {
	struct fb_var_screeninfo var;
	struct fb_fix_screeninfo fix;
};

Why does it need fix? Relying on data from fix for a mode that is not yet
really set is not wise: there's no guarantee that fix will be the same when the
mode is really set later.

Example: dual-head cards, where the resolution on one head is changed so the
card's memory management is changed.

Yes this is really bad. The way most drivers are written is that testing
var does not change fix. If you grab fix after testing the video mode it
will be for the current set resolution. Not the one you are testing
for. For most drivers you need to physically set the mode to change
fix. Sometimes the information in fix can only be obtained from the
hardware and this requires a video mode change.
   I do agree their are problems with fix and var being grabbed by
seperate ioctl calls. This can cause really problems (think fork). But
the solution is much more complex than just combining both fix and var
into one ioctl. Consider the situation with several apps having /dev/fbX
open and one app changes the video mode. Since changing the video mode
will change var and fix not only for the app that changed the video mode
but also for the other apps depending on the values it obtains for var and
fix. Now they will have the wrong values. Their are other issues as well.
As you can see it is more complex than what you think.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/