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

[Jesse Barnes]

On Thu, 6 Mar 2014 09:35:23 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

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

Do you think that's worthwhile?  I guess we'll block userspace a bit
here, but presumably the cursor won't be visible until the mode set
completes anyway...

But queuing this stuff is another option.

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

Yeah that's a good point, and if we're not careful it could result in
some visible ugliness.

-- 
Jesse Barnes, Intel Open Source Technology Center