Re: [PATCH] drm/i915: Document locking guidelines

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Aug 30, 2019 at 01:50:53PM +0300, Joonas Lahtinen wrote:
> To ensure cross-driver locking compatibility, document the expected
> guidelines for implementing the GEM locking in i915. Note that this
> is a description of how things should end up after being reworked,
> and does not reflect the current state of things.
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: CQ Tang <cq.tang@intel.com>
> ---
>  Documentation/gpu/i915.rst | 45 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index e249ea7b0ec7..63a72d10f2c7 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -320,6 +320,51 @@ for execution also include a list of all locations within buffers that
>  refer to GPU-addresses so that the kernel can edit the buffer correctly.
>  This process is dubbed relocation.
>  
> +Locking Guidelines
> +------------------
> +
> +**NOTE:** This is a description of how the locking should be after
> +refactoring is done. Does not necessarily reflect what the locking
> +looks like while WIP.

Maybe use rst note block for this?
.. note::

> +
> +#. All locking rules and interface contracts with cross-driver interfaces
> +   (dma-buf, dma_fence) need to be followed.
> +
> +#. No struct_mutex anywhere in the code
> +
> +#. dma_resv will be the outermost lock (when needed) and ww_acquire_ctx
> +   is to be hoisted at highest level and passed down within i915_gem_ctx
> +   in the call chain
> +
> +#. While holding lru/memory manager (buddy, drm_mm, whatever) locks
> +   system memory allocations are not allowed
> +
> +	* Enforce this by priming lockdep (with fs_reclaim). If we
> +	  allocate memory while holding these looks we get a rehash
> +	  of the shrinker vs. struct_mutex saga, and that would be
> +	  real bad.
> +
> +#. Do not nest different lru/memory manager locks within each other.
> +   Take them in turn to update memory allocations, relying on the object’s
> +   dma_resv ww_mutex to serialize against other operations.
> +
> +#. The suggestion for lru/memory managers locks is that they are small
> +   enough to be spinlocks.
> +
> +#. All features need to come with exhaustive kernel selftests and/or
> +   IGT tests when appropriate
> +
> +#. All LMEM uAPI paths need to be fully restartable (_interruptible()
> +   for all locks/waits/sleeps)
> +
> +	* Error handling validation through signal injection.
> +	  Still the best strategy we have for validating GEM uAPI
> +          corner cases.
> +	  Must be excessively used in the IGT, and we need to check
> +	  that we really have full path coverage of all error cases.
> +
> +	* -EDEADLK handling with ww_mutex
> +

It seems clear and clean to me. So from my point of view:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


>  GEM BO Management Implementation Details
>  ----------------------------------------
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx