[Intel-gfx] [PATCH 1/6] drm/i915: make CRTC enable/disable asynchronous

[Chris Wilson]

On Wed, Mar 05, 2014 at 02:48:26PM -0800, Jesse Barnes wrote:
> @@ -7554,6 +7610,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  		goto fail;
>  	}
>  
> +	intel_sync_crtc(crtc);
> +
>  	/* we only need to pin inside GTT if cursor is non-phy */
>  	mutex_lock(&dev->struct_mutex);
>  	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> @@ -7639,6 +7697,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  	intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
>  	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
>  
> +	intel_sync_crtc(crtc);
> +
>  	if (intel_crtc->active)
>  		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
>  

Hmm. Would be much nicer if touching the cursor didn't incur a delay.
And it would seem to quite easy to capture the state change and queue it
for when the CRTC is re-enabled.

> @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> +	intel_sync_crtc(crtc);
> +
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -9677,8 +9739,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		intel_crtc_disable(&intel_crtc->base);
>  
>  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> -		if (intel_crtc->base.enabled)
> -			dev_priv->display.crtc_disable(&intel_crtc->base);
> +		if (intel_crtc->base.enabled) {
> +			intel_queue_crtc_disable(&intel_crtc->base);
> +			intel_sync_crtc(&intel_crtc->base);
> +		}
>  	}
>  
>  	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
> @@ -9719,7 +9783,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> -		dev_priv->display.crtc_enable(&intel_crtc->base);
> +		intel_queue_crtc_enable(&intel_crtc->base);
>  
>  	/* FIXME: add subpixel order */
>  done:
> @@ -9740,8 +9804,9 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  
>  	ret = __intel_set_mode(crtc, mode, x, y, fb);
>  
> -	if (ret == 0)
> -		intel_modeset_check_state(crtc->dev);
> +	/* FIXME: check after async crtc enable/disable */
> +//	if (ret == 0)
> +//		intel_modeset_check_state(crtc->dev);
>  
>  	return ret;
>  }
> @@ -10306,6 +10371,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> +
> +	INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work);
> +	INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work);

I feel using independent work items (remember the global wq is really a
pool of many wq) is horribly prone to deadlocks.

We have the usual caveat that this has an implicit API change in that
setcrtc can now return before the change is complete - and so userspace
may write to a still currently visible scanout. Its not a huge issue
(and is a change I am in favour of), it is just a change in behaviour we
have to be wary of (which also means stating it in the changelog for future
reference).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre