[PATCH v2] drm/nouveau: idle the channel before destroying an engine object via abi16

[Alex]

Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx, nouveau@xxxxxxxxxxxxxxxxxxxxx

nouveau_abi16_ioctl_del() destroys an engine object (ENGOBJ) created by
nouveau_abi16_ioctl_new() without waiting for the owning channel to go
idle.  The legacy path, nouveau_abi16_ioctl_gpuobj_free(), does call
nouveau_channel_idle() first ("synchronize with the user channel and
destroy the gpu object"), the new ioctl does not.

Chromium-based applications (Chrome, Yandex Browser, Electron apps)
create and immediately delete probe 3D objects at startup.  If the GPU
is busy with other work at that moment (compositor animation while the
app is launched from the GNOME dash, cursor movement), the engine
context is unbound while the channel still references it, and the GPU
faults:

  nouveau 0000:01:00.0: fifo: fault 00 [VIRT_READ] at 0000000000000000
    engine 00 [GR] client 0f [HUB/CTXCTL] reason 00 [PDE] on channel 5
  nouveau 0000:01:00.0: fifo: ... PAGE_NOT_PRESENT ... channel 5 killed!

after which the desktop freezes.  Observed on GF114 (GeForce GTX 560 Ti)
with Linux 7.0.x, reproducible almost every time by launching a Chromium
app from the dash; not seen in 10 consecutive launches with this patch.

Remember the channel an engine object was created on, drop the reference
when the channel is destroyed first, and idle the channel before
nvif_object_dtor() like the gpuobj_free path does.

Signed-off-by: Alex <xnderv@xxxxxxxxxx>
---
v2: wait for the channel's scheduler to drain before idling the channel
    when VM_BIND is in use.  Legacy PUSHBUF submissions are serialized
    with the ioctl by cli->mutex, but EXEC jobs are pushed to the channel
    asynchronously from the scheduler workqueue, so emitting a fence from
    nouveau_abi16_ioctl_del() could race with nouveau_exec_job_run(), and
    jobs still queued in the entity would not have been covered by the
    fence at all.  Reuse the existing job-list wait from
    nouveau_sched_fini() through a small helper, nouveau_sched_wait_idle().
    (Race pointed out by the Sashiko AI review of v1.)

drivers/gpu/drm/nouveau/nouveau_abi16.c | 27 +++++++++++++++++++++++-
drivers/gpu/drm/nouveau/nouveau_sched.c |  9 +++++++-
drivers/gpu/drm/nouveau/nouveau_sched.h |  1 +
3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c
index b776030..d979cd8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
+++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
@@ -89,6 +89,7 @@ struct nouveau_abi16_obj {
 		ENGOBJ,
 	} type;
 	u64 object;
+	struct nouveau_abi16_chan *chan;
 
 	struct nvif_object engobj;
 
@@ -173,6 +174,13 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16,
 			struct nouveau_abi16_chan *chan)
 {
 	struct nouveau_abi16_ntfy *ntfy, *temp;
+	struct nouveau_abi16_obj *obj;
+
+	/* Forget references to this channel held by engine objects. */
+	list_for_each_entry(obj, &abi16->objects, head) {
+		if (obj->chan == chan)
+			obj->chan = NULL;
+	}
 
 	/* Cancel all jobs from the entity's queue. */
 	if (chan->sched)
@@ -784,8 +792,24 @@ nouveau_abi16_ioctl_del(struct nouveau_abi16 *abi16, struct nvif_ioctl_v0 *ioctl
 
 	obj = nouveau_abi16_obj_find(abi16, ioctl->object);
 	if (obj) {
-		if (obj->type == ENGOBJ)
+		if (obj->type == ENGOBJ) {
+			/* Synchronize with the user channel before destroying the
+			 * engine object, as the legacy GPUOBJ_FREE path does.  The
+			 * channel may still have work queued that needs the engine
+			 * context which is about to be unbound.
+			 *
+			 * Legacy PUSHBUF submissions are serialized with this ioctl
+			 * by cli->mutex.  With VM_BIND, EXEC jobs are pushed to the
+			 * channel asynchronously by the scheduler, so wait for every
+			 * submitted job to finish before touching the channel.
+			 */
+			if (obj->chan && obj->chan->chan) {
+				if (obj->chan->sched)
+					nouveau_sched_wait_idle(obj->chan->sched);
+				nouveau_channel_idle(obj->chan->chan);
+			}
 			nvif_object_dtor(&obj->engobj);
+		}
 		nouveau_abi16_obj_del(obj);
 	}
 
@@ -826,6 +850,7 @@ nouveau_abi16_ioctl_new(struct nouveau_abi16 *abi16, struct nvif_ioctl_v0 *ioctl
 	obj = nouveau_abi16_obj_new(abi16, ENGOBJ, args->object);
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
+	obj->chan = chan;
 
 	ret = nvif_object_ctor(&chan->chan->user, "abi16EngObj", args->handle, args->oclass,
 			       NULL, 0, &obj->engobj);
diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c
index b3f02c4..956ca73 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sched.c
+++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
@@ -493,13 +493,20 @@ nouveau_sched_job_list_empty(struct nouveau_sched *sched)
 	return empty;
 }
 
+/* Wait until every job submitted to the scheduler has finished. */
+void
+nouveau_sched_wait_idle(struct nouveau_sched *sched)
+{
+	wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
+}
+
 static void
 nouveau_sched_fini(struct nouveau_sched *sched)
 {
 	struct drm_gpu_scheduler *drm_sched = &sched->base;
 	struct drm_sched_entity *entity = &sched->entity;
 
-	wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
+	nouveau_sched_wait_idle(sched);
 
 	drm_sched_entity_fini(entity);
 	drm_sched_fini(drm_sched);
diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.h b/drivers/gpu/drm/nouveau/nouveau_sched.h
index 51ce8dc..0562af6 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sched.h
+++ b/drivers/gpu/drm/nouveau/nouveau_sched.h
@@ -115,5 +115,6 @@ struct nouveau_sched {
 int nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
 			 struct workqueue_struct *wq, u32 credit_limit);
 void nouveau_sched_destroy(struct nouveau_sched **psched);
+void nouveau_sched_wait_idle(struct nouveau_sched *sched);
 
 #endif