matroxfb console oops in 2.4.2x

matroxfb console oops in 2.4.2x

[David Woodhouse]

If you have matroxfb with acceleration enabled but no other console, and
set console=tty0 on the command line, it dies because its putcs
functions are called before matroxfb_init_putc() is called.

Below is a workaround which lets the machine boot. It's obviously not a
fix.

--- drivers/video/matrox/matroxfb_accel.c~	Wed Jun 18 17:16:40 2003
+++ drivers/video/matrox/matroxfb_accel.c	Thu Jun 19 10:57:54 2003
@@ -487,6 +487,9 @@
 
 	DBG_HEAVY("matroxfb_cfb8_putc");
 
+	if (!ACCESS_FBINFO(curr.putc))
+		return;
+
 	fgx = attr_fgcol(p, c);
 	bgx = attr_bgcol(p, c);
 	fgx |= (fgx << 8);
@@ -504,6 +507,9 @@
 
 	DBG_HEAVY("matroxfb_cfb16_putc");
 
+	if (!ACCESS_FBINFO(curr.putc))
+		return;
+
 	fgx = ((u_int16_t*)p->dispsw_data)[attr_fgcol(p, c)];
 	bgx = ((u_int16_t*)p->dispsw_data)[attr_bgcol(p, c)];
 	fgx |= (fgx << 16);
@@ -519,6 +525,9 @@
 
 	DBG_HEAVY("matroxfb_cfb32_putc");
 
+	if (!ACCESS_FBINFO(curr.putc))
+		return;
+
 	fgx = ((u_int32_t*)p->dispsw_data)[attr_fgcol(p, c)];
 	bgx = ((u_int32_t*)p->dispsw_data)[attr_bgcol(p, c)];
 	ACCESS_FBINFO(curr.putc)(fgx, bgx, p, c, yy, xx);
@@ -662,6 +671,9 @@
 
 	DBG_HEAVY("matroxfb_cfb8_putcs");
 
+	if (!ACCESS_FBINFO(curr.putcs))
+		return;
+
 	c = scr_readw(s);
 	fgx = attr_fgcol(p, c);
 	bgx = attr_bgcol(p, c);
@@ -681,6 +693,9 @@
 
 	DBG_HEAVY("matroxfb_cfb16_putcs");
 
+	if (!ACCESS_FBINFO(curr.putcs))
+		return;
+
 	c = scr_readw(s);
 	fgx = ((u_int16_t*)p->dispsw_data)[attr_fgcol(p, c)];
 	bgx = ((u_int16_t*)p->dispsw_data)[attr_bgcol(p, c)];
@@ -697,6 +712,9 @@
 	MINFO_FROM_DISP(p);
 
 	DBG_HEAVY("matroxfb_cfb32_putcs");
+
+	if (!ACCESS_FBINFO(curr.putcs))
+		return;
 
 	c = scr_readw(s);
 	fgx = ((u_int32_t*)p->dispsw_data)[attr_fgcol(p, c)];





-- 
dwmw2

Re: matroxfb console oops in 2.4.2x

[Petr Vandrovec]

On 24 Jun 03 at 10:09, David Woodhouse wrote:
> On Thu, 2003-06-19 at 20:45, Petr Vandrovec wrote:
> > This one is culprit. If you'll comment this message out, it will not
> > crash.
> 
> As discussed, this is true but if anyone _else_ happens to call printk
> during the same period, it'll still crash. 
> 
> Matroxfb is registering a screen for which it's not yet willing to
> attempt output -- and taking out this printk only serves to fix the
> coincidence which makes it 100% reproducible -- the thing is still
> broken without that printk.

There is no way around - at least I do not know such. matroxfb cannot 
initialize hardware before call to the set_var, as otherwise you'll 
get white 80x25 square instead of vgacon text copied to the matroxfb.

If you'll replace matroxfb_set_var(..., -2, &ACCESS_FBINFO(fbcon))
with matroxfb_set_var(..., -1, &ACCESS_FBINFO(fbcon)) in matroxfb_base.c,
you'll get behavior you are asking for: hardware gets initialized before
call to register_framebuffer(). Unfortunately it breaks 
take_over_console for all vgacon users - and as there is more vgacon
users than sh users, I prefer just creating dummy putc/putcs functions
which will do nothing, over changing initialization order.

I'm still trying to find why PLL does not lock on your hardware,
but I still do not understand why it works on ia32 for secondary adapters,
but not on sh.
                                                    Petr Vandrovec
                                                    vandrove@vc.cvut.cz
                                                    

Re: matroxfb console oops in 2.4.2x

[David Woodhouse]

On Thu, 2003-06-19 at 20:45, Petr Vandrovec wrote:
> This one is culprit. If you'll comment this message out, it will not
> crash.

As discussed, this is true but if anyone _else_ happens to call printk
during the same period, it'll still crash. 

Matroxfb is registering a screen for which it's not yet willing to
attempt output -- and taking out this printk only serves to fix the
coincidence which makes it 100% reproducible -- the thing is still
broken without that printk.

-- 
dwmw2

Re: matroxfb console oops in 2.4.2x

[David Woodhouse]

On Thu, 2003-06-19 at 20:45, Petr Vandrovec wrote:
> On 19 Jun 03 at 14:47, David Woodhouse wrote:
> > On Thu, 2003-06-19 at 13:21, Petr Vandrovec wrote:
> 
> > take_over_console() attempts to redraw the screen.
> 
> It is not take_over_console... It does init first. 

Hmmm yes. The 'Pixel PLL not locked' message happens in the middle of
take_over_console(), so if I do something stupid like trying to debug
using printk instead of GDB, I get...

<7>take_over_console starts
<7>take_over_console calls save_screen()
 <crash>

... but if I make the Pixel PLL message KERN_DEBUG too, it continues...

<7>matroxfb: Pixel PLL not locked after 5 secs
<7>take_over_console calls update_screen()...
<4>Console: switching to colour frame buffer device 160x64
<7>take_over_console ends

> This one is culprit. If you'll comment this message out, it will not
> crash.

Indeed -- it won't crash _today_ if I do that.

But the problem is not recursion -- the problem is that there is a time
window during which _any_ printk (which could be from a timer or
interrupt) would kill the box, because it seems the console is getting
registered and allowing output before the hardware is fully initialised
and ready to accept it.

I don't know whether you consider this a bug in the generic console
code, or a bug in matroxfb -- but removing the 'Pixel PLL not locked'
message is only a workaround which serves to mask the real problem, so
I'd advocate leaving it in for now until the problem is fixed correctly.

> Ok. It means that hardware is completely uninitialized when this happens.
> Probably accelerator clocks are stopped (== message about pixclocks was
> right...) Bad.

The hardware _was_ completely uninitialised before matroxfb started,
certainly. 

I'd sort of expect it to have been initialised before we attempt to
register it as a console device though -- and even if it's OK to
register a partially-initialised device because the console registration
is supposed to perform the final initialisation by setting modes, etc.,
then I'd _still_ expect it to be initialised before the console code
actually lets any printk() through...

> Does driver work with your change without problems? It looks strange
> to me that PLL did not stabilized in 5 seconds. Do you get same message
> when you change videomode with fbset, or happens this only once during
> boot, and never again?

Once during boot and never again.

-- 
dwmw2

Re: matroxfb console oops in 2.4.2x

[David Woodhouse]

On Thu, 2003-06-19 at 20:45, Petr Vandrovec wrote:
> On 19 Jun 03 at 14:47, David Woodhouse wrote:
> > On Thu, 2003-06-19 at 13:21, Petr Vandrovec wrote:
> 
> > take_over_console() attempts to redraw the screen.
> 
> It is not take_over_console... It does init first. 

Hmm. OK. For some reason my two-year-old GDB and KGDB between them
aren't giving me a sensible backtrace today so I can't see precisely
what's happening. I accused take_over_console because my vague memory of
a debugging printk session the other day led me to believe that was the
case.

take_over_console (actually update_region()) is definitely doing
_something_ bizarre... there have been 34 lines of printk output since
boot, and it's rendering the 34th line 34 times, one on each of the top
34 lines of the new console.

> > >  
> > matroxfb: Pixel PLL not locked after 5 secs
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   
> This one is culprit. If you'll comment this message out, it will not
> crash.

OK; I'll try that in the morning.

> > Console: switching to colour frame buffer device 160x64
> > fb0: MATROX VGA frame buffer device
> > 
> > If I call matrox_init_putc() earlier as you suggest, then it seems to
> > end up busy-waiting in mga_fifo()...
> 
> Ok. It means that hardware is completely uninitialized when this happens.
> Probably accelerator clocks are stopped (== message about pixclocks was
> right...) Bad.
> 
> Does driver work with your change without problems? 

Yes, it's fine. The hardware just seems to take a few seconds to
initialise after printing the console/matroxfb-related messages.

> It looks strange to me that PLL did not stabilized in 5 seconds. Do
> you get same message when you change videomode with fbset, or happens
> this only once during boot, and never again?

Answering that question will require compiling/obtaining fbset for SH3.
Tomorrow.

-- 
dwmw2

Re: matroxfb console oops in 2.4.2x

[Petr Vandrovec]

On 19 Jun 03 at 14:47, David Woodhouse wrote:
> On Thu, 2003-06-19 at 13:21, Petr Vandrovec wrote:

> take_over_console() attempts to redraw the screen.

It is not take_over_console... It does init first. 
> >  It is not allowed to call fbdev's putc 
> > before mode set was issued (at least I always believed to it; before
> > first mode set hardware is in VGA state)
> 
> If I omit the fixes, I just get...
> 
> matroxfb: Matrox Mystique (PCI) detected
> matroxfb: 1280x1024x8bpp (virtual: 1280x1635)
> matroxfb: framebuffer at 0x1000000, mapped to 0xc017d000, size 2097152
> matroxfb: Pixel PLL not locked after 5 secs
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
This one is culprit. If you'll comment this message out, it will not
crash.
 
> Console: switching to colour frame buffer device 160x64
> fb0: MATROX VGA frame buffer device
> 
> If I call matrox_init_putc() earlier as you suggest, then it seems to
> end up busy-waiting in mga_fifo()...

Ok. It means that hardware is completely uninitialized when this happens.
Probably accelerator clocks are stopped (== message about pixclocks was
right...) Bad.

Does driver work with your change without problems? It looks strange
to me that PLL did not stabilized in 5 seconds. Do you get same message
when you change videomode with fbset, or happens this only once during
boot, and never again?
                                        Thanks,
                                                Petr Vandrovec

Re: matroxfb console oops in 2.4.2x

[David Woodhouse]

On Thu, 2003-06-19 at 13:21, Petr Vandrovec wrote:
> On 19 Jun 03 at 11:06, David Woodhouse wrote:
> 
> > Below is a workaround which lets the machine boot. It's obviously not a
> > fix.

> It looks like that someone tried to print something on the console
> during fbcon initialization.

take_over_console() attempts to redraw the screen.

>  It is not allowed to call fbdev's putc 
> before mode set was issued (at least I always believed to it; before
> first mode set hardware is in VGA state)

In this case, the hardware is uninitialised -- it's not in a PeeCee.
Is that relevant?

> Do not you see some error message in dmesg with these fixes?

If I omit the fixes, I just get...

matroxfb: Matrox Mystique (PCI) detected
matroxfb: 1280x1024x8bpp (virtual: 1280x1635)
matroxfb: framebuffer at 0x1000000, mapped to 0xc017d000, size 2097152
matroxfb: Pixel PLL not locked after 5 secs
+$S07#ba

Include the hack I showed you in matroxfb_cfb*_putcs, and I get what I
expect instead of the crash...

Console: switching to colour frame buffer device 160x64
fb0: MATROX VGA frame buffer device

If I call matrox_init_putc() earlier as you suggest, then it seems to
end up busy-waiting in mga_fifo()...

0x8c0dba18 <matrox_cfbX_putcs+184>:     mov.l   @(r0,r3),r1
0x8c0dba1a <matrox_cfbX_putcs+186>:     extu.b  r1,r1
0x8c0dba1c <matrox_cfbX_putcs+188>:     cmp/hi  r2,r1
0x8c0dba1e <matrox_cfbX_putcs+190>:     bf.s    0x8c0dba18 <matrox_cfbX_putcs+184>

(gdb) regs
PC=8c0dba1c SR=40000100 PR=8c0dbb72 MACH=00000003 MACHL=00000780
GBR=f89858fb VBR=8c006000 SSR=00000000 SPC=00000000
R0-R7  b3020000 00000005 00000005 00001e10 07070707 8c164d0c 00000000 01800010
R8-R15 b3020000 00000010 002f0028 00000010 00000025 8c1d6948 8c2a3b14 8c2a3b14
(gdb)  list *$pc
0x8c0dba1c is in matrox_cfbX_putcs (matroxfb_accel.c:610).
605             fxbndry = ((xx + fontwidth(p) - 1) << 16) | xx;
606             mmio = ACCESS_FBINFO(mmio.vbase);
607             while (count--) {
608                     u_int8_t* chardata = p->fontdata + (scr_readw(s++) & p->charmask)*charcell;
609
610                     mga_fifo(6);
611                     mga_writel(mmio, M_FXBNDRY, fxbndry);
612                     mga_writel(mmio, M_AR0, ar0);
613                     mga_writel(mmio, M_AR3, 0);
614                     if (easy) {



-- 
dwmw2

Re: matroxfb console oops in 2.4.2x

[Petr Vandrovec]

On 19 Jun 03 at 11:06, David Woodhouse wrote:

> Below is a workaround which lets the machine boot. It's obviously not a
> fix.
> 
> --- drivers/video/matrox/matroxfb_accel.c~  Wed Jun 18 17:16:40 2003
> +++ drivers/video/matrox/matroxfb_accel.c   Thu Jun 19 10:57:54 2003
> @@ -487,6 +487,9 @@
>  
>     DBG_HEAVY("matroxfb_cfb8_putc");
>  
> +   if (!ACCESS_FBINFO(curr.putc))
> +       return;
> +

It looks like that someone tried to print something on the console
during fbcon initialization. It is not allowed to call fbdev's putc 
before mode set was issued (at least I always believed to it; before
first mode set hardware is in VGA state). Do not you see some error 
message in dmesg with these fixes?

> @@ -504,6 +507,9 @@
>  
>     DBG_HEAVY("matroxfb_cfb16_putc");
>  
> +   if (!ACCESS_FBINFO(curr.putc))
> +       return;
> +

Instead of plugging these tests into fast path please call
"matrox_init_putc(PMINFO NULL, NULL)" somewhere in the initMatrox2(),
before call to the register_framebuffer. At worst some part of video
memory gets smashed by painted characters, but no damage should
occur.
                                                Petr Vandrovec