• Daniel Vetter's avatar
    dma-resv: Also prime acquire ctx for lockdep · fedf7a44
    Daniel Vetter authored
    Semnatically it really doesn't matter where we grab the ticket. But
    since the ticket is a fake lockdep lock, it matters for lockdep
    validation purposes.
    
    This means stuff like grabbing a ticket and then doing
    copy_from/to_user isn't allowed anymore. This is a changed compared to
    the current ttm fault handler, which doesn't bother with having a full
    reservation. Since I'm looking into fixing the TODO entry in
    ttm_mem_evict_wait_busy() I think that'll have to change sooner or
    later anyway, better get started. A bit more context on why I'm
    looking into this: For backwards compat with existing i915 gem code I
    think we'll have to do full slowpath locking in the i915 equivalent of
    the eviction code. And with dynamic dma-buf that will leak across
    drivers, so another thing we need to standardize and make sure it's
    done the same way everyway.
    
    Unfortunately this means another full audit of all drivers:
    
    - gem helpers: acquire_init is done right before taking locks, so no
      problem. Same for acquire_fini and unlocking, which means nothing
      that's not already covered by the dma_resv_lock rules will be caught
      with this extension here to the acquire_ctx.
    
    - etnaviv: An absolute massive amount of code is run between the
      acquire_init and the first lock acquisition in submit_lock_objects.
      But nothing that would touch user memory and could cause a fault.
      Furthermore nothing that uses the ticket, so even if I missed
      something, it would be easy to fix by pushing the acquire_init right
      before the first use. Similar on the unlock/acquire_fini side.
    
    - i915: Right now (and this will likely change a lot rsn) the acquire
      ctx and actual locks are right next to each another. No problem.
    
    - msm has a problem: submit_create calls acquire_init, but then
      submit_lookup_objects() has a bunch of copy_from_user to do the
      object lookups. That's the only thing before submit_lock_objects
      call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
      does not have this issue since it copies all the userspace structs
      earlier. submit_cleanup does not have any such issues.
    
      With the prep patch to pull out the acquire_ctx and reorder it msm
      is going to be safe too.
    
    - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
      Similar on the acquire_fini/ttm_bo_unreserve side.
    
    - ttm execbuf utils: acquire context and locking are even in the same
      functions here (one function to reserve everything, the other to
      unreserve), so all good.
    
    - vc4: Another case where acquire context and locking are handled in
      the same functions (one function to lock everything, the other to
      unlock).
    
    Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    Cc: Chris Wilson <chris@chris-wilson.co.uk>
    Cc: Christian König <christian.koenig@amd.com>
    Cc: Sumit Semwal <sumit.semwal@linaro.org>
    Cc: linux-media@vger.kernel.org
    Cc: linaro-mm-sig@lists.linaro.org
    Cc: Huang Rui <ray.huang@amd.com>
    Cc: Eric Anholt <eric@anholt.net>
    Cc: Ben Skeggs <bskeggs@redhat.com>
    Cc: Alex Deucher <alexander.deucher@amd.com>
    Cc: Rob Herring <robh@kernel.org>
    Cc: Lucas Stach <l.stach@pengutronix.de>
    Cc: Russell King <linux+etnaviv@armlinux.org.uk>
    Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
    Cc: Rob Clark <robdclark@gmail.com>
    Cc: Sean Paul <sean@poorly.run>
    Acked-by: default avatarChristian König <christian.koenig@amd.com>
    Reviewed-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
    Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
    Link: https://patchwork.freedesktop.org/patch/msgid/20191119210844.16947-3-daniel.vetter@ffwll.ch
    fedf7a44
dma-resv.c 16.3 KB