On Tue, Jun 09, 2015 at 12:13:05PM +0200, Lucas Stach wrote:
To make this a bit easier for me I would like to ask you to please keep the relevant discussions on E-Mail. Following another communication channel like IRC just makes things harder for me. I'm prepared to take critique on public mail, no need to keep things hidden. ;)
Unfortunately, a lot of discussion does happen on IRC - that's why there exists a #etnaviv channel. It's not that it's hidden, it's just that you're only there very rarely.
Am Donnerstag, den 28.05.2015, 00:03 +0100 schrieb Russell King - ARM Linux:
Lucas,
We definitely need to talk about this... please can you take a look at my patch stack which I mentioned in my reply to Alexander. I know that you already based your patch set off one of my out-dated patch stacks, but unless you grab a more recent one, you're going to be solving a lot of bugs that I've fixed.
Right, thanks for the pointer. I took a look at this and it doesn't seem to clash with things I did until now, so it should be pretty easy for me to resync your changes into a common tree.
That's good.
Also, I've heard via IRC that you've updated my DDX - which is something I've also done (I think I did mention that I would be doing this... and I also have a bunch of work on the Xrender backend.) I suspect that makes your work there redundant.
I've minimally beat it into shape to work on top of the new kernel driver and fixed some XV accel bugs when used with GStreamer.
What bugs are they? It would be nice to have at least bug fixes sent in a timely manner.
I'm also looking at accelerating rotated blits to speed up the rotated display case. I consider none of this to be stable and tested enough to push it out right now. I'll keep watching the things you do the driver and ping you if I have anything worthwhile to integrate.
Rotated blits for general pixmaps don't make sense.
The only place where this becomes useful is in Xrender's composite method, where we have picture transforms to deal with.
However, I forsee problems there, as miComputeCompositeRegion() doesn't take account of any rotations. (The source pict->clientClip is in unrotated coordinates looking at miValidatePicture(), and miClipPictureSrc() will apply that clip region in an untranslated manner to the destination.) Even considering flips, I'm not sure it's valid to use the region computed by miComputeCompositeRegion() as the computed region would appear (at least to me) to be wrong. Maybe I'm misunderstanding something in the Xrender extension though.
I have some experimental code I'm running on iMX6 which implements the Xrandr H and V flips, as well as 180° rotation (iow, H and V flip) but the conclusion I've come to is it will probably be much better (and more efficient) to use any abilities of the KMS driver to do the final rotation/flipping rather than having the GPU do this. Couple this with my point above, and it's the reason why I haven't put much effort into that bit.
Another issue is that we incur quite a heavy overhead if we allocate an etnadrm buffer, and then map it into userspace. We end up allocating all pages for the buffer as soon as any page is faulted in (consider if it's a 1080p buffer...) and then setup the scatterlists. That's useless overhead if we later decide that we're not going to pass it to the GPU (eg, because Xorg allocated a pixmap which it then only performed CPU operations on.) I have "changes" for this which aren't prepared as proper patches yet (in other words, they're currently as a playground of changes.)
I think this is a userspace problem. Userspace should not allocate any GEM objects until it is clear that the buffer is going to be used by the GPU. Experience with the Intel DDX shows that a crucial part for good X accel performance is to have a proper strategy for migrating pixmaps between the CPU and GPU in place, even with UMA.
I don't think so. There are certain cases where we know that a pixmap is never going to be supported by the GPU (eg, unsupported format) but there are a lot of cases where we just don't know.
The problem is that bouncing a 1080p pixmap between the CPU and GPU is expensive. If we're the situation where we have multiple different buffers for a single pixmap (eg, one for the fb code, one for the GPU) then we'll be forever copying pixels between the two, which just adds to the overhead. Yes, I'm aware that i915 does damage tracking to reduce the amount of copying, but that still doesn't get away from reading from an uncached pixmap.
(btw, when this code runs on Armada DRM, it's running with fully cached pixmaps allocated by Armada DRM, it doesn't use the Etnaviv allocator, and it doesn't suffer from these overheads as a result.)
I have other improvements pending which needs proper perf analysis before I can sort them out properly. These change the flow for a submitted command set - reading all data from userspace before taking dev->struct_mutex, since holding this lock over a page fault isn't particularly graceful. Do we really want to stall graphics on the entire GPU while some process' page happens to be swapped out to disk? I suspect not.
As long as those changes don't incur any userspace visible changes it should be safe to keep them in the playground until the basics are mainline. Nonetheless I would be grateful if you could point me to those patches, so I can get a picture of how those changes would look like.
They don't, but the one we do need to solve before we can think about mainlining anything is the cache status of the command buffers, and whether we still use bos in userspace for those. If we're going to switch to reading via copy_from_user() (which has the advantage that it'll work on virtual caches - having userspace write via the userspace alias, and then trying to read from the kernel space alias is fraught with problems for systems with virtual caches) then there's no point in having the commands stored in a bo.