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

[Imre Deak]

On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote:
> This lets us return to userspace more quickly and should improve init
> and suspend/resume times as well, allowing us to return to userspace
> sooner.

IMHO this is a good move towards a full command queue based solution for
kms commands, where eventually we have to think less of concurrency.
That is if we can queue all the other kms commands too (flip,
set_plane). But I don't see why that wouldn't be possible.

Btw, why do you have a separate disable and enable queue?

--Imre

> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |   2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |   4 +-
>  drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |   4 ++
>  4 files changed, 94 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 08052f3d..29cc079 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -456,7 +456,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		 */
>  		mutex_lock(&dev->mode_config.mutex);
>  		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -			dev_priv->display.crtc_disable(crtc);
> +			dev_priv->display._crtc_disable(crtc);
>  		mutex_unlock(&dev->mode_config.mutex);
>  
>  		intel_modeset_suspend_hw(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 42f83f2..4c39bb5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -438,8 +438,8 @@ struct drm_i915_display_funcs {
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> -	void (*crtc_enable)(struct drm_crtc *crtc);
> -	void (*crtc_disable)(struct drm_crtc *crtc);
> +	void (*_crtc_enable)(struct drm_crtc *crtc);
> +	void (*_crtc_disable)(struct drm_crtc *crtc);
>  	void (*off)(struct drm_crtc *crtc);
>  	void (*write_eld)(struct drm_connector *connector,
>  			  struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 46ce940..c066a7d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1747,6 +1747,63 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  	I915_WRITE(_TRANSA_CHICKEN2, val);
>  }
>  
> +static void intel_sync_crtc(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = crtc->dev;
> +
> +	WARN(!mutex_is_locked(&intel_crtc->base.mutex), "need crtc mutex\n");
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	mutex_unlock(&intel_crtc->base.mutex);
> +	flush_work(&intel_crtc->disable_work);
> +	flush_work(&intel_crtc->enable_work);
> +	mutex_lock(&intel_crtc->base.mutex);
> +	mutex_lock(&dev->mode_config.mutex);
> +}
> +
> +static void intel_crtc_disable_work(struct work_struct *work)
> +{
> +	struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc,
> +						     disable_work);
> +	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	mutex_lock(&intel_crtc->base.mutex);
> +	dev_priv->display._crtc_disable(&intel_crtc->base);
> +	mutex_unlock(&intel_crtc->base.mutex);
> +	mutex_unlock(&dev->mode_config.mutex);
> +}
> +
> +void intel_queue_crtc_disable(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	schedule_work(&intel_crtc->disable_work);
> +}
> +
> +static void intel_crtc_enable_work(struct work_struct *work)
> +{
> +	struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc,
> +						     enable_work);
> +	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	mutex_lock(&intel_crtc->base.mutex);
> +	dev_priv->display._crtc_enable(&intel_crtc->base);
> +	mutex_unlock(&intel_crtc->base.mutex);
> +	mutex_unlock(&dev->mode_config.mutex);
> +}
> +
> +static void intel_queue_crtc_enable(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	schedule_work(&intel_crtc->enable_work);
> +}
> +
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
>   * @crtc: crtc responsible for the pipe
> @@ -4309,7 +4366,6 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc,
>  void intel_crtc_update_dpms(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder;
>  	bool enable = false;
>  
> @@ -4317,9 +4373,9 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
>  		enable |= intel_encoder->connectors_active;
>  
>  	if (enable)
> -		dev_priv->display.crtc_enable(crtc);
> +		intel_queue_crtc_enable(crtc);
>  	else
> -		dev_priv->display.crtc_disable(crtc);
> +		intel_queue_crtc_disable(crtc);
>  
>  	intel_crtc_update_sarea(crtc, enable);
>  }
> @@ -4334,7 +4390,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	/* crtc should still be enabled when we disable it. */
>  	WARN_ON(!crtc->enabled);
>  
> -	dev_priv->display.crtc_disable(crtc);
> +	dev_priv->display._crtc_disable(crtc);
>  	intel_crtc->eld_vld = false;
>  	intel_crtc_update_sarea(crtc, false);
>  	dev_priv->display.off(crtc);
> @@ -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);
>  
> @@ -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);
>  }
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> @@ -10716,29 +10784,29 @@ static void intel_init_display(struct drm_device *dev)
>  	if (HAS_DDI(dev)) {
>  		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
> -		dev_priv->display.crtc_enable = haswell_crtc_enable;
> -		dev_priv->display.crtc_disable = haswell_crtc_disable;
> +		dev_priv->display._crtc_enable = haswell_crtc_enable;
> +		dev_priv->display._crtc_disable = haswell_crtc_disable;
>  		dev_priv->display.off = haswell_crtc_off;
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
> -		dev_priv->display.crtc_enable = ironlake_crtc_enable;
> -		dev_priv->display.crtc_disable = ironlake_crtc_disable;
> +		dev_priv->display._crtc_enable = ironlake_crtc_enable;
> +		dev_priv->display._crtc_disable = ironlake_crtc_disable;
>  		dev_priv->display.off = ironlake_crtc_off;
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
> -		dev_priv->display.crtc_enable = valleyview_crtc_enable;
> -		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> +		dev_priv->display._crtc_enable = valleyview_crtc_enable;
> +		dev_priv->display._crtc_disable = i9xx_crtc_disable;
>  		dev_priv->display.off = i9xx_crtc_off;
>  		dev_priv->display.update_plane = i9xx_update_plane;
>  	} else {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
> -		dev_priv->display.crtc_enable = i9xx_crtc_enable;
> -		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> +		dev_priv->display._crtc_enable = i9xx_crtc_enable;
> +		dev_priv->display._crtc_disable = i9xx_crtc_disable;
>  		dev_priv->display.off = i9xx_crtc_off;
>  		dev_priv->display.update_plane = i9xx_update_plane;
>  	}
> @@ -11142,7 +11210,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 * ...  */
>  		plane = crtc->plane;
>  		crtc->plane = !plane;
> -		dev_priv->display.crtc_disable(&crtc->base);
> +		intel_queue_crtc_disable(&crtc->base);
>  		crtc->plane = plane;
>  
>  		/* ... and break all links. */
> @@ -11417,7 +11485,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		intel_modeset_update_staged_output_state(dev);
>  	}
>  
> -	intel_modeset_check_state(dev);
> +//	intel_modeset_check_state(dev);
>  }
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a4ffc02..b90f1f5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,6 +384,9 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	struct work_struct enable_work;
> +	struct work_struct disable_work;
>  };
>  
>  struct intel_plane_wm_parameters {
> @@ -736,6 +739,7 @@ void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_config *pipe_config);
> +void intel_queue_crtc_disable(struct drm_crtc *crtc);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);