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

[Jesse Barnes]

On Tue, 1 Dec 2009 14:18:14 +0800
Zhenyu Wang <zhenyuw at linux.intel.com> wrote:

> 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)

Given the failure mode, it seems like we should have a fix in the
kernel.  I don't really have an issue with making mode setting ignore
signals, but your patch makes me wonder if we don't handle other error
cases very gracefully either, that's all.

So I'm fine with your original kernel patch.

-- 
Jesse Barnes, Intel Open Source Technology Center