[Intel-gfx] [PATCH 3/3] drm/i915: Don't wait interruptible for possible plane buffer flush

[Zhenyu Wang]

On 2009.12.01 13:33:24 +0800, Zhenyu Wang wrote:
> On 2009.11.30 12:59:30 -0800, Jesse Barnes wrote:
> > On Thu, 26 Nov 2009 09:08:52 +0800
> > Zhenyu Wang <zhenyuw at linux.intel.com> wrote:
> > 
> > > On 2009.11.25 12:16:33 -0800, Eric Anholt wrote:
> > > > On Wed, 25 Nov 2009 13:09:39 +0800, Zhenyu Wang
> > > > <zhenyuw at linux.intel.com> wrote:
> > > > > When we setup buffer for display plane, we'll check any pending
> > > > > required GPU flush and possible make interruptible wait for flush
> > > > > complete. But that wait would be most possibly to fail in case of
> > > > > signals received for X process, which will then fail modeset
> > > > > process and put display engine in unconsistent state. The result
> > > > > could be blank screen or CPU hang, and DDX driver would always
> > > > > turn on outputs DPMS after whatever modeset fails or not.
> > > > > 
> > > > > So this one creates new helper for setup display plane buffer, and
> > > > > when needing flush using uninterruptible wait for that.
> > > > 
> > > > Can you explain why the modeset process is special and shouldn't be
> > > > interruptible?  It certainly seems to me like it should be
> > > > interruptible like other driver commands are.
> > > 
> > > If wait is interrupted by something else, it will abort plane setup
> > > and mode_set function, within that we've already done new DPLL
> > > setting and other stuff. Failure in mode_set also won't proceed on
> > > real pipe setup later in commit function, so we're stopped in the
> > > middle of the timing sensitive modesetting process. Normally no plane
> > > enabled means blank screen like for bug 24009.
> > 
> > It's a nasty bug, thanks for catching it.  We can either make our GTT
> > transition uninterruptible (what you've done), or we can make the
> > failure paths of the mode setting core a bit more robust against
> > signals.  Have you looked at what it would take to do the latter?  It
> > might be good to do anyway, since even with an uninterruptible flush we
> > might fail (or timeout, do we do that?) and so would need to unwind any
> > previous programming...
> > 
> 
> We don't have a timeout version of that yet. I don't have good idea on how
> to handle it in failure path, undo pipe and output off action? I was trying
> to put fb domain change earlier, but current crtc->prepare() has void return,
> so not a good place for this. Or earlier in mode_set, we might still need
> to handle for signal. So my point is we'd better not to be interrupted in
> mode setting.
> 
> I think another approach might be simpler like below patch, that we always
> try to wait rendering on fb bo to be finished before modesetting. So later
> kernel will just see it fine for plane and ready to be used, we may remove
> the domain change in kernel as well, but that depends on user space version.
> This patch is totally _untested_.
> 

Sorry, this version fixes compiling errors...Any comment?

From 909300f6b91a0fc367397d6880759cc91fec4806 Mon Sep 17 00:00:00 2001
From: Zhenyu Wang <zhenyuw at linux.intel.com>
Date: Tue, 1 Dec 2009 14:16:32 +0800
Subject: [PATCH] Wait rendering to fb before modesetting

Safely wait rendering to be complete for front buffer before
modesetting. Kernel does interruptible wait for this, but signals
would abort it which fails the whole modesetting process.

Signed-off-by: Zhenyu Wang <zhenyuw at linux.intel.com>
---
 src/drmmode_display.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index a469f6c..707e148 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -313,6 +313,7 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 	int fb_id;
 	drmModeModeInfo kmode;
 	unsigned int pitch = scrn->displayWidth * intel->cpp;
+	drm_intel_bo *fb_bo;
 
 	if (drmmode->fb_id == 0) {
 		ret = drmModeAddFB(drmmode->fd,
@@ -336,6 +337,8 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 	crtc->y = y;
 	crtc->rotation = rotation;
 
+	fb_bo = intel->front_buffer->bo;
+
 	output_ids = xcalloc(sizeof(uint32_t), xf86_config->num_output);
 	if (!output_ids) {
 		ret = FALSE;
@@ -376,7 +379,11 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		fb_id = drmmode_crtc->rotate_fb_id;
 		x = 0;
 		y = 0;
+		fb_bo = drmmode_crtc->rotate_bo;
 	}
+	/* Wait rendering to fb object complete before actual modesetting */
+	drm_intel_gem_bo_start_gtt_access(fb_bo, 1);
+
 	ret = drmModeSetCrtc(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
 			     fb_id, x, y, output_ids, output_count, &kmode);
 	if (ret)
-- 
1.6.3.3

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20091201/647e5fda/attachment.sig>