Re: [Intel-gfx] [PATCH 4/8] drm/i915/gt: Guard timeline pinning with its own mutex

2019-08-15 Thread Chris Wilson 
Quoting Matthew Auld (2019-08-15 20:19:58)
> On Wed, 14 Aug 2019 at 10:28, Chris Wilson  wrote:
> >
> > In preparation for removing struct_mutex from around context retirement,
> > we need to make timeline pinning safe. Since multiple engines/contexts
> > can share a single timeline, it needs to be protected by a mutex.
> >
> > Signed-off-by: Chris Wilson 
> 
> With a more accurate commit message,
> Reviewed-by: Matthew Auld 

Cruel.

In preparation for removing struct_mutex from around context retirement,
we need to make timeline pinning and unpinning safe. Since multiple
engines/contexts can share a single timeline, we cannot rely on
borrowing the context mutex (otherwise we could state that the timeline
is only pinned/unpinned inside the context pin/unpin and so guarded by
it). However, we only perform a sequence of atomic operations inside the
timeline pin/unpin and the sequence of those operations is safe for a
concurrent unpin / pin, so we can relax the struct_mutex requirement.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/8] drm/i915/gt: Guard timeline pinning with its own mutex

2019-08-15 Thread Matthew Auld 
On Wed, 14 Aug 2019 at 10:28, Chris Wilson  wrote:
>
> In preparation for removing struct_mutex from around context retirement,
> we need to make timeline pinning safe. Since multiple engines/contexts
> can share a single timeline, it needs to be protected by a mutex.
>
> Signed-off-by: Chris Wilson 

With a more accurate commit message,
Reviewed-by: Matthew Auld 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/8] drm/i915/gt: Guard timeline pinning with its own mutex

2019-08-14 Thread Chris Wilson 
In preparation for removing struct_mutex from around context retirement,
we need to make timeline pinning safe. Since multiple engines/contexts
can share a single timeline, it needs to be protected by a mutex.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/gt/intel_timeline.c  | 27 +--
 .../gpu/drm/i915/gt/intel_timeline_types.h|  2 +-
 drivers/gpu/drm/i915/gt/mock_engine.c |  6 ++---
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c 
b/drivers/gpu/drm/i915/gt/intel_timeline.c
index 355dfc52c804..7b476cd55dac 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline.c
+++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
@@ -211,9 +211,9 @@ int intel_timeline_init(struct intel_timeline *timeline,
void *vaddr;
 
kref_init(>kref);
+   atomic_set(>pin_count, 0);
 
timeline->gt = gt;
-   timeline->pin_count = 0;
 
timeline->has_initial_breadcrumb = !hwsp;
timeline->hwsp_cacheline = NULL;
@@ -280,7 +280,7 @@ void intel_timelines_init(struct drm_i915_private *i915)
 
 void intel_timeline_fini(struct intel_timeline *timeline)
 {
-   GEM_BUG_ON(timeline->pin_count);
+   GEM_BUG_ON(atomic_read(>pin_count));
GEM_BUG_ON(!list_empty(>requests));
 
if (timeline->hwsp_cacheline)
@@ -314,33 +314,31 @@ int intel_timeline_pin(struct intel_timeline *tl)
 {
int err;
 
-   if (tl->pin_count++)
+   if (atomic_add_unless(>pin_count, 1, 0))
return 0;
-   GEM_BUG_ON(!tl->pin_count);
-   GEM_BUG_ON(tl->active_count);
 
err = i915_vma_pin(tl->hwsp_ggtt, 0, 0, PIN_GLOBAL | PIN_HIGH);
if (err)
-   goto unpin;
+   return err;
 
tl->hwsp_offset =
i915_ggtt_offset(tl->hwsp_ggtt) +
offset_in_page(tl->hwsp_offset);
 
cacheline_acquire(tl->hwsp_cacheline);
+   if (atomic_fetch_inc(>pin_count)) {
+   cacheline_release(tl->hwsp_cacheline);
+   __i915_vma_unpin(tl->hwsp_ggtt);
+   }
 
return 0;
-
-unpin:
-   tl->pin_count = 0;
-   return err;
 }
 
 void intel_timeline_enter(struct intel_timeline *tl)
 {
struct intel_gt_timelines *timelines = >gt->timelines;
 
-   GEM_BUG_ON(!tl->pin_count);
+   GEM_BUG_ON(!atomic_read(>pin_count));
if (tl->active_count++)
return;
GEM_BUG_ON(!tl->active_count); /* overflow? */
@@ -372,7 +370,7 @@ void intel_timeline_exit(struct intel_timeline *tl)
 
 static u32 timeline_advance(struct intel_timeline *tl)
 {
-   GEM_BUG_ON(!tl->pin_count);
+   GEM_BUG_ON(!atomic_read(>pin_count));
GEM_BUG_ON(tl->seqno & tl->has_initial_breadcrumb);
 
return tl->seqno += 1 + tl->has_initial_breadcrumb;
@@ -523,11 +521,10 @@ int intel_timeline_read_hwsp(struct i915_request *from,
 
 void intel_timeline_unpin(struct intel_timeline *tl)
 {
-   GEM_BUG_ON(!tl->pin_count);
-   if (--tl->pin_count)
+   GEM_BUG_ON(!atomic_read(>pin_count));
+   if (!atomic_dec_and_test(>pin_count))
return;
 
-   GEM_BUG_ON(tl->active_count);
cacheline_release(tl->hwsp_cacheline);
 
__i915_vma_unpin(tl->hwsp_ggtt);
diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h 
b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
index b1a9f0c54bf0..2b1baf2fcc8e 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
@@ -41,7 +41,7 @@ struct intel_timeline {
 * but the pin_count is protected by a combination of serialisation
 * from the intel_context caller plus internal atomicity.
 */
-   unsigned int pin_count;
+   atomic_t pin_count;
unsigned int active_count;
 
const u32 *hwsp_seqno;
diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c 
b/drivers/gpu/drm/i915/gt/mock_engine.c
index a63dd8a42cd4..54a11dde3076 100644
--- a/drivers/gpu/drm/i915/gt/mock_engine.c
+++ b/drivers/gpu/drm/i915/gt/mock_engine.c
@@ -34,13 +34,13 @@
 
 static void mock_timeline_pin(struct intel_timeline *tl)
 {
-   tl->pin_count++;
+   atomic_inc(>pin_count);
 }
 
 static void mock_timeline_unpin(struct intel_timeline *tl)
 {
-   GEM_BUG_ON(!tl->pin_count);
-   tl->pin_count--;
+   GEM_BUG_ON(!atomic_read(>pin_count));
+   atomic_dec(>pin_count);
 }
 
 static struct intel_ring *mock_ring(struct intel_engine_cs *engine)
-- 
2.23.0.rc1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx