On Tue, Feb 03, 2015 at 02:28:26PM +0100, Christian Gmeiner wrote: > 2015-02-03 13:28 GMT+01:00 Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx>: > > What I've found with *my* etnaviv drm implementation (not Christian's - I > > found it impossible to work with Christian, especially with the endless > > "msm doesn't do it that way, so we shouldn't" responses and his attitude > > towards cherry-picking my development work [*]) is that it's much easier to > > keep the GPU MMU local to the GPU and under the control of the DRM MM code, > > rather than attaching the IOMMU to the DMA API and handling it that way. > > > > Keep in mind that I tried to reach you several times via mail and irc > and you simply > ignored me. Did you know that took almost all of your patches (with > small changes)? > And I needed to cherry pick you patches as they were a) wrong, b) solved in a > different way or c) had "hack" in the subject. I am quite sorry that I > ended that > way, but it is not only my fault! Exactly - you *took* every patch that I published whether I was ready for you to take it or not. That's not how kernel development works. Kernel development works by people working on the code, and *pushing* patches or git trees upstream. It doesn't work by having people running around *taking* patches from people just because they feel like it. I asked you several times not to do that which means the only way I can control you is by *not* publishing my changes, thereby denying other people the ability to test my changes. Another result of you *taking* patches from me is that you totally *screwed* my ability to work with you. If you make this stuff unnecessary hard, you can expect people to walk away, and that's precisely what I've done. There's also the issue of you *taking* my patches and then applying them to your tree with your own modifications, again *screwing* my tree, and screwing my ability - again - to work with you. Many of my patches in your repository are also marked as you being the author of them... which _really_ is not nice. Your "review" comments of "based 1:1 on the MSM driver" were really crazy. Just because one DRM driver does something one way does not make it the only way, nor does it make it suitable for use everywhere, even if you modelled your driver on MSM. It certainly doesn't mean that the way the MSM driver does it is correct either. And frankly, you calling my patches "wrong" is laughable. I have a stable fully functional Xorg DDX driver here which works with my version of your etnaviv DRM across several Vivante GPUs - GC660 on Dove, and two revisions of GC320 on iMX6 (which are notoriously buggy). No kernel oopses, no GPU lockups. I've had machines with uptimes of a month here with it, with the driver being exercised frequently during that period. You refused to take things such as the DMA address monitoring for GPU hangups to stop it mis-firing. This _is_ a bug fix. Without that, your driver spits out random GPU hangups when there isn't actually any hangup at all. *Reliably* so. Your excuse for not taking it was "The current vivante driver doesn't do that." While that's true, the V2 Vivante drivers _do_ do it in their "guard thread" - and they do it because - as I already explained to you - it's entirely possible for the GPU to take a long time, longer than your hangcheck timeout, to render a series of 1080p operations. And again, not everything that the Vivante drivers do is correct either. Jon and myself know that very well having spent a long time debugging their GPL'd offerings. Even the "hack" patch was mostly correct - the reason that it is labelled as a "hack" is because - as the commit log said - it should really be done by the MMUv1 code, but that requires your entire IOMMU _bodge_ to be rewritten properly. Even the Vivante drivers use that region when they can. Then there's also the compatibility with the etnaviv library - which is an important thing if you want people to make use of your driver. You applied the patches for and then reverted which completely screws the Xorg DDX driver, making it impossible to support both etnaviv and etnadrm without having two almost identical copies of the same code. I don't want to maintain almost identical copies of that same code, and no one in their right mind would want that. Having some level of sane user compatibility between etnaviv and etnaviv drm will _gain_ you uses as it will allow people to write code which works on both platforms - and it's really not difficult to do. (In fact, I've proven it by writing a shim layer between the etnaviv API and the DRM interfaces in the DDX driver.) Then there's the round-robin IOMMU address space allocation, which is required to avoid corrupted pixmaps (which is something that _your_ driver does very very well - in fact, so well that it corrupts system memory), and the reaping of the IOMMU space when we run out of IOMMU space to map. Now, on to things that you do wrong. There's your bodge with the component helper too which I'm disgusted with, and I'll tell you now - your use of the component helper is wrong. In your latest repository, there's this reserved-memory thing which you've invented - to work around iMX6 with 2GB of RAM allocating the command ring buffer at physical addresses >= 0x80000000. That's absolutely not necessary, the GPU has physical offset registers which can be used to program the lower 2G of MMUv1 space at the appropriate offset - and there's good reasons to do that - it prevents the GPU being able to access 0x00000000-0x10000000 which are where the peripheral registers are on the iMX6 - it prevents the GPU being able to scribble into (eg) the SDRAM controller registers etc potentially taking the system down. Yes, it won't work if we see 3G on iMX6, but that's something which can be addressed in other ways (such as passing appropriate GFP_* flags.) Here's a reminder of the kind of "high quality" review comments you provided: * staging: etnaviv: ensure cleanup of reservation object commit 729b31a8dd07d5db744cc5cb32c28ebf2e8cadb5 This is based 1:1 on msm drm driver. I will talk with Rob about it and will postpone this patch. * staging: etnaviv: implement MMU reaping commit 0c7f0736cc02ba83dea15d73e9fa98277839ca67 I will _NOT_ take this one. * staging: etnaviv: stop the hangcheck timer mis-firing commit f0c3d8f2bf81774aaf19284ec3287e343a772e63 Too hacky, current vivante drivers do not have this hack. I will _NOT_ take this one. * staging: etnaviv: increase iommu page table size to 512KiB commit 610da4c465732f1c20495f8d7d9504ec25665bb0 I will _NOT_ take this one. I will postpone it. * staging: etnaviv: fix fence wrapping for gem objects commit 218e8ffce1d57a797e7f3234cd8ffd62fc4dab71 This is based 1:1 on msm drm driver. I will talk with Rob about it and will postpone this patch. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
