From: Francisco Jerez Date: Mon, 18 Oct 2010 01:53:39 +0000 (+0200) Subject: drm/nouveau: Refactor context destruction to avoid a lock ordering issue. X-Git-Tag: v2.6.38-rc1~419^2~37^2~78 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=3945e47543863385b54d94c94b023ee7ca9df972;p=platform%2Fupstream%2Fkernel-adaptation-pc.git 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 Signed-off-by: Francisco Jerez Signed-off-by: Ben Skeggs --- diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c index 0e2414b..9a051fa 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 699d546..198dabe 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 513c106..1a4ba6c 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 25c439d..4c0d3a8 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 98b9525..1e2ad39 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 39328fc..912556f 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 cd931b5..e3a87a6 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 12ab9cd..8a04020 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 3c7be3d..311ac9e 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 e0b41a2..70d97cd 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 815960f..d3295aa 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 24a3f84..dcc9175 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); }