Re: [Intel-gfx] [PATCH 05/16] drm/i915: Enable i915_gem_wait_for_idle() without holding struct_mutex

[Joonas Lahtinen]

On ma, 2016-08-01 at 20:28 +0100, Chris Wilson wrote:
> On Mon, Aug 01, 2016 at 07:22:27PM +0100, Chris Wilson wrote:
> > 
> > The principal motivation for this was to try and eliminate the
> > struct_mutex from i915_gem_suspend - but we still need to hold the mutex
> > current for the i915_gem_context_lost(). (The issue there is that there
> > may be an indirect lockdep cycle between cpu_hotplug (i.e. suspend) and
> > struct_mutex via the stop_machine().) For the moment, enabling last
> > request tracking for the engine, allows us to do busyness checking and
> > waiting without requiring the struct_mutex - which is useful in its own
> > right.
> Couple of mistakes here: stop-engines tweaking still from when this was
> only aiming at making i915_gem_suspend() lockless (now broken out to a
> separate patc) and more importantly, accessing
> dev_priv->mm.interruptible not under any controlling lock. That takes
> passing around bool interruptible and suddenly we have a bigger patch.
> :|

Not sure what to do with this information, will you send a new
revision?

Regards, Joonas

> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/16] drm/i915: Enable i915_gem_wait_for_idle() without holding struct_mutex

[Chris Wilson]

On Mon, Aug 01, 2016 at 07:22:27PM +0100, Chris Wilson wrote:
> The principal motivation for this was to try and eliminate the
> struct_mutex from i915_gem_suspend - but we still need to hold the mutex
> current for the i915_gem_context_lost(). (The issue there is that there
> may be an indirect lockdep cycle between cpu_hotplug (i.e. suspend) and
> struct_mutex via the stop_machine().) For the moment, enabling last
> request tracking for the engine, allows us to do busyness checking and
> waiting without requiring the struct_mutex - which is useful in its own
> right.

Couple of mistakes here: stop-engines tweaking still from when this was
only aiming at making i915_gem_suspend() lockless (now broken out to a
separate patc) and more importantly, accessing
dev_priv->mm.interruptible not under any controlling lock. That takes
passing around bool interruptible and suddenly we have a bigger patch.
:|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 05/16] drm/i915: Enable i915_gem_wait_for_idle() without holding struct_mutex

[Chris Wilson]

The principal motivation for this was to try and eliminate the
struct_mutex from i915_gem_suspend - but we still need to hold the mutex
current for the i915_gem_context_lost(). (The issue there is that there
may be an indirect lockdep cycle between cpu_hotplug (i.e. suspend) and
struct_mutex via the stop_machine().) For the moment, enabling last
request tracking for the engine, allows us to do busyness checking and
waiting without requiring the struct_mutex - which is useful in its own
right.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem.c | 37 ++---
 drivers/gpu/drm/i915/i915_gem_evict.c   |  2 +-
 drivers/gpu/drm/i915/i915_gem_request.c |  5 +++--
 drivers/gpu/drm/i915/i915_gem_request.h | 11 ++
 drivers/gpu/drm/i915/i915_gpu_error.c   |  2 +-
 drivers/gpu/drm/i915/i915_irq.c |  3 +--
 drivers/gpu/drm/i915/intel_engine_cs.c  |  8 ++-
 drivers/gpu/drm/i915/intel_pm.c |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 16 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.h | 24 +++--
 10 files changed, 52 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f164ad482bdc..8ac9605d5125 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2432,13 +2432,18 @@ static void i915_gem_reset_engine_status(struct 
intel_engine_cs *engine)
 
 static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine)
 {
+   struct drm_i915_gem_request *request;
struct intel_ring *ring;
 
+   request = i915_gem_active_peek(>last_request,
+  >i915->drm.struct_mutex);
+
/* Mark all pending requests as complete so that any concurrent
 * (lockless) lookup doesn't try and wait upon the request as we
 * reset it.
 */
-   intel_engine_init_seqno(engine, engine->last_submitted_seqno);
+   if (request)
+   intel_engine_init_seqno(engine, request->fence.seqno);
 
/*
 * Clear the execlists queue up before freeing the requests, as those
@@ -2460,15 +2465,9 @@ static void i915_gem_reset_engine_cleanup(struct 
intel_engine_cs *engine)
 * implicit references on things like e.g. ppgtt address spaces through
 * the request.
 */
-   if (!list_empty(>request_list)) {
-   struct drm_i915_gem_request *request;
-
-   request = list_last_entry(>request_list,
- struct drm_i915_gem_request,
- link);
-
+   if (request)
i915_gem_request_retire_upto(request);
-   }
+   GEM_BUG_ON(intel_engine_is_active(engine));
 
/* Having flushed all requests from all queues, we know that all
 * ringbuffers must now be empty. However, since we do not reclaim
@@ -2896,8 +2895,6 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
*dev_priv)
struct intel_engine_cs *engine;
int ret;
 
-   lockdep_assert_held(_priv->drm.struct_mutex);
-
for_each_engine(engine, dev_priv) {
if (engine->last_context == NULL)
continue;
@@ -4069,21 +4066,10 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct 
drm_i915_gem_object *obj,
return NULL;
 }
 
-static void
-i915_gem_stop_engines(struct drm_device *dev)
+int i915_gem_suspend(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = to_i915(dev);
-   struct intel_engine_cs *engine;
-
-   for_each_engine(engine, dev_priv)
-   dev_priv->gt.stop_engine(engine);
-}
-
-int
-i915_gem_suspend(struct drm_device *dev)
-{
-   struct drm_i915_private *dev_priv = to_i915(dev);
-   int ret = 0;
+   int ret;
 
intel_suspend_gt_powersave(dev_priv);
 
@@ -4112,7 +4098,6 @@ i915_gem_suspend(struct drm_device *dev)
 * and similar for all logical context images (to ensure they are
 * all ready for hibernation).
 */
-   i915_gem_stop_engines(dev);
i915_gem_context_lost(dev_priv);
mutex_unlock(>struct_mutex);
 
@@ -4128,7 +4113,7 @@ i915_gem_suspend(struct drm_device *dev)
return 0;
 
 err:
-   mutex_unlock(>struct_mutex);
+   mutex_unlock(_priv->drm.struct_mutex);
return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c 
b/drivers/gpu/drm/i915/i915_gem_evict.c
index 7be425826539..624c0f016c84 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -39,7 +39,7 @@ gpu_is_idle(struct drm_i915_private *dev_priv)
struct intel_engine_cs *engine;
 
for_each_engine(engine, dev_priv) {
-   if (!list_empty(>request_list))
+   if (intel_engine_is_active(engine))
return false;
}
 
diff --git