Nouveau - Mar 2013 - mesa vdpau regression with "dri2: Fix potential race and crash for swap at next vblank."

Hey Mario,

It seems that your ddx commit b4231dd715a8 is causing a regression when I use
mplayer -vo vdpau rendering with mesa 9.1.
It fails to start drawing here, leaving the mplayer screen black, I can make it
recover usually by seeking or moving another
window in front. When I revert the commit it behaves normally. Changing
can_sync_to_vblank(draw) to 0 in
nouveau_dri2_schedule_swap also fixes it.

The nouveau kernel driver currently doesn't fill in a frame number, so MSC
will always be zero for now,
and this makes mplayer choose target_msc = 0 as 'swap at next interval',
which appears to be broken by the commit.

The following patch fixes frame numbers in the nouveau kernel module, but it
only decreases the likelyhood of
occuring, it still happens when seeking, for example.

~Maarten

---
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 4610c3a..fd3cb62 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -572,17 +572,21 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct
drm_framebuffer *fb,
 	if (!s)
 		return -ENOMEM;
 
-	/* Don't let the buffers go away while we flip */
-	ret = nouveau_page_flip_reserve(old_bo, new_bo);
-	if (ret)
-		goto fail_free;
-
 	/* Initialize a page flip struct */
 	*s = (struct nouveau_page_flip_state)
 		{ { }, event, nouveau_crtc(crtc)->index,
 		  fb->bits_per_pixel, fb->pitches[0], crtc->x, crtc->y,
 		  new_bo->bo.offset };
 
+	ret = drm_vblank_get(dev, s->crtc);
+	if (ret)
+		goto fail_free;
+
+	/* Don't let the buffers go away while we flip */
+	ret = nouveau_page_flip_reserve(old_bo, new_bo);
+	if (ret)
+		goto fail_put;
+
 	/* Choose the channel the flip will be handled in */
 	fence = new_bo->bo.sync_obj;
 	if (fence)
@@ -614,6 +618,8 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct
drm_framebuffer *fb,
 
 fail_unreserve:
 	nouveau_page_flip_unreserve(old_bo, new_bo, NULL);
+fail_put:
+	drm_vblank_put(dev, s->crtc);
 fail_free:
 	kfree(s);
 	return ret;
@@ -638,24 +644,16 @@ nouveau_finish_page_flip(struct nouveau_channel *chan,
 	}
 
 	s = list_first_entry(&fctx->flip, struct nouveau_page_flip_state,
head);
-	if (s->event) {
-		struct drm_pending_vblank_event *e = s->event;
-		struct timeval now;
-
-		do_gettimeofday(&now);
-		e->event.sequence = 0;
-		e->event.tv_sec = now.tv_sec;
-		e->event.tv_usec = now.tv_usec;
-		list_add_tail(&e->base.link,
&e->base.file_priv->event_list);
-		wake_up_interruptible(&e->base.file_priv->event_wait);
-	}
-
 	list_del(&s->head);
+
+	if (s->event)
+		drm_send_vblank_event(dev, s->crtc, s->event);
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+	drm_vblank_put(dev, s->crtc);
+
 	if (ps)
 		*ps = *s;
 	kfree(s);
-
-	spin_unlock_irqrestore(&dev->event_lock, flags);
 	return 0;
 }

Op 28-03-13 12:00, Maarten Lankhorst schreef:
> Hey Mario,
>
> It seems that your ddx commit b4231dd715a8 is causing a regression when I
use mplayer -vo vdpau rendering with mesa 9.1.
> It fails to start drawing here, leaving the mplayer screen black, I can
make it recover usually by seeking or moving another
> window in front. When I revert the commit it behaves normally. Changing
can_sync_to_vblank(draw) to 0 in
> nouveau_dri2_schedule_swap also fixes it.
>
> The nouveau kernel driver currently doesn't fill in a frame number, so
MSC will always be zero for now,
> and this makes mplayer choose target_msc = 0 as 'swap at next
interval', which appears to be broken by the commit.
>
> The following patch fixes frame numbers in the nouveau kernel module, but
it only decreases the likelyhood of
> occuring, it still happens when seeking, for example.
>
> ~Maarten
>
Ohey found a flip bug too, that was weird, I'll send a patch for both. This
is just silly...

--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -593,7 +597,7 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct
drm_framebuffer *fb,
 
 	/* Emit a page flip */
 	if (nv_device(drm->device)->card_type >= NV_50) {
-		ret = nv50_display_flip_next(crtc, fb, chan, 0);
+		ret = nv50_display_flip_next(crtc, fb, chan, /*swap_interval=*/ 1);
 		if (ret) {
 			mutex_unlock(&chan->cli->mutex);
 			goto fail_unreserve;