drm/nouveau: Refactor context destruction to avoid a lock ordering issue.

The destroy_context() engine hooks call gpuobj management functions to
release the channel resources, these functions use HARDIRQ-unsafe locks
whereas destroy_context() is called with the HARDIRQ-safe
context_switch_lock held, that's a lock ordering violation.

Push the engine-specific channel destruction logic into destroy_context()
and let the hardware-specific code lock and unlock when it's actually
needed. Change the engine destruction order to avoid a race in the small
gap between pgraph and pfifo context uninitialization.

Reported-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Signed-off-by: Francisco Jerez <currojerez@riseup.net>
Signed-off-by: Ben Skeggs <bskeggs@redhat.com>

12 files changed, 116 insertions, 49 deletions

diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c
index 0e2414b0ae5..9a051fafa7c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_channel.c
+++ b/drivers/gpu/drm/nouveau/nouveau_channel.c
@@ -313,32 +313,20 @@ nouveau_channel_put(struct nouveau_channel **pchan)
         /* boot it off the hardware */
         pfifo->reassign(dev, false);
 
-        /* We want to give pgraph a chance to idle and get rid of all potential
-         * errors. We need to do this before the lock, otherwise the irq handler
-         * is unable to process them.
+        /* We want to give pgraph a chance to idle and get rid of all
+         * potential errors. We need to do this without the context
+         * switch lock held, otherwise the irq handler is unable to
+         * process them.
          */
         if (pgraph->channel(dev) == chan)
                 nouveau_wait_for_idle(dev);
 
-        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
-
-        pgraph->fifo_access(dev, false);
-        if (pgraph->channel(dev) == chan)
-                pgraph->unload_context(dev);
-        pgraph->destroy_context(chan);
-        pgraph->fifo_access(dev, true);
-
-        if (pfifo->channel_id(dev) == chan->id) {
-                pfifo->disable(dev);
-                pfifo->unload_context(dev);
-                pfifo->enable(dev);
-        }
+        /* destroy the engine specific contexts */
         pfifo->destroy_context(chan);
+        pgraph->destroy_context(chan);
 
         pfifo->reassign(dev, true);
 
-        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
-
         /* aside from its resources, the channel should now be dead,
          * remove it from the channel list
          */
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 699d546623b..198dabebafb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -998,14 +998,12 @@ extern int  nv04_fifo_unload_context(struct drm_device *);
 extern int  nv10_fifo_init(struct drm_device *);
 extern int  nv10_fifo_channel_id(struct drm_device *);
 extern int  nv10_fifo_create_context(struct nouveau_channel *);
-extern void nv10_fifo_destroy_context(struct nouveau_channel *);
 extern int  nv10_fifo_load_context(struct nouveau_channel *);
 extern int  nv10_fifo_unload_context(struct drm_device *);
 
 /* nv40_fifo.c */
 extern int  nv40_fifo_init(struct drm_device *);
 extern int  nv40_fifo_create_context(struct nouveau_channel *);
-extern void nv40_fifo_destroy_context(struct nouveau_channel *);
 extern int  nv40_fifo_load_context(struct nouveau_channel *);
 extern int  nv40_fifo_unload_context(struct drm_device *);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c
index 513c1063fb5..1a4ba6ccafb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -137,7 +137,7 @@ static int nouveau_init_engine_ptrs(struct drm_device *dev)
                 engine->fifo.cache_pull         = nv04_fifo_cache_pull;
                 engine->fifo.channel_id         = nv10_fifo_channel_id;
                 engine->fifo.create_context     = nv10_fifo_create_context;
-                engine->fifo.destroy_context    = nv10_fifo_destroy_context;
+                engine->fifo.destroy_context    = nv04_fifo_destroy_context;
                 engine->fifo.load_context       = nv10_fifo_load_context;
                 engine->fifo.unload_context     = nv10_fifo_unload_context;
                 engine->display.early_init      = nv04_display_early_init;
@@ -191,7 +191,7 @@ static int nouveau_init_engine_ptrs(struct drm_device *dev)
                 engine->fifo.cache_pull         = nv04_fifo_cache_pull;
                 engine->fifo.channel_id         = nv10_fifo_channel_id;
                 engine->fifo.create_context     = nv10_fifo_create_context;
-                engine->fifo.destroy_context    = nv10_fifo_destroy_context;
+                engine->fifo.destroy_context    = nv04_fifo_destroy_context;
                 engine->fifo.load_context       = nv10_fifo_load_context;
                 engine->fifo.unload_context     = nv10_fifo_unload_context;
                 engine->display.early_init      = nv04_display_early_init;
@@ -245,7 +245,7 @@ static int nouveau_init_engine_ptrs(struct drm_device *dev)
                 engine->fifo.cache_pull         = nv04_fifo_cache_pull;
                 engine->fifo.channel_id         = nv10_fifo_channel_id;
                 engine->fifo.create_context     = nv10_fifo_create_context;
-                engine->fifo.destroy_context    = nv10_fifo_destroy_context;
+                engine->fifo.destroy_context    = nv04_fifo_destroy_context;
                 engine->fifo.load_context       = nv10_fifo_load_context;
                 engine->fifo.unload_context     = nv10_fifo_unload_context;
                 engine->display.early_init      = nv04_display_early_init;
@@ -302,7 +302,7 @@ static int nouveau_init_engine_ptrs(struct drm_device *dev)
                 engine->fifo.cache_pull         = nv04_fifo_cache_pull;
                 engine->fifo.channel_id         = nv10_fifo_channel_id;
                 engine->fifo.create_context     = nv40_fifo_create_context;
-                engine->fifo.destroy_context    = nv40_fifo_destroy_context;
+                engine->fifo.destroy_context    = nv04_fifo_destroy_context;
                 engine->fifo.load_context       = nv40_fifo_load_context;
                 engine->fifo.unload_context     = nv40_fifo_unload_context;
                 engine->display.early_init      = nv04_display_early_init;
diff --git a/drivers/gpu/drm/nouveau/nv04_fifo.c b/drivers/gpu/drm/nouveau/nv04_fifo.c
index 25c439dcdfd..4c0d3a8fca6 100644
--- a/drivers/gpu/drm/nouveau/nv04_fifo.c
+++ b/drivers/gpu/drm/nouveau/nv04_fifo.c
@@ -151,10 +151,27 @@ void
 nv04_fifo_destroy_context(struct nouveau_channel *chan)
 {
         struct drm_device *dev = chan->dev;
+        struct drm_nouveau_private *dev_priv = dev->dev_private;
+        struct nouveau_fifo_engine *pfifo = &dev_priv->engine.fifo;
+        unsigned long flags;
 
-        nv_wr32(dev, NV04_PFIFO_MODE,
-                nv_rd32(dev, NV04_PFIFO_MODE) & ~(1 << chan->id));
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pfifo->reassign(dev, false);
+
+        /* Unload the context if it's the currently active one */
+        if (pfifo->channel_id(dev) == chan->id) {
+                pfifo->disable(dev);
+                pfifo->unload_context(dev);
+                pfifo->enable(dev);
+        }
+
+        /* Keep it from being rescheduled */
+        nv_mask(dev, NV04_PFIFO_MODE, 1 << chan->id, 0);
+
+        pfifo->reassign(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
 
+        /* Free the channel resources */
         nouveau_gpuobj_ref(NULL, &chan->ramfc);
 }
 
diff --git a/drivers/gpu/drm/nouveau/nv04_graph.c b/drivers/gpu/drm/nouveau/nv04_graph.c
index 98b9525c1eb..1e2ad394233 100644
--- a/drivers/gpu/drm/nouveau/nv04_graph.c
+++ b/drivers/gpu/drm/nouveau/nv04_graph.c
@@ -412,10 +412,25 @@ int nv04_graph_create_context(struct nouveau_channel *chan)
 
 void nv04_graph_destroy_context(struct nouveau_channel *chan)
 {
+        struct drm_device *dev = chan->dev;
+        struct drm_nouveau_private *dev_priv = dev->dev_private;
+        struct nouveau_pgraph_engine *pgraph = &dev_priv->engine.graph;
         struct graph_state *pgraph_ctx = chan->pgraph_ctx;
+        unsigned long flags;
+
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pgraph->fifo_access(dev, false);
+
+        /* Unload the context if it's the currently active one */
+        if (pgraph->channel(dev) == chan)
+                pgraph->unload_context(dev);
 
+        /* Free the context resources */
         kfree(pgraph_ctx);
         chan->pgraph_ctx = NULL;
+
+        pgraph->fifo_access(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
 }
 
 int nv04_graph_load_context(struct nouveau_channel *chan)
diff --git a/drivers/gpu/drm/nouveau/nv10_fifo.c b/drivers/gpu/drm/nouveau/nv10_fifo.c
index 39328fcce70..912556f2e33 100644
--- a/drivers/gpu/drm/nouveau/nv10_fifo.c
+++ b/drivers/gpu/drm/nouveau/nv10_fifo.c
@@ -73,17 +73,6 @@ nv10_fifo_create_context(struct nouveau_channel *chan)
         return 0;
 }
 
-void
-nv10_fifo_destroy_context(struct nouveau_channel *chan)
-{
-        struct drm_device *dev = chan->dev;
-
-        nv_wr32(dev, NV04_PFIFO_MODE,
-                        nv_rd32(dev, NV04_PFIFO_MODE) & ~(1 << chan->id));
-
-        nouveau_gpuobj_ref(NULL, &chan->ramfc);
-}
-
 static void
 nv10_fifo_do_load_context(struct drm_device *dev, int chid)
 {
diff --git a/drivers/gpu/drm/nouveau/nv10_graph.c b/drivers/gpu/drm/nouveau/nv10_graph.c
index cd931b57cf0..e3a87a64c16 100644
--- a/drivers/gpu/drm/nouveau/nv10_graph.c
+++ b/drivers/gpu/drm/nouveau/nv10_graph.c
@@ -875,10 +875,25 @@ int nv10_graph_create_context(struct nouveau_channel *chan)
 
 void nv10_graph_destroy_context(struct nouveau_channel *chan)
 {
+        struct drm_device *dev = chan->dev;
+        struct drm_nouveau_private *dev_priv = dev->dev_private;
+        struct nouveau_pgraph_engine *pgraph = &dev_priv->engine.graph;
         struct graph_state *pgraph_ctx = chan->pgraph_ctx;
+        unsigned long flags;
+
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pgraph->fifo_access(dev, false);
+
+        /* Unload the context if it's the currently active one */
+        if (pgraph->channel(dev) == chan)
+                pgraph->unload_context(dev);
 
+        /* Free the context resources */
         kfree(pgraph_ctx);
         chan->pgraph_ctx = NULL;
+
+        pgraph->fifo_access(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
 }
 
 void
diff --git a/drivers/gpu/drm/nouveau/nv20_graph.c b/drivers/gpu/drm/nouveau/nv20_graph.c
index 12ab9cd56ec..8a040201255 100644
--- a/drivers/gpu/drm/nouveau/nv20_graph.c
+++ b/drivers/gpu/drm/nouveau/nv20_graph.c
@@ -425,9 +425,21 @@ nv20_graph_destroy_context(struct nouveau_channel *chan)
         struct drm_device *dev = chan->dev;
         struct drm_nouveau_private *dev_priv = dev->dev_private;
         struct nouveau_pgraph_engine *pgraph = &dev_priv->engine.graph;
+        unsigned long flags;
 
-        nouveau_gpuobj_ref(NULL, &chan->ramin_grctx);
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pgraph->fifo_access(dev, false);
+
+        /* Unload the context if it's the currently active one */
+        if (pgraph->channel(dev) == chan)
+                pgraph->unload_context(dev);
+
+        pgraph->fifo_access(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
+
+        /* Free the context resources */
         nv_wo32(pgraph->ctx_table, chan->id * 4, 0);
+        nouveau_gpuobj_ref(NULL, &chan->ramin_grctx);
 }
 
 int
diff --git a/drivers/gpu/drm/nouveau/nv40_fifo.c b/drivers/gpu/drm/nouveau/nv40_fifo.c
index 3c7be3dc8b8..311ac9ea5d5 100644
--- a/drivers/gpu/drm/nouveau/nv40_fifo.c
+++ b/drivers/gpu/drm/nouveau/nv40_fifo.c
@@ -70,17 +70,6 @@ nv40_fifo_create_context(struct nouveau_channel *chan)
         return 0;
 }
 
-void
-nv40_fifo_destroy_context(struct nouveau_channel *chan)
-{
-        struct drm_device *dev = chan->dev;
-
-        nv_wr32(dev, NV04_PFIFO_MODE,
-                nv_rd32(dev, NV04_PFIFO_MODE) & ~(1 << chan->id));
-
-        nouveau_gpuobj_ref(NULL, &chan->ramfc);
-}
-
 static void
 nv40_fifo_do_load_context(struct drm_device *dev, int chid)
 {
diff --git a/drivers/gpu/drm/nouveau/nv40_graph.c b/drivers/gpu/drm/nouveau/nv40_graph.c
index e0b41a26447..70d97cde49d 100644
--- a/drivers/gpu/drm/nouveau/nv40_graph.c
+++ b/drivers/gpu/drm/nouveau/nv40_graph.c
@@ -79,6 +79,22 @@ nv40_graph_create_context(struct nouveau_channel *chan)
 void
 nv40_graph_destroy_context(struct nouveau_channel *chan)
 {
+        struct drm_device *dev = chan->dev;
+        struct drm_nouveau_private *dev_priv = dev->dev_private;
+        struct nouveau_pgraph_engine *pgraph = &dev_priv->engine.graph;
+        unsigned long flags;
+
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pgraph->fifo_access(dev, false);
+
+        /* Unload the context if it's the currently active one */
+        if (pgraph->channel(dev) == chan)
+                pgraph->unload_context(dev);
+
+        pgraph->fifo_access(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
+
+        /* Free the context resources */
         nouveau_gpuobj_ref(NULL, &chan->ramin_grctx);
 }
 
diff --git a/drivers/gpu/drm/nouveau/nv50_fifo.c b/drivers/gpu/drm/nouveau/nv50_fifo.c
index 815960fe4f4..d3295aae0c4 100644
--- a/drivers/gpu/drm/nouveau/nv50_fifo.c
+++ b/drivers/gpu/drm/nouveau/nv50_fifo.c
@@ -292,10 +292,23 @@ void
 nv50_fifo_destroy_context(struct nouveau_channel *chan)
 {
         struct drm_device *dev = chan->dev;
+        struct drm_nouveau_private *dev_priv = dev->dev_private;
+        struct nouveau_fifo_engine *pfifo = &dev_priv->engine.fifo;
         struct nouveau_gpuobj *ramfc = NULL;
+        unsigned long flags;
 
         NV_DEBUG(dev, "ch%d\n", chan->id);
 
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pfifo->reassign(dev, false);
+
+        /* Unload the context if it's the currently active one */
+        if (pfifo->channel_id(dev) == chan->id) {
+                pfifo->disable(dev);
+                pfifo->unload_context(dev);
+                pfifo->enable(dev);
+        }
+
         /* This will ensure the channel is seen as disabled. */
         nouveau_gpuobj_ref(chan->ramfc, &ramfc);
         nouveau_gpuobj_ref(NULL, &chan->ramfc);
@@ -306,6 +319,10 @@ nv50_fifo_destroy_context(struct nouveau_channel *chan)
                 nv50_fifo_channel_disable(dev, 127);
         nv50_fifo_playlist_update(dev);
 
+        pfifo->reassign(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
+
+        /* Free the channel resources */
         nouveau_gpuobj_ref(NULL, &ramfc);
         nouveau_gpuobj_ref(NULL, &chan->cache);
 }
diff --git a/drivers/gpu/drm/nouveau/nv50_graph.c b/drivers/gpu/drm/nouveau/nv50_graph.c
index 24a3f848757..dcc9175fb79 100644
--- a/drivers/gpu/drm/nouveau/nv50_graph.c
+++ b/drivers/gpu/drm/nouveau/nv50_graph.c
@@ -242,17 +242,28 @@ nv50_graph_destroy_context(struct nouveau_channel *chan)
 {
         struct drm_device *dev = chan->dev;
         struct drm_nouveau_private *dev_priv = dev->dev_private;
+        struct nouveau_pgraph_engine *pgraph = &dev_priv->engine.graph;
         int i, hdr = (dev_priv->chipset == 0x50) ? 0x200 : 0x20;
+        unsigned long flags;
 
         NV_DEBUG(dev, "ch%d\n", chan->id);
 
         if (!chan->ramin)
                 return;
 
+        spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
+        pgraph->fifo_access(dev, false);
+
+        if (pgraph->channel(dev) == chan)
+                pgraph->unload_context(dev);
+
         for (i = hdr; i < hdr + 24; i += 4)
                 nv_wo32(chan->ramin, i, 0);
         dev_priv->engine.instmem.flush(dev);
 
+        pgraph->fifo_access(dev, true);
+        spin_unlock_irqrestore(&dev_priv->context_switch_lock, flags);
+
         nouveau_gpuobj_ref(NULL, &chan->ramin_grctx);
 }