2004-04-15 20:25:22

by Felix von Leitner

[permalink] [raw]
Subject: radeonfb broken

I am using the radeonfb on a Radeon 9600 Pro with a TFT display, and
radeonfb very recently started to give me a picture at all, so I am
quite happy about that.

However, running mplayer does not work at all on radeonfb. mplayer
inquires about the color depth, I am using 32 bit color depth for this,
but radeonfb says it's DirectColor instead of TrueColor, so mplayer
tries to initialize the palette and fails.

Also, using fbset to set the mode to 1600x1200 fails. The mode is
changed, but the text console resolution stays the same. Worse, a
"setfont" changes back to 1024x768.

Also, I cannot view images on console with fbi or fbv.

Felix


2004-04-15 21:14:26

by Timothy Miller

[permalink] [raw]
Subject: Re: radeonfb broken



Felix von Leitner wrote:
> I am using the radeonfb on a Radeon 9600 Pro with a TFT display, and
> radeonfb very recently started to give me a picture at all, so I am
> quite happy about that.
>
> However, running mplayer does not work at all on radeonfb. mplayer
> inquires about the color depth, I am using 32 bit color depth for this,
> but radeonfb says it's DirectColor instead of TrueColor, so mplayer
> tries to initialize the palette and fails.
>
> Also, using fbset to set the mode to 1600x1200 fails. The mode is
> changed, but the text console resolution stays the same. Worse, a
> "setfont" changes back to 1024x768.
>
> Also, I cannot view images on console with fbi or fbv.
>
> Felix


What annoys me most about the Radeon driver is the off-by-one error in
the bmove routine. Whenever text is copied to the right or down, it
gets positioned incorrectly. I posted the fix, but no one paid attention.

2004-04-16 01:52:13

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: radeonfb broken


>
> What annoys me most about the Radeon driver is the off-by-one error in
> the bmove routine. Whenever text is copied to the right or down, it
> gets positioned incorrectly. I posted the fix, but no one paid attention.

Mayb it was just "missed" in the flow of hundreds of mails that go
through this list. Can you re-sent it to me, and also precise which
kernel version it applies to ?

Ben.


2004-04-16 01:55:52

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: radeonfb broken


> However, running mplayer does not work at all on radeonfb. mplayer
> inquires about the color depth, I am using 32 bit color depth for this,
> but radeonfb says it's DirectColor instead of TrueColor, so mplayer
> tries to initialize the palette and fails.

It is DirectColor and setting the palette should work. If it doesn't
then we indeed have a bug, but so far, I didn't have any problems
with MacOnLinux which does fbdev and sets the palette properly not
with XFree which does also set the palette. In DirectColor, the
palette is actually a gamma table.

> Also, using fbset to set the mode to 1600x1200 fails. The mode is
> changed, but the text console resolution stays the same. Worse, a
> "setfont" changes back to 1024x768.

Yes, that's not a radeonfb problem. The current 2.6 fbcon driver is
completely broken in this regard imho. It tries to adapt the display
mode on stty and setfont, which usually doesn't work properly since it
can't calculate supported modes properly (well... radeonfb sort of can
but it's the only one who can and there are still issues). On the
other hand, it doesn't adapt properly to changes done via fbset.

I have done some workarounds for this but they triggered other bugs
in the fbcon driver at that time, some of them causing memory
corruption, so I didn't commit my changes upstream.

A bunch of this should have been fixed by now, I need to re-sync with
James Simmons and see if I can make that to work now.

> Also, I cannot view images on console with fbi or fbv.
>
> Felix
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Benjamin Herrenschmidt <[email protected]>

2004-04-16 15:55:08

by Timothy Miller

[permalink] [raw]
Subject: Re: radeonfb broken


Andrew Morton asked me to repost the patch for the Radeon FB off-by-one
bug. I'll see about making a proper patch when I get home, but if you
want to fix it quicker, I'll just tell you what to change.


In the druvers/video/radeonfb.c, there is a function called
fbcon_radeon_bmove. In there, you'll see this code:

if (srcy < dsty) {
srcy += height;
dsty += height;
} else
dp_cntl |= DST_Y_TOP_TO_BOTTOM;

if (srcx < dstx) {
srcx += width;
dstx += width;
} else
dp_cntl |= DST_X_LEFT_TO_RIGHT;


Those adds need to be reduced by one. The code should look like this:


if (srcy < dsty) {
srcy += height - 1;
dsty += height - 1;
} else
dp_cntl |= DST_Y_TOP_TO_BOTTOM;

if (srcx < dstx) {
srcx += width - 1;
dstx += width - 1;
} else
dp_cntl |= DST_X_LEFT_TO_RIGHT;



This bug is in the mainline kernel, and since I have direct experience
programming for the Radeon, I knew how to fix it, but I also noticed
that the Red Hat kernel "2.4.18-27.7.x" has the proper fix in it.


Whenever I download a new 2.4 kernel for gentoo, I have to manually make
that fix. I'm also disappointed that acceleration for Radeon has
disappeared completely from 2.6.

2004-04-16 15:59:52

by Timothy Miller

[permalink] [raw]
Subject: Re: radeonfb broken



Benjamin Herrenschmidt wrote:
>>What annoys me most about the Radeon driver is the off-by-one error in
>>the bmove routine. Whenever text is copied to the right or down, it
>>gets positioned incorrectly. I posted the fix, but no one paid attention.
>
>
> Mayb it was just "missed" in the flow of hundreds of mails that go
> through this list. Can you re-sent it to me, and also precise which
> kernel version it applies to ?
>

Oh, I failed to mention this bit.

I've seen it in 2.4.22-gentoo-r7 and 2.4.25-gentoo.

The bug is NOT present in Red Hat's 2.4.18-27.7.x

2004-04-16 16:05:31

by Timothy Miller

[permalink] [raw]
Subject: Re: radeonfb broken



Benjamin Herrenschmidt wrote:
>>What annoys me most about the Radeon driver is the off-by-one error in
>>the bmove routine. Whenever text is copied to the right or down, it
>>gets positioned incorrectly. I posted the fix, but no one paid attention.
>
>
> Mayb it was just "missed" in the flow of hundreds of mails that go
> through this list. Can you re-sent it to me, and also precise which
> kernel version it applies to ?
>
> Ben.


BTW, now that we're on the topic of Radeon, could someone tell me how to
tell the kernel the default resolution to use when initializing the
console?

When using a CRT, it defaults to 640x480. When I use my Planar PQ191
19" 1280x1024 monitor, it defaults to 1024x768. I want it to default to
1280x1024. There's a tool, fbset or something like that, which I can
use AFTER bootup, but trying to put that into init causes all sorts of
conflicts. I need to be able to tell the kernel, either at compile time
or on the boot command line.


Moving off topic, my cries for help from the XFree86 people also seem to
have gotten lost in the flow of hundreds of usenet messages. Try as I
might, I cannot seem to get XFree86 to talk to my monitor at anything
other than 60Hz. Even though LCD monitors have a persistent image,
increasing the frame rate CAN reduce motion blur slightly.


Thanks.

2004-04-16 16:46:53

by Randy.Dunlap

[permalink] [raw]
Subject: Re: radeonfb broken

On Fri, 16 Apr 2004 12:06:22 -0400 Timothy Miller wrote:

|
| BTW, now that we're on the topic of Radeon, could someone tell me how to
| tell the kernel the default resolution to use when initializing the
| console?
|
| When using a CRT, it defaults to 640x480. When I use my Planar PQ191
| 19" 1280x1024 monitor, it defaults to 1024x768. I want it to default to
| 1280x1024. There's a tool, fbset or something like that, which I can
| use AFTER bootup, but trying to put that into init causes all sorts of
| conflicts. I need to be able to tell the kernel, either at compile time
| or on the boot command line.

See <Documentation/fb/modedb.txt> for a little help: <quote>

Valid mode specifiers (mode_option argument):

<xres>x<yres>[-<bpp>][@<refresh>]
<name>[-<bpp>][@<refresh>]

with <xres>, <yres>, <bpp> and <refresh> decimal numbers and <name> a string.
Things between square brackets are optional.
</quote>


I haven't done this, but I've seen emails using syntax like so:

(from /etc/lilo.conf:)

append="video=radeonfb:1024x768-32@100"

or:
append="video=radeonfb radeonfb.mode_option=1280x1024-32@60"

or:
append="video=radeonfb:1024x768-32@100"


Looking at the driver source code, I'd guess that the middle
one above is closest to correct.

ISTM that Documentation/kernel-parameters.txt needs a paragraph
about syntax for specifying parameters for built-in modules vs.
loadable modules....


| Moving off topic, my cries for help from the XFree86 people also seem to
| have gotten lost in the flow of hundreds of usenet messages. Try as I
| might, I cannot seem to get XFree86 to talk to my monitor at anything
| other than 60Hz. Even though LCD monitors have a persistent image,
| increasing the frame rate CAN reduce motion blur slightly.


--
~Randy
"We have met the enemy and he is us." -- Pogo (by Walt Kelly)
(Again. Sometimes I think ln -s /usr/src/linux/.config .signature)

2004-04-16 20:35:17

by James Simmons

[permalink] [raw]
Subject: Re: radeonfb broken


Thanks for the fix.


On Fri, 16 Apr 2004, Timothy Miller wrote:

>
> Andrew Morton asked me to repost the patch for the Radeon FB off-by-one
> bug. I'll see about making a proper patch when I get home, but if you
> want to fix it quicker, I'll just tell you what to change.
>
>
> In the druvers/video/radeonfb.c, there is a function called
> fbcon_radeon_bmove. In there, you'll see this code:
>
> if (srcy < dsty) {
> srcy += height;
> dsty += height;
> } else
> dp_cntl |= DST_Y_TOP_TO_BOTTOM;
>
> if (srcx < dstx) {
> srcx += width;
> dstx += width;
> } else
> dp_cntl |= DST_X_LEFT_TO_RIGHT;
>
>
> Those adds need to be reduced by one. The code should look like this:
>
>
> if (srcy < dsty) {
> srcy += height - 1;
> dsty += height - 1;
> } else
> dp_cntl |= DST_Y_TOP_TO_BOTTOM;
>
> if (srcx < dstx) {
> srcx += width - 1;
> dstx += width - 1;
> } else
> dp_cntl |= DST_X_LEFT_TO_RIGHT;
>
>
>
> This bug is in the mainline kernel, and since I have direct experience
> programming for the Radeon, I knew how to fix it, but I also noticed
> that the Red Hat kernel "2.4.18-27.7.x" has the proper fix in it.
>
>
> Whenever I download a new 2.4 kernel for gentoo, I have to manually make
> that fix. I'm also disappointed that acceleration for Radeon has
> disappeared completely from 2.6.
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2004-04-16 21:04:15

by Timothy Miller

[permalink] [raw]
Subject: Re: radeonfb broken



James Simmons wrote:
> Thanks for the fix.
>

My pleasure!

2004-04-17 00:32:22

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: radeonfb broken

On Sat, 2004-04-17 at 01:58, Timothy Miller wrote:
> Benjamin Herrenschmidt wrote:
> >>What annoys me most about the Radeon driver is the off-by-one error in
> >>the bmove routine. Whenever text is copied to the right or down, it
> >>gets positioned incorrectly. I posted the fix, but no one paid attention.
> >
> >
> > Mayb it was just "missed" in the flow of hundreds of mails that go
> > through this list. Can you re-sent it to me, and also precise which
> > kernel version it applies to ?
> >
>
> Oh, I failed to mention this bit.
>
> I've seen it in 2.4.22-gentoo-r7 and 2.4.25-gentoo.
>
> The bug is NOT present in Red Hat's 2.4.18-27.7.x

Can you send me the fix you posted back then ?

Ben.


2004-04-19 02:18:39

by David Eger

[permalink] [raw]
Subject: Re: radeonfb broken


My apologies; the bug is mine. It's a simple issue of taking care
of overlapping regions in calls to copyarea(). I sent the fix to the
linux-fbdev mailing list earlier today. The patch is reprinted below;
Hopefully Linus will take it for 2.6.6.

-dte

--- drivers/video/aty/radeon_accel.c.orig 2004-04-19 01:26:52.000000000 +0200
+++ drivers/video/aty/radeon_accel.c 2004-04-19 01:49:14.000000000 +0200
@@ -53,6 +53,18 @@
static void radeonfb_prim_copyarea(struct radeonfb_info *rinfo,
const struct fb_copyarea *area)
{
+ int xdir, ydir;
+ u32 sx, sy, dx, dy, w, h;
+
+ w = area->width; h = area->height;
+ dx = area->dx; dy = area->dy;
+ sx = area->sx; sy = area->sy;
+ xdir = sx - dx;
+ ydir = sy - dy;
+
+ if ( xdir < 0 ) { sx += w-1; dx += w-1; }
+ if ( ydir < 0 ) { sy += h-1; dy += h-1; }
+
radeon_fifo_wait(3);
OUTREG(DP_GUI_MASTER_CNTL,
rinfo->dp_gui_master_cntl /* i.e. GMC_DST_32BPP */
@@ -60,12 +72,13 @@
| ROP3_S
| DP_SRC_RECT );
OUTREG(DP_WRITE_MSK, 0xffffffff);
- OUTREG(DP_CNTL, (DST_X_LEFT_TO_RIGHT | DST_Y_TOP_TO_BOTTOM));
+ OUTREG(DP_CNTL, (xdir>=0 ? DST_X_LEFT_TO_RIGHT : 0)
+ | (ydir>=0 ? DST_Y_TOP_TO_BOTTOM : 0));

radeon_fifo_wait(3);
- OUTREG(SRC_Y_X, (area->sy << 16) | area->sx);
- OUTREG(DST_Y_X, (area->dy << 16) | area->dx);
- OUTREG(DST_HEIGHT_WIDTH, (area->height << 16) | area->width);
+ OUTREG(SRC_Y_X, (sy << 16) | sx);
+ OUTREG(DST_Y_X, (dy << 16) | dx);
+ OUTREG(DST_HEIGHT_WIDTH, (h << 16) | w);
}


2004-04-19 14:18:29

by Timothy Miller

[permalink] [raw]
Subject: Re: radeonfb broken



Benjamin Herrenschmidt wrote:
> On Sat, 2004-04-17 at 01:58, Timothy Miller wrote:
>
>>Benjamin Herrenschmidt wrote:
>>
>>>>What annoys me most about the Radeon driver is the off-by-one error in
>>>>the bmove routine. Whenever text is copied to the right or down, it
>>>>gets positioned incorrectly. I posted the fix, but no one paid attention.
>>>
>>>
>>>Mayb it was just "missed" in the flow of hundreds of mails that go
>>>through this list. Can you re-sent it to me, and also precise which
>>>kernel version it applies to ?
>>>
>>
>>Oh, I failed to mention this bit.
>>
>>I've seen it in 2.4.22-gentoo-r7 and 2.4.25-gentoo.
>>
>>The bug is NOT present in Red Hat's 2.4.18-27.7.x
>
>
> Can you send me the fix you posted back then ?


Sorry. I forgot all about this. If the description I sent isn't good
enough, I'll have to make a proper patch. Is that what you want? I
doubt I gave my original email, but its essential content is in my more
recent posting anyhow.

2004-04-19 22:41:15

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: radeonfb broken


> Sorry. I forgot all about this. If the description I sent isn't good
> enough, I'll have to make a proper patch. Is that what you want? I
> doubt I gave my original email, but its essential content is in my more
> recent posting anyhow.

Yes, please.
Ben.


2004-04-20 23:21:15

by David Eger

[permalink] [raw]
Subject: Re: radeonfb broken


On Mon, Apr 19, 2004 at 10:31:27AM -0400, Timothy Miller wrote:
>Ah, so you are the one who wrote the Radeon driver? Thanks for the fix.

Originally it was Ani Joshi's driver. Now BenH maintains it.
I just wrote the acceleration code for radeon for 2.6 ;-)

>One question I have is how to turn on acceleration in the 2.6 kernel.

Funny you should ask. We're discussing over on the fbdev list
( http://linux-fbdev.sourceforge.net/ ) why it's so slow and how to fix it.
It turns out that the fbcon layer has some heuristics to decide how
best to accelerate text rendering (i.e., redraw, call driver accel routines,
or do a screen-to-screen bitblt). These heuristics are just plain *bad*.
At the moment, I just comment out the logic and hard-wire the choice
to "use the driver accel routines, dumbass" as in the excerpt below.

We ought to fix this logic so the video cards indicate which method
is best. The current fields in fb_fix_screeninfo don't really
convey the message well, as you can see in the logic below..


Excerpt from linux/drivers/video/console/fbcon.c:

#define divides(a, b) ((!(a) || (b)%(a)) ? 0 : 1)

/* ... */

static __inline__ void updatescrollmode(struct display *p, struct
vc_data *vc)
{
struct fb_info *info = registered_fb[(int) con2fb_map[vc->vc_num]];

int m;
/*if (p->scrollmode & __SCROLL_YFIXED)
return;
if (divides(info->fix.ywrapstep, vc->vc_font.height) &&
divides(vc->vc_font.height, info->var.yres_virtual))
m = __SCROLL_YWRAP;
else if (divides(info->fix.ypanstep, vc->vc_font.height) &&
info->var.yres_virtual >= info->var.yres +
vc->vc_font.height)
m = __SCROLL_YPAN;
else if (p->scrollmode & __SCROLL_YNOMOVE)
m = __SCROLL_YREDRAW;
else*/
m = __SCROLL_YMOVE;
p->scrollmode = (p->scrollmode & ~__SCROLL_YMASK) | m;
}

- end Excerpt

So the new driver will work once we patch it to tell fbcon not to be
stupid ;-)

sorry, i'm not sure about setting initial resolution with kernel arguments..

-dte



----- End forwarded message -----