Bug 1293 - MGA EXA support
MGA EXA support
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Driver/mga
git
x86 (IA32) Linux (All)
: medium enhancement
Assigned To: ajax at nwnk dot net
Xorg Project Team
2011BRB_Reviewed
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-09-05 16:40 UTC by ajax at nwnk dot net
Modified: 2014-03-20 21:33 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
mga-render-accel-6-7.patch (573 bytes, patch)
2004-09-06 20:36 UTC, ajax at nwnk dot net
no flags Details | Splinter Review
mga-render-accel-6-8.patch (2.82 KB, patch)
2004-09-06 20:47 UTC, ajax at nwnk dot net
no flags Details | Splinter Review
mga-render-accel-6-8-v2.patch (3.06 KB, patch)
2004-09-14 09:17 UTC, ajax at nwnk dot net
no flags Details | Splinter Review
Output of rendercheck using mga(4) with the v2 patch applied. (30.52 KB, text/plain)
2004-09-14 17:39 UTC, Tiago de Paula Peixoto
no flags Details
Rendercheck output with x8r8g8b8 added to format list (38.24 KB, text/plain)
2004-09-14 21:45 UTC, Tiago de Paula Peixoto
no flags Details
mga-render.diff (42.42 KB, patch)
2004-12-20 23:29 UTC, Eric Anholt
no flags Details | Splinter Review
Updated mga-render.diff to work with current CVS HEAD (42.25 KB, patch)
2005-10-10 01:10 UTC, Tilman Sauerbeck
no flags Details | Splinter Review
Updated mga-render.diff to work with current CVS HEAD (fixed) (42.23 KB, patch)
2005-10-10 03:15 UTC, Tilman Sauerbeck
no flags Details | Splinter Review
Screenshot of a GTK app that shows buggy text rendering (48.11 KB, image/png)
2005-10-14 04:12 UTC, Tilman Sauerbeck
no flags Details
mga-exa-1.patch (14.29 KB, patch)
2006-03-12 08:50 UTC, ajax at nwnk dot net
no flags Details | Splinter Review
mga-exa-2.patch (33.29 KB, patch)
2006-03-21 09:09 UTC, ajax at nwnk dot net
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description ajax at nwnk dot net 2004-09-05 16:40:01 UTC
in MGANAME(AccelInit) in mga_storm.c, we always disable doRender for g200
through g550.  doRender controls the initialization of the Render accel hooks.

this looks like it was double-fixed between 4.3 and 4.4; once to disable it for
dualhead setups and again to disable it entirely.  i haven't checked the xf86
bugzilla for an explanation, but we should at least enable Render accel for
single head.
Comment 1 ajax at nwnk dot net 2004-09-06 20:36:40 UTC
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.
Comment 2 ajax at nwnk dot net 2004-09-06 20:47:14 UTC
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.
Comment 3 Eric Anholt 2004-09-09 12:02:23 UTC
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)
Comment 4 Tiago de Paula Peixoto 2004-09-10 09:44:48 UTC
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. :-]
Comment 5 ajax at nwnk dot net 2004-09-14 09:17:36 UTC
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?
Comment 6 Tiago de Paula Peixoto 2004-09-14 13:52:24 UTC
(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...
Comment 7 ajax at nwnk dot net 2004-09-14 13:56:50 UTC
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?
Comment 8 Tiago de Paula Peixoto 2004-09-14 17:39:26 UTC
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...
Comment 9 ajax at nwnk dot net 2004-09-14 17:54:02 UTC
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().
Comment 10 Eric Anholt 2004-09-14 18:17:27 UTC
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.
Comment 11 Tiago de Paula Peixoto 2004-09-14 19:01:25 UTC
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.
Comment 12 Tiago de Paula Peixoto 2004-09-14 19:21:53 UTC
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...
Comment 13 ajax at nwnk dot net 2004-09-14 21:04:27 UTC
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?
Comment 14 Eric Anholt 2004-09-14 21:07:37 UTC
The Alpha call should be happening for non-subpixel, antialiased Xft text that's
a solid color (i.e. almost all the time).
Comment 15 Tiago de Paula Peixoto 2004-09-14 21:45:58 UTC
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?
Comment 16 Eric Anholt 2004-09-14 22:05:16 UTC
(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.
Comment 17 Tiago de Paula Peixoto 2004-09-15 18:09:04 UTC
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?
Comment 18 Tiago de Paula Peixoto 2004-09-15 18:37:20 UTC
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.

Comment 19 Daniel Goller 2004-09-18 22:27:17 UTC
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?

Comment 20 Eric Anholt 2004-12-20 23:29:58 UTC
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.
Comment 21 Eric Anholt 2004-12-24 14:10:04 UTC
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.
Comment 22 ajax at nwnk dot net 2005-01-06 15:33:17 UTC
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.
Comment 23 ajax at nwnk dot net 2005-07-03 12:43:33 UTC
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.
Comment 24 Tilman Sauerbeck 2005-10-10 01:10:37 UTC
Created attachment 3529 [details] [review]
Updated mga-render.diff to work with current CVS HEAD
Comment 25 Tilman Sauerbeck 2005-10-10 03:15:11 UTC
Created attachment 3531 [details] [review]
Updated mga-render.diff to work with current CVS HEAD (fixed)

Sorry, attached a bad patch earlier.
Comment 26 ajax at nwnk dot net 2005-10-10 11:10:29 UTC
i would strongly prefer to see Render acceleration for mga(4) done through EXA
at this point.
Comment 27 Tilman Sauerbeck 2005-10-11 10:25:38 UTC
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.
Comment 28 ajax at nwnk dot net 2005-10-11 11:03:45 UTC
(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.
Comment 29 Tilman Sauerbeck 2005-10-14 04:12:43 UTC
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.
Comment 30 Tilman Sauerbeck 2005-10-14 04:30:04 UTC
(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.
Comment 31 ajax at nwnk dot net 2006-03-12 08:48:28 UTC
got bored, started hacking an EXA conversion.
Comment 32 ajax at nwnk dot net 2006-03-12 08:50:38 UTC
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.
Comment 33 ajax at nwnk dot net 2006-03-21 09:09:02 UTC
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.
Comment 34 Daniel Goller 2006-03-21 15:28:18 UTC
i don't have a machine to test this on currently, is this expected to work well
on a G550?
Comment 35 ajax at nwnk dot net 2006-03-22 09:34:47 UTC
(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.
Comment 36 Tilman Sauerbeck 2006-08-10 13:43:52 UTC
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.
Comment 37 Tilman Sauerbeck 2006-09-20 12:58:41 UTC
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.
Comment 38 Tilman Sauerbeck 2006-11-19 04:05:54 UTC
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.
Comment 39 Tilman Sauerbeck 2006-11-19 05:10:21 UTC
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.
Comment 40 Daniel Stone 2007-02-27 01:24:03 UTC
Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.
Comment 41 Jeremy Huddleston 2011-10-03 09:58:21 UTC
This died off 5 years ago.  Do we still care, or "WONTFIX"?
Comment 42 Connor Behan 2012-03-24 21:35:10 UTC
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.
Comment 43 ajax at nwnk dot net 2014-03-20 21:33:09 UTC
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).