drm/i915: Document locking guidelines

Commit Message

Joonas Lahtinen Aug. 30, 2019, 10:50 a.m. UTC

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 <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
Signed-off-by: Chris Wilson <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Cc: Matthew Auld <[e-mail]>
Cc: Abdiel Janulgue <[e-mail]>
Cc: CQ Tang <[e-mail]>
---
 Documentation/gpu/i915.rst | 45 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Rodrigo Vivi Sept. 4, 2019, 4:55 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Signed-off-by: Chris Wilson <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: Matthew Auld <[e-mail]>
> Cc: Abdiel Janulgue <[e-mail]>
> Cc: CQ Tang <[e-mail]>
> ---
>  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 <[e-mail]>


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

Joonas Lahtinen Nov. 5, 2019, 11:06 a.m. UTC | #2

Dave, ping for Acked-by here so we can merge? You already gave an
early ack in IRC while travelling.

Regards, Joonas

Quoting Joonas Lahtinen (2019-08-30 13:50:53)
> 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 <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Signed-off-by: Chris Wilson <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: Matthew Auld <[e-mail]>
> Cc: Abdiel Janulgue <[e-mail]>
> Cc: CQ Tang <[e-mail]>
> ---
>  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.
> +
> +#. 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
> +
>  GEM BO Management Implementation Details
>  ----------------------------------------
>  
> -- 
> 2.20.1
>

Dave Airlie April 16, 2020, 7:13 p.m. UTC | #3

Acked-by: Dave Airlie <[e-mail]>

On Tue, 5 Nov 2019 at 21:06, Joonas Lahtinen
<[e-mail]> wrote:
>
> Dave, ping for Acked-by here so we can merge? You already gave an
> early ack in IRC while travelling.
>
> Regards, Joonas
>
> Quoting Joonas Lahtinen (2019-08-30 13:50:53)
> > 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 <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > Signed-off-by: Chris Wilson <[e-mail]>
> > Cc: Dave Airlie <[e-mail]>
> > Cc: Matthew Auld <[e-mail]>
> > Cc: Abdiel Janulgue <[e-mail]>
> > Cc: CQ Tang <[e-mail]>
> > ---
> >  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.
> > +
> > +#. 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
> > +
> >  GEM BO Management Implementation Details
> >  ----------------------------------------
> >
> > --
> > 2.20.1
> >
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Joonas Lahtinen May 14, 2020, 5:21 p.m. UTC | #4

Pushed using the note:: block. Thanks for the review and ack.

Regards, Joonas

Patch

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.
+
+#. 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
+
 GEM BO Management Implementation Details
 ----------------------------------------