Re: [PATCH] fb: Fix memcpy abuse
Keith Packard kei...@keithp.com (23/08/2011): I've merged a fix to master today; I'd love to get a bit of testing before I finish the (slightly delayed) 1.11 release. Sorry I didn't make it before 1.11; it's in unstable now though, so any regression should be reported soon enough, and possibly be fixable for the first stable bugfix release. Mraw, KiBi. signature.asc Description: Digital signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
On Mon, 16 May 2011 18:05:41 +0200, Cyril Brulebois k...@debian.org wrote: I'm about to send a follow-up to your initial patch, I tried to preserve authorship information, but not exactly sure how to handle the v2-with-a-different-author thing. Feel free to point any issue. I've merged a fix to master today; I'd love to get a bit of testing before I finish the (slightly delayed) 1.11 release. -- keith.pack...@intel.com pgp0N99nMF9fs.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
On Tue, 24 May 2011 16:31:27 +0200, Soeren Sandmann sandm...@cs.au.dk wrote: If a requirement is merged for pixman 0.23, we can make 0.24 happen before August 19th. Thanks. -- keith.pack...@intel.com pgpDuJMh1tguv.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
Keith Packard kei...@keithp.com writes: On Mon, 23 May 2011 14:34:55 +0200, Soeren Sandmann sandm...@cs.au.dk wrote: I think Keith said that the merge window for 1.11 closes May 29th though, so there may not be enough time. That's the current schedule, with the release happening on August 19. Let me know if that works for pixman and we can look at merging a requirement for 0.23 before the merge window closes this Friday. If a requirement is merged for pixman 0.23, we can make 0.24 happen before August 19th. Soren ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
Jeremy Huddleston jerem...@apple.com writes: That's quite easy to fix. Too bad that this problem was brought up on xorg-devel mailing list a few weeks too late for overlapped pixman_blt to be introduced in pixman 0.22.0. My main concern earlier was about how to use the newly added pixman features in xserver and provide a smooth upgrade path without breaking anything. But now I understand that it just requires adding this feature to pixman, then wait till the next stable pixman version 0.24.0 goes out, then add the needed changes to xserver to use this feature and bump the required pixman version to 0.24.0 at the same time. And finally the users will be able to enjoy faster non-hardware accelerated scrolling after the next stable xserver version gets released and adopted by linux distros. Right? Yeah, that's about right. Get it into pixman-0.23.x. Once we actually have a 0.23.x release of pixman, we can add that functionality to xserver master with appropriate dependency-foo added to configure.ac. Right - the X server can depend on an unstable version of pixman as long as the stable pixman will come out before a stable X server, so - add feature to pixman 0.23.x - release 0.23.2 - add feature to X server would be the procedure. Looking at past release cycles, my guess is that 0.24 will be out around the same time as xorg-server-1.11, so I don't think there will be any objections to such a dependency... Soren, can you please confirm my assumptions here =) As far as pixman is concerned, it has more or less kept a six month release cycle, which would put 0.24 around November 2011. However, I'd be fine with a short 0.24 release coming out in August if that's useful. I think Keith said that the merge window for 1.11 closes May 29th though, so there may not be enough time. Soren ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
On Mon, 23 May 2011 14:34:55 +0200, Soeren Sandmann sandm...@cs.au.dk wrote: I think Keith said that the merge window for 1.11 closes May 29th though, so there may not be enough time. That's the current schedule, with the release happening on August 19. Let me know if that works for pixman and we can look at merging a requirement for 0.23 before the merge window closes this Friday. -- keith.pack...@intel.com pgptIf6X6r7c1.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
On May 21, 2011, at 05:03, Siarhei Siamashka wrote: On Sat, Apr 30, 2011 at 3:39 PM, Soeren Sandmann sandm...@cs.au.dk wrote: If I remember correctly, the main objection I had to the overlapped blt patch was that it inlined the C fallback into all the SIMD versions instead of just calling down through the delegate. That's quite easy to fix. Too bad that this problem was brought up on xorg-devel mailing list a few weeks too late for overlapped pixman_blt to be introduced in pixman 0.22.0. My main concern earlier was about how to use the newly added pixman features in xserver and provide a smooth upgrade path without breaking anything. But now I understand that it just requires adding this feature to pixman, then wait till the next stable pixman version 0.24.0 goes out, then add the needed changes to xserver to use this feature and bump the required pixman version to 0.24.0 at the same time. And finally the users will be able to enjoy faster non-hardware accelerated scrolling after the next stable xserver version gets released and adopted by linux distros. Right? Yeah, that's about right. Get it into pixman-0.23.x. Once we actually have a 0.23.x release of pixman, we can add that functionality to xserver master with appropriate dependency-foo added to configure.ac. Looking at past release cycles, my guess is that 0.24 will be out around the same time as xorg-server-1.11, so I don't think there will be any objections to such a dependency... Soren, can you please confirm my assumptions here =) Sure. Just these other things are much less commonly used and do not require any urgent attention. It's more like a code refactoring/cleanup work but not something that is clearly beneficial for the end users. Refactoring and code cleanup is almost always beneficial for the end users, albeit indirectly. It helps us more easily spot bugs and more easily maintain the codebase. --Jeremy ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
Adam Jackson a...@redhat.com (02/05/2011): I like the idea of having more in pixman and less in fb. […] I'm also happy with that, but I'd really love to get correctness in the server in the meanwhile. I've applied Soeren's and Walter's suggestions and the offending memcpy() calls are apparently gone (thanks to the glibc wrapper Aurélien Jarno implemented to report memcpy calls with overlapping areas through syslog[1]), tested by playing the resize-my-terminal-like-crazy game. 1. http://packages.qa.debian.org/e/eglibc/news/20110511T224840Z.html I'm about to send a follow-up to your initial patch, I tried to preserve authorship information, but not exactly sure how to handle the v2-with-a-different-author thing. Feel free to point any issue. Mraw, KiBi. signature.asc Description: Digital signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
On Thu, Apr 21, 2011 at 11:37 PM, Adam Jackson a...@redhat.com wrote:
The memcpy fast path implicitly assumes that the copy walks
left-to-right. That's not something memcpy guarantees, and newer glibc
on some processors will indeed break that assumption. Since we walk a
line at a time, check the source and destination against the width of
the blit to determine whether we can be sloppy enough to allow memcpy.
(Having done this, we can remove the check for !reverse as well.)
On an Intel Core i7-2630QM with an NVIDIA GeForce GTX 460M running in
NoAccel, the broken code and various fixes for -copywinwin{10,100,500}
gives (edited to fit in 80 columns):
1: Disable the fastpath entirely
2: Replace memcpy with memmove
3: This fix
4: The code before this fix
1 2 3 4 Operation
-- --- --- ---
258000 269000 ( 1.04) 544000 ( 2.11) 552000 ( 2.14) Copy 10x10
21300 23000 ( 1.08) 43700 ( 2.05) 47100 ( 2.21) Copy 100x100
960 962 ( 1.00) 1990 ( 2.09) 1990 ( 2.07) Copy 500x500
So it's a modest performance hit, but correctness demands it, and it's
probably worth keeping the 2x speedup from having the fast path in the
first place.
In my opinion, still the best solution is to do this stuff in pixman,
because it has its own SIMD optimized code and can do a lot better job
than memcpy/memmove (for example, it can be improved to use MOVNTx
instructions for x86 when scrolling large areas exceeding L2/L3 cache
size, and this optimization can't be easily done by glibc if
memcpy/memmove is used separately for each individual scanline). I did
some experiments with improving scrolling performance when using
non-hardware accelerated framebuffer earlier and it showed a really
major speedup on ARM:
http://lists.x.org/archives/xorg-devel/2009-November/003536.html
I'm a bit surprised that there were no other people even trying to
push for something similar because the scrolling performance issue is
so obvious. You are more likely to see the people trying to use ARM
devices without hardware acceleration, become horrified by poor
scrolling performance, and then start searching for a way to obtain
and install the proprietary binary drivers :)
Signed-off-by: Adam Jackson a...@redhat.com
---
fb/fbblt.c | 9 +++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fb/fbblt.c b/fb/fbblt.c
index a040298..b955245 100644
--- a/fb/fbblt.c
+++ b/fb/fbblt.c
@@ -65,6 +65,7 @@ fbBlt (FbBits *srcLine,
int n, nmiddle;
Bool destInvarient;
int startbyte, endbyte;
+ int careful;
FbDeclareMergeRop ();
if (bpp == 24 !FbCheck24Pix (pm))
@@ -74,12 +75,16 @@ fbBlt (FbBits *srcLine,
return;
}
- if (alu == GXcopy pm == FB_ALLONES !reverse
+ careful = !((srcLine dstLine srcLine + width * (bpp/8) dstLine) ||
+ (dstLine srcLine dstLine + width * (bpp/8) srcLine))
||
+ (bpp % 8);
+
+ if (alu == GXcopy pm == FB_ALLONES !careful
!(srcX 7) !(dstX 7) !(width 7)) {
int i;
CARD8 *src = (CARD8 *) srcLine;
CARD8 *dst = (CARD8 *) dstLine;
-
+
srcStride *= sizeof(FbBits);
dstStride *= sizeof(FbBits);
width = 3;
What is the story with the 'reverse' variable? Does it have no use
anymore and needs to be purged later?
--
Best regards,
Siarhei Siamashka
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] fb: Fix memcpy abuse
Adam Jackson a...@redhat.com writes: -if (alu == GXcopy pm == FB_ALLONES !reverse +careful = !((srcLine dstLine srcLine + width * (bpp/8) dstLine) || +(dstLine srcLine dstLine + width * (bpp/8) srcLine)) || + (bpp % 8); The srcLine and dstLine variables are pointers to FbBits which is an alias for uint32_t, but width * (bpp/8) computes the number of *bytes* in the line. For this to work, I think srcLine and dstLine pointers need casts to (CARD8 *). Also, in principle, the first in each line should be =. Otherwise the code would miss the case where the two lines overlap because they are identical. This is only a bug in principle since the lines shouldn't be identical, or if they are, memcpy() probably would be fine. Finally, I think the overlapping check could be simplified by considering how to check that the lines *don't* overlap. This is the case precisely when the end of the src line is before or equal to the beginning of the dst line, or the beginning of the src line is after or equal to the end of the dst line: no_overlap = ((CARD8 *)srcLine + width * (bpp / 8) = dstLine || (CARD8 *)dstLine + width * (bpp / 8) = srcLine); Negating and applying De Morgan then gives: overlap = (CARD8 *)srcLine + width * (bpp / 8) dstLine (CARD8 *)dstLine + width * (bpp / 8) srcLine; So the careful check then looks something like this: careful = ((CARD8 *)srcLine + width * (bpp / 8) dstLine (CARD8 *)dstLine + width * (bpp / 8) srcLine) || (bpp % 8); Soren ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
