Console rotation problems

Console rotation problems

[Benjamin Herrenschmidt]

Hi Antonio !

I decided to give a quick test to console rotation on my g5 (radeonfb)
and couldn't get it to work.

When I tried echo'ing something in con_rotate, something very strange
happened:

pogo:/sys/class/graphics/fb0# echo "1" >con_rotate
benh@pogo:~$

As you can see, it looks like I pressed ctrl-D (which I didn't do) and
thus exited my shell session. Thus my shell session got killed some way.
Verified it twice. Also the console stopped displaying anything (that
is,whatever was displayed was still there but the cursor didn't blink
anymore and no new stuff got displayed). 

When I then tried to echo "0" back there, it just hung in the kernel
(probably on a semaphore, I could still use the box for a little while
before it completely locked up).

Ben.

Re: Console rotation problems

[Benjamin Herrenschmidt]

On Thu, 2005-11-24 at 11:45 +1100, Benjamin Herrenschmidt wrote:
> Hi Antonio !
> 
> I decided to give a quick test to console rotation on my g5 (radeonfb)
> and couldn't get it to work.
> 
> When I tried echo'ing something in con_rotate, something very strange
> happened:
> 
> pogo:/sys/class/graphics/fb0# echo "1" >con_rotate
> benh@pogo:~$

  .../...

And here is the Oops that explains the shell exit and that I didn't see
the first time :)

Trap 0x600 is an alignment exception, which is a bit weird, I'll try to
dig a bit more, it could be a problem with the alignment trap handler on
ppc.

Ben.

REGS: c00000000f7c3150 TRAP: 0600   Not tainted  (2.6.15-rc2)
MSR: 9000000000009032 <EE,ME,IR,DR>  CR: 44242488  XER: 00000000
DAR: C00000007E56B002, DSISR: 0000000002200000
TASK = c00000000f5a4040[2505] 'bash' THREAD: c00000000f7c0000 CPU: 1
GPR00: 0000000000000010 C00000000F7C33D0 C0000000006952D8 0000000000000058
GPR04: 0000000000000004 0000000000000010 0000000000000001 0000000000000000
GPR08: 0000000000000004 C00000007E56B002 C00000007E56B002 000000000000003F
GPR12: 0000000000000001 C00000000057D800 00000000100D0000 00000000100D2B38
GPR16: 00000000100D0000 00000000100A0000 0000000000000000 0000000000000000
GPR20: 0000000000000010 0000000000000010 C00000000074E600 0000000000000000
GPR24: 0000000000000010 00000000000000FF 0000000000000008 000000000000000B
GPR28: 0000000000000008 C00000000046B7F0 C0000000005F7D30 C00000007E56B000
NIP [C00000000025BAB8] .fbcon_rotate_font+0x500/0x54c
LR [C00000000025B7C4] .fbcon_rotate_font+0x20c/0x54c
Call Trace:
[C00000000F7C33D0] [C00000000025B7A8] .fbcon_rotate_font+0x1f0/0x54c (unreliable)[C00000000F7C34B0] [C000000000259654] .fbcon_switch+0x6a0/0x780
[C00000000F7C3630] [C00000000029ACF8] .redraw_screen+0x1c8/0x264
[C00000000F7C36C0] [C00000000029B5E0] .vc_resize+0x46c/0x474
[C00000000F7C37B0] [C000000000255E94] .fbcon_modechanged+0x16c/0x484
[C00000000F7C3890] [C000000000259930] .fbcon_event_notify+0x1fc/0x564
[C00000000F7C39F0] [C0000000000574FC] .notifier_call_chain+0x64/0x98
[C00000000F7C3A80] [C0000000002623F0] .fb_con_duit+0x2c/0x44
[C00000000F7C3B10] [C0000000002659EC] .store_con_rotate+0x5c/0x88
[C00000000F7C3BC0] [C0000000002A8004] .class_device_attr_store+0x60/0x7c
[C00000000F7C3C40] [C0000000000F01B0] .sysfs_write_file+0xf8/0x1a0
[C00000000F7C3CF0] [C00000000009EEFC] .vfs_write+0x1c0/0x1fc
[C00000000F7C3D90] [C00000000009F014] .sys_write+0x4c/0x90
[C00000000F7C3E30] [C000000000008600] syscall_exit+0x0/0x18
Instruction dump:
7d093670 7d290194 7d5f5214 7d6b0436 7d2707b4 55293032 796007e1 7d094050
78e71f24 4182001c 7d804036 7d275214 <7d6048a8> 7d6b0378 7d6049ad 40c2fff4

Re: Console rotation problems

[Benjamin Herrenschmidt]

On Thu, 2005-11-24 at 11:52 +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2005-11-24 at 11:45 +1100, Benjamin Herrenschmidt wrote:
> > Hi Antonio !
> > 
> > I decided to give a quick test to console rotation on my g5 (radeonfb)
> > and couldn't get it to work.
> > 
> > When I tried echo'ing something in con_rotate, something very strange
> > happened:
> > 
> > pogo:/sys/class/graphics/fb0# echo "1" >con_rotate
> > benh@pogo:~$
> 
>   .../...
> 
> And here is the Oops that explains the shell exit and that I didn't see
> the first time :)
> 
> Trap 0x600 is an alignment exception, which is a bit weird, I'll try to
> dig a bit more, it could be a problem with the alignment trap handler on
> ppc.

Ok, looks like an unaligned set_bit() or atomic_or(). You can't do
atomic operations on non-aligned quantities. Let me check the code ...

OUCH !

Is there any reason why you are using set_bit & friends all over the
place over there in fbcon_rotate.h ? Are you actually trying to perform
atomic operations ?

set_bit(), clear_bit() etc... are

1) atomic. that mean slow, won't work on uncached memory (like
framebuffer), and won't work if not aligned propertly

2) won't guarantee the bit position, they are consistent within an
architecture but not accross.

If you want bit manipulation of pixel data, use your own routines, using
set_bit/clear_bit & friends is completely bogus.

The __xxx versions will fix 1), but not 2) btw.

Ben.

Re: Console rotation problems

[Benjamin Herrenschmidt]

Remove bogus usage of test/set_bit() from fbcon rotation code and just
manipulate the bits directly. This fixes an oops on powerpc among others
and should be faster. Seems to work fine on the G5 here.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

And here is the fix. Tony, did I miss something ?

Index: linux-serialfix/drivers/video/console/fbcon_rotate.h
===================================================================
--- linux-serialfix.orig/drivers/video/console/fbcon_rotate.h	2005-11-23 11:07:23.000000000 +1100
+++ linux-serialfix/drivers/video/console/fbcon_rotate.h	2005-11-24 14:02:22.000000000 +1100
@@ -21,21 +21,13 @@
         (s == SCROLL_REDRAW || s == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
         (i)->var.xres : (i)->var.xres_virtual; })
 
-/*
- * The bitmap is always big endian
- */
-#if defined(__LITTLE_ENDIAN)
-#define FBCON_BIT(b) (7 - (b))
-#else
-#define FBCON_BIT(b) (b)
-#endif
 
 static inline int pattern_test_bit(u32 x, u32 y, u32 pitch, const char *pat)
 {
 	u32 tmp = (y * pitch) + x, index = tmp / 8,  bit = tmp % 8;
 
 	pat +=index;
-	return (test_bit(FBCON_BIT(bit), (void *)pat));
+	return (*pat) & (0x80 >> bit);
 }
 
 static inline void pattern_set_bit(u32 x, u32 y, u32 pitch, char *pat)
@@ -43,7 +35,8 @@ static inline void pattern_set_bit(u32 x
 	u32 tmp = (y * pitch) + x, index = tmp / 8, bit = tmp % 8;
 
 	pat += index;
-	set_bit(FBCON_BIT(bit), (void *)pat);
+
+	(*pat) |= 0x80 >> bit;
 }
 
 static inline void rotate_ud(const char *in, char *out, u32 width, u32 height)
Index: linux-serialfix/drivers/video/console/fbcon_ccw.c
===================================================================
--- linux-serialfix.orig/drivers/video/console/fbcon_ccw.c	2005-11-15 11:54:14.000000000 +1100
+++ linux-serialfix/drivers/video/console/fbcon_ccw.c	2005-11-24 14:03:48.000000000 +1100
@@ -34,7 +34,7 @@ static inline void ccw_update_attr(u8 *d
 		msk <<= (8 - mod);
 
 	if (offset > mod)
-		set_bit(FBCON_BIT(7), (void *)&msk1);
+		msk1 |= 0x01;
 
 	for (i = 0; i < vc->vc_font.width; i++) {
 		for (j = 0; j < width; j++) {

Re: Console rotation problems

[Antonino A. Daplas]

Benjamin Herrenschmidt wrote:
> Remove bogus usage of test/set_bit() from fbcon rotation code and just
> manipulate the bits directly. This fixes an oops on powerpc among others
> and should be faster. Seems to work fine on the G5 here.

Thanks, I reached a point when my head became muddled with bit 
manipulations, so I used arch-specific bitops but complete forgot
that they were atomic :-)

> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Antonino Daplas <adaplas@pol.net>

> ---
> 
> And here is the fix. Tony, did I miss something ?

Works in little-endian too, so thank you very much. cfbimageblit may
also suffer from this same mistake (mine).  So can you test with
12x22 fonts at rotate 1 or 3 with acceleration off?

This particular line in cfbimgblit.c:slow_imageblit() is definitely
questionable.

- color = (*s & 1 << (BIT_NR(l))) ? fgcolor : bgcolor;
+ color = (*s & (1 << l)) ? fgcolor : bgcolor;


Tony

Re: [Linux-fbdev-devel] Re: Console rotation problems

[Geert Uytterhoeven]

On Thu, 24 Nov 2005, Antonino A. Daplas wrote:
> Benjamin Herrenschmidt wrote:
> > Remove bogus usage of test/set_bit() from fbcon rotation code and just
> > manipulate the bits directly. This fixes an oops on powerpc among others
> > and should be faster. Seems to work fine on the G5 here.
> 
> Thanks, I reached a point when my head became muddled with bit 
> manipulations, so I used arch-specific bitops but complete forgot
> that they were atomic :-)

I haven't really looked at the rotation code, but I guess it can be optimized a
lot by using similar techniques like c2p (chunky-to-planar) conversions.

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

Re: [Linux-fbdev-devel] Re: Console rotation problems

[Antonino A. Daplas]

Geert Uytterhoeven wrote:
> On Thu, 24 Nov 2005, Antonino A. Daplas wrote:
>> Benjamin Herrenschmidt wrote:
>>> Remove bogus usage of test/set_bit() from fbcon rotation code and just
>>> manipulate the bits directly. This fixes an oops on powerpc among others
>>> and should be faster. Seems to work fine on the G5 here.
>> Thanks, I reached a point when my head became muddled with bit 
>> manipulations, so I used arch-specific bitops but complete forgot
>> that they were atomic :-)
> 
> I haven't really looked at the rotation code, but I guess it can be optimized a
> lot by using similar techniques like c2p (chunky-to-planar) conversions.
> 

Actually, BenH's patch touched code that is in the slow path.  The font
rotation is done on the entire fontdata, then stored, on a console switch.
Thus the rest of the drawing functions need not do repetitive and expensive
bitmap rotation.

Scrolling performance of a normally oriented and upside-down rotation is
practically the same (1024x768 8x16 redraw).

con_rotate = 0
real    0m2.550s
user    0m0.000s
sys     0m2.543s

con_rotate = 2
real    0m2.559s
user    0m0.001s
sys     0m2.545s


CCW and CW rotation, is of course slower, because the blit direction
tends to be top->down, rather than left->right.

con_rotate = 1
real    0m4.675s
user    0m0.000s
sys     0m4.663s

con_rotate = 3
real    0m4.701s
user    0m0.001s
sys     0m4.684s
  
Tony