[Intel-gfx] [RFC] drm/i915: Check if we hold a wakeref during ioread32/iowrite32

[Chris Wilson]

On Tue, Feb 23, 2016 at 05:09:29PM +0200, Imre Deak wrote:
> On ti, 2016-02-23 at 14:55 +0000, Chris Wilson wrote:
> > On Tue, Feb 23, 2016 at 04:47:17PM +0200, Imre Deak wrote:
> > > The device needs to be in D0 state whenever we call these
> > > functions, so
> > > add the corresponding assert checks.
> > 
> > No. In quite a few of those cases we are calling iowrite to non-
> > device
> > memory, even normal pages.
> 
> Hm right didn't think about that. I guess the only case we care about
> then are accesses through the GTT.
> 
> > How's the separation of struct_mutex from rpm going so that we can
> > forgo
> > adding assertions and use explicit power management instead?
> 
> It's planned to be done, but no one is working on that yet. This is
> something we could still have regardless, similarly to other helpers
> accessing the device.

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 31b600d31158..b8687b6a6acb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1430,24 +1430,6 @@ static int intel_runtime_suspend(struct device *device)
 
        DRM_DEBUG_KMS("Suspending device\n");
 
-       /*
-        * We could deadlock here in case another thread holding struct_mutex
-        * calls RPM suspend concurrently, since the RPM suspend will wait
-        * first for this RPM suspend to finish. In this case the concurrent
-        * RPM resume will be followed by its RPM suspend counterpart. Still
-        * for consistency return -EAGAIN, which will reschedule this suspend.
-        */
-       if (!mutex_trylock(&dev->struct_mutex)) {
-               DRM_DEBUG_KMS("device lock contention, deffering suspend\n");
-               /*
-                * Bump the expiration timestamp, otherwise the suspend won't
-                * be rescheduled.
-                */
-               pm_runtime_mark_last_busy(device);
-
-               return -EAGAIN;
-       }
-
        disable_rpm_wakeref_asserts(dev_priv);
 
        /*
@@ -1455,7 +1437,6 @@ static int intel_runtime_suspend(struct device *device)
         * an RPM reference.
         */
        i915_gem_release_all_mmaps(dev_priv);
-       mutex_unlock(&dev->struct_mutex);
 
        intel_guc_suspend(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 79706621e6e4..4f6127466822 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1670,9 +1670,13 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
        /* Serialisation between user GTT access and our code depends upon
         * revoking the CPU's PTE whilst the mutex is held. The next user
         * pagefault then has to wait until we release the mutex.
+        *
+        * Note that RPM complicates somewhat by adding an additional
+        * requirement that operations to the GGTT be made holding the RPM
+        * wakeref. This in turns allow us to release the mmap from within
+        * the RPM suspend code ignoring the struct_mutex serialisation in
+        * lieu of the RPM barriers.
         */
-       lockdep_assert_held(&obj->base.dev->struct_mutex);
-
        if (!obj->fault_mappable)
                return;
 
@@ -1685,11 +1689,21 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
        obj->fault_mappable = false;
 }
 
+static void assert_rpm_release_all_mmaps(struct drm_i915_private *dev_priv)
+{
+       assert_rpm_wakelock_held(dev_priv);
+}
+
 void
 i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
 {
        struct drm_i915_gem_object *obj;
 
+       /* This should only be called by RPM as we require the bound_list
+        * to be protected by the RPM barriers and not struct_mutex.
+        * We check that we are holding the wakeref whenever we manipulate
+        * the dev_priv->mm.bound_list (via assert_rpm_release_all_mmaps).
+        */
        list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
                i915_gem_release_mmap(obj);
 }
@@ -2224,9 +2238,11 @@ i915_gem_object_retire__read(struct i915_gem_active *active,
         * so that we don't steal from recently used but inactive objects
         * (unless we are forced to ofc!)
         */
-       if (obj->bind_count)
+       if (obj->bind_count) {
+               assert_rpm_release_all_mmaps(request->i915);
                list_move_tail(&obj->global_list,
                               &request->i915->mm.bound_list);
+       }
 
        if (i915_gem_object_has_active_reference(obj)) {
                i915_gem_object_unset_active_reference(obj);
@@ -2751,9 +2767,11 @@ int i915_vma_unbind(struct i915_vma *vma)
 
        /* Since the unbound list is global, only move to that list if
         * no more VMAs exist. */
-       if (--obj->bind_count == 0)
+       if (--obj->bind_count == 0) {
+               assert_rpm_release_all_mmaps(to_i915(obj->base.dev));
                list_move_tail(&obj->global_list,
                               &to_i915(obj->base.dev)->mm.unbound_list);
+       }
 
        /* And finally now the object is completely decoupled from this vma,
         * we can drop its hold on the backing storage and allow it to be
@@ -2913,6 +2931,7 @@ i915_vma_insert(struct i915_vma *vma,
                goto err_remove_node;
        }
 
+       assert_rpm_release_all_mmaps(dev_priv);
        list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
        list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
        obj->bind_count++;


-- 
Chris Wilson, Intel Open Source Technology Centre