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

[Zhenyu Wang]

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_.

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 3417cab..cea496d 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;
+	struct 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_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)

-- 
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/9e04bd73/attachment.sig>