Re: DRM/KMS: vmwgfx driver is now available

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Sun, 23 Jul 2023 22:57:14 +0900
> From: PHO <pho%cielonegro.org@localhost>
> 
> On 7/23/23 21:24, Taylor R Campbell wrote:
> > Why can't the loop just use dma_fence_get_rcu and dma_fence_put?
> > 
> > Might need to release and reacquire the lock around dma_fence_put.
> 
> I tried but it didn't work, apparently because someone was holding a 
> pointer to a dying fence and waiting for it to be signaled. Using 
> dma_fence_get_rcu() meant we didn't signal such fences, and the kernel 
> hanged and eventually got killed by heartbeat().

What exactly did you try and what was the heartbeat panic stack trace?
Nothing here should wait for signalling in a way that trips the kernel
heartbeat monitor -- that should only happen if you hold onto a spin
lock for too long.

(That said: yay, the heartbeat monitor is catching what would
otherwise be a silent nonresponsive hang!)

Thinking about it some more, I see there is a snag with the loop and
dma_fence_put, since

(a) we can only use list_next(&fence->head) if we read it and then use
it under the lock, but

(b) we can only dma_fence_put outside the lock because the release
callback, vmw_fence_obj_destroy, takes the lock,

so there's no way to do the iteration safely as is with an extra
reference count acquire/release cycle.  Two options come to mind:

1. Move vmw_fence_obj_destroy to an `RCU' callback so that we can
   safely do dma_fence_put under the lock.

2. Create a list on the stack of signalled fences to put in
   __vmw_fences_update and put them after the loop, outside the lock:

	struct list_head to_put = LIST_HEAD_INIT(to_put);
...
	list_for_each_entry_safe(fence, next_fence, &fman->fence_list, head) {
		if (dma_fence_get_rcu(fence) != 0) {		/* (*) */
			list_del_init(&fence->head);
			continue;
		}
		if (seqno - fence->base.seqno < VMW_FENCE_WRAP) {
			list_del_init(&fence->head);
			list_add(&fence->head, &to_put); 	/* (*) */
			dma_fence_signal_locked(&fence->base);
			...
		}
			break;
	}
...
	if (fence != NULL) {
		spin_unlock(&fman->lock);
		dma_fence_put(fence);
		spin_lock(&fman->lock);
	}
	while ((fence = list_first(&to_put)) != NULL) {
		list_del_init(&fence->head);
		spin_unlock(&fman->lock);
		dma_fence_put(fence);
		spin_lock(&fman->lock);
	}

> > If that doesn't work for some reason and we do have to add a special
> > case, I'd strongly prefer to add a new front end like
> > dma_fence_signal_vmwgfxspecialhack or something (other examples in the
> > tasklet API) that has the relaxed assertion instead of relaxing the
> > assertions for every other driver which isn't bonkers.
> 
> Alright. I'll do that.

This should be a last resort (but not as bad as relaxing the
assertions) if we can't figure out a better way to do it.

I'm not sure the patch above is going to be best (if it works), but
I'd like to see if there's a way to do this without abusing the API
semantics too much before giving in to the abuse.