|
Description
frank huang
2010-06-01 22:55:26 UTC
Make progress in rendering bug in OVER. See my attached pictures for progress bar on new driver. Martin and Mart, give the patch on xorg-geode mail group?? Thanks, Frank Created attachment 36027 [details]
This is old geode driver drawing the progressbar and its result
This is old geode driver drawing the progressbar and its result
Created attachment 36028 [details]
This is new geode driver drawing the progressbar and its result
This is new geode driver drawing the progressbar and its result
Created attachment 36029 [details]
correct result on ATI graphics card as a comparison
correct result on ATI graphics card as a comparison
Created attachment 36030 [details]
This is new geode driver drawing the scrollbar and its result
This is new geode driver drawing the scrollbar and its result
(In reply to comment #1) > Make progress in rendering bug in OVER. See my attached pictures for progress > bar on new driver. > > Martin and Mart, > give the patch on xorg-geode mail group?? Preferably attach the patch to this bug. Created attachment 36056 [details] [review] The new patch for the rendering The new patch for the rendering I have put the patch here and need Martin's help to commit. Hope you guys give me more feedback on the patch. Any questions are welcome. I'll go on researching. Thanks, Frank Created attachment 36092 [details] [review] Patch to solve the rendering issue on 2.11.8 version 1.Change the srcColor type from unsigned int to unsigned long for 32 bit color value use(for example. ARGB32) 2.Add the maskrepeat to record if the mask picture is repeatable 3.Correct the PictOpOver operation in lx_alpha_ops struct 4.Return FALSE in lx_check_composite() for every FALSE condition in lx_prepare_composite(), forbidding the pixmap migration as soon as possible 5.For PictOpOver operation, get the source color from the source pixmap. If that is RGB565, convert is to ARGB32; If that is ARGB32, get the first pixel color 6.Correct the PictOpSrc and PictOpOver operation with mask value 7.Add a fucntion lx_do_composite_maks_opover() to handle the PictOpOver condition with source ARGB32 and maks A8 8.Fix the progressbar and scrollbar bug Thanks, Frank Ideally these change would be worked on in GIT with GIT capabilites, so you can easily make it be 8 patches, instead of one big. So one patch per change. This way it would be much easier to review, get things right, and that's just the right way in open source and GIT :) I hope the changes can be split apart into multiple commits/patches from hereon. Please contact me for any help needed with that. I tested the patch from Friday, which seems similar or the same, and that indeed nicely fixed up my scrollbar/button borders and progressbar drawing issues, but didn't look closely on the concrete code changes yet. Comments on things described in the changelog: 1. Why do you need to change unsigned int to unsigned long to store 32bit values? Unsigned int and unsigned long are both 32bit on x86, and conceptually unsigned int is more correct (also "unsigned long" is 64bit on x86-64, but geode is 32bit of course). Even more correct would be something like __u32 (a Xorg typedef to uint32_t), but we should use whatever other places use for storing 32bit colors, but that's just choice of which typedef. 4. If we can reliably catch inability to accelerate already in CheckComposite (all information in the checks is available to use at CheckComposite time), then we should indeed do it in there to fallback as early as possible (before pixmaps are migrated to GPU memory - memcpy overhead). But this also should mean, that we don't need to do the checks in PrepareComposite anymore, as if CheckComposite returned FALSE, we are guaranteed to not get a call to PrepareComposite with the same rendering commands. So the checks that can be done earlier should moved, not copied. All in all, seems like awesome work, as it does fix most of my most awful rendering problems :) Now lets get this in properly in multiple commits and so on, so it would be easier to do it like that in the future too :) Created attachment 36207 [details]
Picture in chaos in file browser
(In reply to comment #12) > Created an attachment (id=36207) [details] > Picture in chaos in file browser Did you obtain this screenshot with or without your patch applied? This condition can be obtained without my patch. Still I suspect it is a PictOpOver opearation bug. I am finding it now. If you have any good simple example that can reproduce the same issue, please attach your application here. Thanks, Frank Answer for Mart's reply,sorry for late. Split it into 7 patched commited in xorg-driver-geode. I have acceptted your two suggestion below in my patch. Thanks, Frank Created attachment 36229 [details]
Attach a gtk program to reproduce this issue
Created attachment 36230 [details]
Attach this gtk program's result with rendering error(see green part)
Add a program attachment to reproduce the chaos picture issue. (In reply to comment #9) > Created an attachment (id=36092) [details] > Patch to solve the rendering issue on 2.11.8 version I tested this patch on the XO-1. It fixes the black boxes in Gnome and works well across all the Sugar activities I could test. Would now be a good time to push Frank's 7 patches into GIT to facilitate wider testing? (In reply to comment #20) > Would now be a good time to push Frank's 7 patches into GIT to facilitate wider > testing? I have not done any reviewing, but I'd like to see this code land in git. Then I'll use a git snapshot to build a package for Fedora 11, which is what currently runs on the XO-1 and XO-1.5. Bernie, thanks for your test. Glad to hear the news that this patch can work for 15700. I suggest you guys after review, please commit these patches to community for users to test to give me more feedback. Thanks Frank Give a new patch(the 8th) for your test to solve the chaos picture issue. I should make sure this patch reasonable after I discussed with guys on xorg-devel. Thanks, Frank Created attachment 36432 [details]
Wrong rendering on Firefox characters
Wrong rendering on Firefox characters
Created attachment 36486 [details]
Black region and discrete line in firefox
Black region and discrete line in firefox
Allocate the FB memory from 24MB to 32MB in BIOS, firefox mis-rendering issue is disappearring. Off-screen memory region is not big enough for use in 24MB condition. But the patch's modification is correct:). Reserch more on Off-screen memory allocation under 24MB and its use. Thanks, Frank Copy from mail:
>Have you narrowed down if the problem is due to your code failing to handle an offscreen memory allocation failure, or something else?
Your guess is same as me after I enlarge the off-screen memory size that result in better result. Based on my debug, the bug is right here:). Today I have fixed this bug in our driver.
Just as the previous code writes, the off-screen memory size is fixed 256KB(which is reserved long long ago), and in firefox, when you press a refresh button, a big mask region 768x268 is need to be rendered with PictOpOver. You know, for the PictOpOver, the first pass is to do src*mask, and the result is ARGB format. So the off-screen size is 768x268x4=823296 > 256KB, so it overflows! The black region and missing glyph character are due to this.
After I know this, I adjust the off-screen region to 2MB instead of 256KB. As expected, the firefox mis-rendering bug is missing. That proves my guess and the debug result. But as you know, our Framebuffer size is not big enough in Geode, especially for OPLC computer. And 2MB is not big enough. Take an example, 1280x1024x4 is nearly 5MB. So that is possible to allocate so huge memory for off-screen operation.
And Jonathan give me one-line hint, use scanline method(Jonathan usually give me the direction!). So I split the rendering into several chunk, each is 256KB, then do it. Right now, It works fine:). I am happy with this. You know that is very important for us to use a small off-screen region that can make other FB region free. So this patch is valuable.
Thanks,
Frank
Created attachment 37062 [details]
20x20 source picture, 40x40 rendering region with 4 colours
20x20 source picture, 40x40 rendering region with 4 colours
Created attachment 37063 [details]
out of region, src is 22, compared with previous one
out of region, src is 22
Created attachment 37290 [details]
wrong glyph rendering using HW+SW
wrong glyph rendering using HW+SW
Lower the priority of this issue. The patches for misrendering main bug has been released and in review. Update for patch 4: http://lists.x.org/archives/xorg-driver-geode/2010-August/000952.html Thanks, Frank 5b93fdd42d7d8af4535fd62ce0749f2c4434f9fe in Geode 2.11.9 fixes this if I'm not mistaken? This issue can be closed now. Main rendering bugs have been solved in 2.11.9 release. The fixed ops including PictOpOver, PictOpSrc, PictOpClear, PictOpIn, PictOpInReverse, PictOpOut, PictOpOutReverse, PictOpAdd. Any new bug reported, we can reopen this issue. Thanks, Frank |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.