lkml.org 
[lkml]   [2015]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms
    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@arm.linux.org.uk>:
    > > 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.


    \
     
     \ /
      Last update: 2015-02-03 15:41    [W:5.592 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site