Bug 1293

Created attachment 830 [details] [review]
mga-render-accel-6-7.patch

trivial backout of the second fix, enables Render for single-head setups.

won't do anything on 6.8, since the driver still uses the old Render setup
hooks, but should enable Render again for 6.7.

Created attachment 831 [details] [review]
mga-render-accel-6-8.patch

this enables Render on single-head mga, and also converts the driver to use the
new setup hooks.  since the driver only supports the one picture format, it
never used the alphaType or texType parameters, so this is really just a matter
of fixing the function signature.  nonetheless, untested for lack of hardware.

only apply this one against 6.8, obviously.

You shouldn't be hitting the hook at all with your 6.8 patch either, since you
didn't set a list of supported formats.  (See radeon_accelfuncs.c)

I tested the patch on my machine (i686-pc-linux-gnu, MGAg550) with the X.org 6.8
server, and as Eric Anholt predicted, I noticed no difference in performace. Is
there a way to check if in fact RENDER is being accelerated?

I also noticed that the mga(4) driver doesn't support ARGB cursors directly...
Is the Xcursors rendering done using RENDER? If it is, then it's probably still
unaccelerated, since the cursor still flickers a lot and generates artifacts
with OpenGL applications.

I'd be willing to test further patchs if needed. :-]

Created attachment 896 [details] [review]
mga-render-accel-6-8-v2.patch

i guess it does make sense that we need to specify both the src and dst formats
we support.  new patch should get it right.

tiago, care to test?

(In reply to comment #5)

It seems just as slow with the new patch... Non-transparent window movements are
much slower with drop-down shadows, and transparent window movements are very
very slow, taking all CPU. Server-side compositing is just as slow as
client-side compositing (should it be?). The XCursors mouse pointer still
flickers a lot too. 

Just what kind of speed improvement should I expect? Is there a way to check if
the render accel hooks are really being called? I'm going to add some printfs to
the mga hook functions to see if they get called...

it may not be any faster if the formats xcompmgr is using aren't the formats we
advertise support for.  you can stick some debugging printfs in the setup
functions to find out.

could you try running rendercheck (from xapps) as well, to make sure it's
conformant?

Created attachment 901 [details]
Output of rendercheck using mga(4) with the v2 patch applied.

The output of rendercheck in my setup is attached. There are a couple of errors
in the first tests.

I've put printfs in the beginning (before any function calls) of
MGASetupForCPUToScreenAlphaTexture, MGASetupForCPUToScreenTexture and
MGASubsequentCPUToScreenTexture, and none of them was called during rendercheck
(after a recompilation and reload of the driver of course). 

If you could write a patch that would expose all the needed details to debug
this, I could run it and send you back the output...

i believe those errors are expected failures arising from bugs in fb, yes anholt?

the rest looks good.  are you sure the printfs didn't get called?  they'd be in
the X log, not your console.  you might try converting them to ErrorF(), which
works just like printf().

Yes, that rendercheck error is known.  I intend to fix it in fb, and have a
patch for it, once I can find some text somewhere confirming that that's how
coordinates work.  I'm pretty sure about it, though.

ajax: you've set the dest texture formats supported to a8r8g8b8.  That's quite
uncommon (only when you're using the argb visual, mostly).  That's probably why
he isn't getting the printfs hit, because you're not seeing any destinations
that can be accelerated to, so the driver's code is never called.

If you're only going to support operations that don't use destination alpha, you
could throw x8r8g8b8 in the mix and get acceleration for the typical 32bpp
framebuffer case.  The best would be if 16bpp could be supported, too.  The use
of piles of magic numbers in this code means someone might have to go look up
how to do that.

ajax: Yes, after investigating around a bit, I noticed prinfts never print
anything (neither in the console or logfile), so I changed them to xf86DrvMsg.
For instance, in MGASetupForCPUToScreenAlphaTexture I have:

xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "MGASetupForCPUToScreenAlphaTexture
called\n");

To test, I also put something like this at MGANAME(AccelInit). The AccelInit
message appears in the logfile, but nothing for
MGASetupForCPUToScreenAlphaTexture, MGASetupForCPUToScreenTexture and
MGASubsequentCPUToScreenTexture.

I went ahead and did (rather naively) what anholt said, and added PICT_x8r8g8g8
to MGATextureFormats, and now the hooks are getting called:

(WW) MGA(0): MGASetupForCPUToScreenTexture called
(WW) MGA(0): MGASubsequentCPUToScreenTexture called

But these are the only ones that are called (no
MGASetupForCPUToScreenAlphaTexture). I noticed no speedup whatsoever, but that
may be due to the fact that every call to these functions is being logged...

right, the alpha setup would only get called for the a8 format, which is pretty
rare.  i still need to extend rendercheck to cover these cases as well.

with x8r8g8b8 in the format list, do you pass rendercheck?

The Alpha call should be happening for non-subpixel, antialiased Xft text that's
a solid color (i.e. almost all the time).

Created attachment 908 [details]
Rendercheck output with x8r8g8b8 added to format list

There are a couple of extra errors in rendercheck.

I turned off all the logging, and the performance is unchanged, i.e., just as
it was before the RENDER hooks were setup.

Any idea why the alpha call is not happening?

(In reply to comment #12)
> I went ahead and did (rather naively) what anholt said, and added PICT_x8r8g8g8
> to MGATextureFormats, and now the hooks are getting called:

The problem with that is that now you're allowing x8r8g8b8 sources as well,
without adding any code to deal with them.  (an x8 instead of a8 means you need
to wire things such that those bits act like 1s for the purposes of that
operation).  Easiest solution would be to split the formats into DstFormats,
TextureFormats, TextureAlphaFormats, so you'd support a8r8g8b8 and x8r8g8b8 dst,
a8r8g8b8 texfmt, and a8 texalphafmt.

If you're trying to use xcompmgr, I wouldn't expect much performance
improvement.  x11perf aa24text is what I've used in the past to measure render
performance, particularly with XAA.

Ok, so I did:

CARD32 MGAAlphaTextureFormats[2] = {PICT_a8, 0};
CARD32 MGATextureFormats[2] = {PICT_a8r8g8b8, 0};
CARD32 MGATextureDstFormats[3] = {PICT_a8r8g8b8, PICT_x8r8g8b8, 0};

and changed
infoPtr->CPUToScreenTextureDstFormats = MGATextureFormats;
to
infoPtr->CPUToScreenTextureDstFormats = MGATextureDstFormats;

And now rendercheck accuses no error (except that known one at the beggining),
but MGASetupForCPUToScreenAlphaTexture still doesn't get called. Should
MGAAlphaTextureFormats be split too?

Why shouldn't I expect performace improvements with xcompmgr? Wouldn't all the
alpha blending be hardware accelerated if this is setup right?

I've also split

CARD32 MGAAlphaTextureFormats[2] = {PICT_a8, 0};

to

CARD32 MGAAlphaTextureFormats[2] = {PICT_a8, 0};
CARD32 MGAAlphaTextureDstFormats[4] = {PICT_a8, PICT_a8r8g8b8, PICT_x8r8g8b8, 0};

and now MGASetupForCPUToScreenAlphaTexture gets called, and rendercheck doesn't
complain.

what is the current status on this?
does what tiago worked on need to be merged to the patch, or did i misread this
entirely?

Created attachment 1577 [details] [review]
mga-render.diff

Here's my version, with more ops supported, correct dst format support, and
split out into a separate file for sanity.  Passes rendercheck on my G400.

We could support more dstformats if we set MACCESS, it looks like, but then
we'd have to do that work for the tiny bit of gain which is XAA Render accel.

Another report has mga-render.diff renderchecking successfully on a G550.  I'd
appreciate getting testing from a G200 user, but it's probably safe enough.  I'm
going on vacation for a few days, so if someone wanted to commit this in the
meantime, that'd be awesome.

anholt, your patch does not work for me against 6.8.2RC1.  AA text renders as
solid rectangles.  i hit this same behaviour doing tdfx, and istr disabling a8
textures fixed it.  i'll play with it a bit.

anholt, if you get to exa support for mga, go ahead and attach it here. 
otherwise this is sort of a WONTFIX since xaa renderaccel is so bad.

Created attachment 3529 [details] [review]
Updated mga-render.diff to work with current CVS HEAD

Created attachment 3531 [details] [review]
Updated mga-render.diff to work with current CVS HEAD (fixed)

Sorry, attached a bad patch earlier.

i would strongly prefer to see Render acceleration for mga(4) done through EXA
at this point.

Sure, I just wanted to try the XAA patch so I made it work with current CVS HEAD.
I did look at EXA but I just don't know enough about all of this to implement it.

The patch makes rendercheck pass on my G450, but GTK2's text rendering is
slightly flawed with the patch. If that's not old news, I can also provide
screenshots of course.

(In reply to comment #27)
> Sure, I just wanted to try the XAA patch so I made it work with current CVS HEAD.
> I did look at EXA but I just don't know enough about all of this to implement it.

If you're interested in doing exa support, there's some fairly good
documentation at:

http://virtuousgeek.org/exa-driver.txt

and I'd be happy to answer questions.

> The patch makes rendercheck pass on my G450, but GTK2's text rendering is
> slightly flawed with the patch. If that's not old news, I can also provide
> screenshots of course.

Mmm, screenshot would be nice.  It'd also help if you could figure out what the
parameters are to the Render hooks when gtk does text, so we can extend
rendercheck to cover that case.

Created attachment 3557 [details]
Screenshot of a GTK app that shows buggy text rendering

Instead of having a white background, the labels should be transparent.
Forcing a redraw of the window (switching desktops in e17, which I think is the
same as unmapping and mapping the window again) fixes it sometimes.

(In reply to comment #28)

The problem is, I don't know anything about the MGA driver either, and it's not
very well documented IMO.

To give a specific example, I couldn't figure out how to set the memoryBase,
memorySize and offScreenBase values in the ExaDriverPtr struct.

got bored, started hacking an EXA conversion.

Created attachment 4901 [details] [review]
mga-exa-1.patch

quick hack job.  offscreen memory setup still slightly busted (occasional
cursor garbage, etc), no Render yet, but getting there.

Created attachment 5005 [details] [review]
mga-exa-2.patch

second iteration, fill in the Render accel from kdrive and get it to at least
not crash when DRI is turned off.  it's definitely dispatching into the driver
for Render, but given all the other rendering issues it's tough to tell what's
going wrong.

guaranteed broken on g200 and below.

i don't have a machine to test this on currently, is this expected to work well
on a G550?

(In reply to comment #34)
> i don't have a machine to test this on currently, is this expected to work well
> on a G550?

it's known to render incorrectly pretty much across the board.  it should be
considered enough to work from if you want to assist in completing it, but far
from anything any user should attempt.

the "guaranteed broken on g200 and below" comment was due to never bothering to
check which chip it's running on.  the Render accel as currently written always
assumes two texture units, and g200 and below only have one if they have one at all.

I've taken the latest patch, applied a bunch of fixes and put it in the "exa"
branch of the mga driver:
http://gitweb.freedesktop.org/?p=xorg-driver-xf86-video-mga;a=shortlog;h=exa

It still doesn't work correctly though.

Issues left:
* a8 add a8 artifacts
* DRI
* DGA?

Also need to check whether we can drop the MGA_G400_TC2_MAGIC flag in
single-texturing mode.

Eric's recent rendercheck commits show that not only Add is broken for a8 writes.

All ops show problems _except_ Clear, Src, Dst and AtopReverse (and maybe the
Disjoint and Conjoint ops, I don't even test these yet).
It might be coincidence that AtopReverse works, but I'm not sure. I checked that
there's values for the alpha channel other than 0 and 1.0.

In the Add breakage, the alpha channel would always be set to 1.0, but now eg
with Out, I'm seeing an expected value of 1.0, and a tested one of 0.0 - so it's
not always making the destination texture opaque.

I played some more with that bypass332 flag for MACCESS.

Over, In, InReverse, Out, OutReverse, Atop, Add work better (with varying
degree) if it's not set. The other ops want it though.

Not sure what that means, as I have no idea what that 332 conversion really does.
I also didn't test it, but I suspect that _not_ setting the bypass332 flag blurs
the texture as it does for Add.

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

This died off 5 years ago.  Do we still care, or "WONTFIX"?

Didn't this patch make it into the DDX after all? In any event, if you want to close it as WONTFIX, clarify that you'd still accept a patch if some one else does all the work.

mga driver has had exa support for a while now, though I have no real idea how well it works (and am unlikely to fix it if it's broken).