Video driver bug

Video driver bug

[Samuel Rydh]

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 <samuel@ibrium.se>  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/

Re: Video driver bug

[Geert Uytterhoeven]

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/

Re: Video driver bug

[Takashi Oe]

On Sat, 7 Oct 2000, Geert Uytterhoeven wrote:

> > 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/

Re: Video driver bug

[Samuel Rydh]

On Sat, Oct 07, 2000 at 01:50:49PM -0500, Takashi Oe wrote:
>
> On Sat, 7 Oct 2000, Geert Uytterhoeven wrote:
>
> > > 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.
>

Well, using the FB_ACTIVATE_TEST only tells me whether a particular
mode is acceptable or not. I need the information encoded in the
fix structure which is only available for the active mode.

/Samuel


----------------------------------------------------------
 E-mail <samuel@ibrium.se>  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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> > 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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Mon, 9 Oct 2000, James Simmons wrote:
> > 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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Tue, 10 Oct 2000, Geert Uytterhoeven wrote:
> On Mon, 9 Oct 2000, James Simmons wrote:
> > > 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.

> > 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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> 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.

> > 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/

Re: [linux-fbdev] Re: Video driver bug

[Benjamin Herrenschmidt]

>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/

Re: [linux-fbdev] Re: Video driver bug

[Takashi Oe]

On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:

> >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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Fri, 13 Oct 2000, Takashi Oe wrote:
> On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:
>
> > >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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> > > 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/

Re: [linux-fbdev] Re: Video driver bug

[Samuel Rydh]

On Mon, Oct 16, 2000 at 05:22:58PM -0700, James Simmons wrote:
>
> > > > 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.
>
> 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.

What MOL needs from the fix structure is the bytes_per_line field and
the page_offset of framebuffer. This information is necessary in order
to make it possible to switch seamlessly (by using the MMU) from a
RAM based framebuffer to the physical one.

What I'd like to see is good way to obtain these parameters without
actually setting the video mode. And, of course, it would be nice
if they were guaranteed to be constant for a given video mode.

Cheers,

/Samuel


----------------------------------------------------------
 E-mail <samuel@ibrium.se>  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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Tue, 17 Oct 2000, Samuel Rydh wrote:
> On Mon, Oct 16, 2000 at 05:22:58PM -0700, James Simmons wrote:
> > > > > 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.
> >
> > 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.
>
> What MOL needs from the fix structure is the bytes_per_line field and
> the page_offset of framebuffer. This information is necessary in order
> to make it possible to switch seamlessly (by using the MMU) from a
> RAM based framebuffer to the physical one.

OK.

> What I'd like to see is good way to obtain these parameters without
> actually setting the video mode. And, of course, it would be nice

Currently that's possible, but it will no longer be in the future. But I guess
it's only a few modes (640x400, 800x600, 1024x768, 1280x1024), so it won't
cause too much annoyance?

Alternatively you can get the info once and store it in a config file in the
user's homedir. If it later turns out to be invalid (e.g. due to dual-head
issues, or because you changed your video card), you can offer to reprobe when
restarting MOL the next time.

> if they were guaranteed to be constant for a given video mode.

That depends on the hardware. When you're running on the second head of e.g. a
Matrox G400, it cannot be guaranteed.

So MOL should test whether those values are still valid when switching from X
to the console, and return to X when they are no longer valid.

Is this acceptable?

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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> > What I'd like to see is good way to obtain these parameters without
> > actually setting the video mode. And, of course, it would be nice
>
> Currently that's possible, but it will no longer be in the future. But I guess
> it's only a few modes (640x400, 800x600, 1024x768, 1280x1024), so it won't
> cause too much annoyance?

Only if you open /dev/fb and then do a VT_ACTIVATE. Since /dev/fb is
attached to the tty attached to the process. Well usually. What we could
do is create a ioctl that retrieves fb_fix_screeninfo and fb_var_screeninfo
at the same time. Their also exist the problem of forking with using 2
ioctls to get info the current video mode.

fb_var_screeninfo first_var;
fb_fix_screeninfo fix;

ioctl(fd, FBIOGET_VSCREENINFO, first_var);
ioctl(fd, FBIOGET_FSCREENINFO, fix);

pid = fork();

if (child) {
   fb_var_screeninfo different_var_already_made_up;

   ioctl(fd, FBIOPUT_VSCREENINFO, different_var_already_made_up;
   /* Could be wrong info depending on the card and what platform it is on:-( */
   mmap(fix->smem_start, fix->smem_len, ...);
}

The fact is also with many types of hardware setups such as dual head on
matrox as Geert pointed out it might be impossible to do what you want.


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

Re: [linux-fbdev] Re: Video driver bug

[Samuel Rydh]

On Tue, Oct 17, 2000 at 01:37:58PM +0200, Geert Uytterhoeven wrote:

> Alternatively you can get the info once and store it in a config file in the
> user's homedir. If it later turns out to be invalid (e.g. due to dual-head
> issues, or because you changed your video card), you can offer to reprobe when
> restarting MOL the next time.

OK, I'll settle for this solution. It is good to force manual confirmation
of each video mode anyway.

However, I discovered another bug in aty128fb.c. The aty_128_encode_var
sets var->activate to zero (preventing FB_ACTIVATE_TEST from
working). A simple fix:

+   int activate;
    aty128_encode_var(var, &par, info);

-   if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
+   if ((activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
        return 0;

(the bug is present in both 2.2 and 2.4).

Cheers,

/Samuel


----------------------------------------------------------
 E-mail <samuel@ibrium.se>  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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> >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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:
> >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/

Re: [linux-fbdev] Re: Video driver bug

[Samuel Rydh]

On Sat, Oct 14, 2000 at 12:09:37PM +0200, Geert Uytterhoeven wrote:
> On Fri, 13 Oct 2000, Benjamin Herrenschmidt wrote:
> > >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?

Well, that would cripple MOL considerably. A lot of MacOS applications
(admittedly most of these are games) expect to be able to change
the resolution or the depth. But the really imporant reason is that
in order to allow switching between MOL-in-a-window and console video
(which is much faster), several resolutions must be supported since
running MOL in a X-window with the same size as the screen is not very useful.

The main problem is that MacOS wants to know all the details
about the available video modes at startup. This information is also
necessary in order to make a transition possible from X to the
console.

Thus, if the API is going to change, I would really like to see a
possibility to obtain both the var and the fix information for
any supported video mode.

Cheers,

/Samuel


----------------------------------------------------------
 E-mail <samuel@ibrium.se>  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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> > 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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Tue, 10 Oct 2000, James Simmons wrote:
> > > 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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> > 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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Sat, 7 Oct 2000, Geert Uytterhoeven wrote:
> 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.

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/

Re: [linux-fbdev] Re: Video driver bug

[Benjamin Herrenschmidt]

>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/

Re: [linux-fbdev] Re: Video driver bug

[Geert Uytterhoeven]

On Sat, 14 Oct 2000, Benjamin Herrenschmidt wrote:
> >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/

Re: [linux-fbdev] Re: Video driver bug

[James Simmons]

> 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/