On Thu, 2026-08-13 at 01:13 +0200, Marek Czernohous wrote:
>
> Move the subscription behind context_new() so the handler cannot
> observe
> a half-built fence context. The failure path is unchanged in effect:
> the
> caller drops the channel with nouveau_channel_del() either way, which
> since the previous patch unsubscribes before freeing the context.
>
> One behavioural change worth naming, and it is not free: a kill
> delivered while the channel is still initialising is no longer
> observed,
> because the subscription is not armed yet. That window does not close
> here, it moves, and on Fermi and newer it grows by the span between
> the
> old subscription point and context_new(). What changes is what the
> window costs. Before, a kill landing in it reached a half-built fence
> context; now it is missed, and the channel is left blocked with
> chan->killed still 0, so nouveau_channel_idle() and the checks in
> nouveau_gem_ioctl_pushbuf() and nouveau_exec_ioctl_exec() keep
> treating
> it as alive. The missed-kill window is not introduced by this patch
> either:
> nvkm_uchan_init() already calls nvkm_chan_allow() and
> nvkm_chan_insert(),
> so the channel is schedulable before nouveau_channel_init()
> subscribes at
> all. Closing it properly means subscribing before the channel becomes
> schedulable, which is a bigger change than this fix.
I don't think this would be a big change, actually. If I understand
this code properly, we don't need to move around the channel event init
at all. Maybe:
* Add another atomic to `nouveau_fence` to indicate if the fence is
ready
* Have `nouveau_channel_kill()` check this atomic after setting the
killed bit, and only call `nouveau_fence_context_kill()` in the
event the atomic is set.
* Back in `nouveau_channel_init()`, after we initialize the rest of
the channel (we still haven't set the atomic yet) grab the fence
context lock to delay, but not block, any incoming kills.
* Check the killed atomic to see if a kill event came in the window
before the fence was ready.
* Set the atomic indicating that the fence is ready.
* Drop the lock.
* If we found that the killed atomic had been set while holding the
lock, call nouveau_fence_context_kill() manually (or queue up a call
to it if that works better with a workqueue perhaps?).
Hopefully I didn't miss anything that would make my idea not make
sense. And consider trying to write the respin without using claude!
You will learn more, the work I've done is the only reason I was able
to think of a solution like that only a few minutes after reading the
code here.
Also - it might not need to be an atomic, I'd have to look a bit more
closely to know though.
>
> As with the previous patch this is unreachable below Fermi today, and
> the last patch in this series lowers the gate to NV50.
>
> Fixes: ea13e5abf807 ("drm/nouveau: signal pending fences when channel
> has been killed")
> Cc: [email protected]
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Marek Czernohous <[email protected]>
> ---
> drivers/gpu/drm/nouveau/nouveau_chan.c | 55 +++++++++++++++---------
> --
> 1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c
> b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index f142f6310596..07b0bd1bc519 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -370,27 +370,6 @@ nouveau_channel_init(struct nouveau_channel
> *chan, u32 vram, u32 gart)
> if (ret)
> return ret;
>
> - if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) {
> - DEFINE_RAW_FLEX(struct nvif_event_v0, args, data,
> - sizeof(struct nvif_chan_event_v0));
> - struct nvif_chan_event_v0 *host =
> - (struct nvif_chan_event_v0 *)args->data;
> -
> - host->version = 0;
> - host->type = NVIF_CHAN_EVENT_V0_KILLED;
> -
> - ret = nvif_event_ctor(&chan->user, "abi16ChanKilled", chan->chid,
> - nouveau_channel_killed, false,
> - args, __struct_size(args), &chan->kill);
> - if (ret == 0)
> - ret = nvif_event_allow(&chan->kill);
> - if (ret) {
> - NV_ERROR(drm, "Failed to request channel kill "
> - "notification: %d\n", ret);
> - return ret;
> - }
> - }
> -
> /* allocate dma objects to cover all allowed vram, and gart */
> if (device->info.family < NV_DEVICE_INFO_V0_FERMI) {
> if (device->info.family >= NV_DEVICE_INFO_V0_TESLA) {
> @@ -494,7 +473,39 @@ nouveau_channel_init(struct nouveau_channel
> *chan, u32 vram, u32 gart)
> }
>
> /* initialise synchronisation */
> - return nouveau_fence(drm)->context_new(chan);
> + ret = nouveau_fence(drm)->context_new(chan);
> + if (ret)
> + return ret;
> +
> + /*
> + * Subscribe to the channel-kill event last. The handler
> + * dereferences chan->fence, and the fence context is only complete
> + * once context_new() has returned: the backends assign chan->fence
> + * from kzalloc() before nouveau_fence_context_new() initialises the
> + * lock and the pending list, so an event arriving in between would
> + * find a non-NULL but unusable context and walk a NULL list head.
> + */
> + if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) {
> + DEFINE_RAW_FLEX(struct nvif_event_v0, args, data,
> + sizeof(struct nvif_chan_event_v0));
> + struct nvif_chan_event_v0 *host =
> + (struct nvif_chan_event_v0 *)args->data;
> +
> + host->version = 0;
> + host->type = NVIF_CHAN_EVENT_V0_KILLED;
> +
> + ret = nvif_event_ctor(&chan->user, "abi16ChanKilled", chan->chid,
> + nouveau_channel_killed, false,
> + args, __struct_size(args), &chan->kill);
> + if (ret == 0)
> + ret = nvif_event_allow(&chan->kill);
> + if (ret) {
> + NV_ERROR(drm, "Failed to request channel kill notification: %d\n",
> ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> }
>
> int
