Re: [PATCH v3.6 16/22] drm/i915: Implement intel_crtc_control using atomic state, v4.

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, May 26, 2015 at 10:35:22AM +0200, Maarten Lankhorst wrote:
> Assume the callers lock everything with drm_modeset_lock_all.
> 
> This change had to be done after converting suspend/resume to
> use atomic_state so the atomic state is preserved, otherwise
> all transitional state is erased.
> 
> Now all callers of .crtc_enable and .crtc_disable go through
> atomic modeset! :-D
> 
> Changes since v1:
> - Only check for crtc_state->active in valleyview_modeset_global_pipes.
> - Only check for crtc_state->active in modeset_update_crtc_power_domains.
> Changes since v2:
> - Rework on top of the changed patch order.
> Changes since v3:
> - Rename intel_crtc_toggle in description to *_control
> - Change return value to int.
> - Do not add plane state, should be done implicitly already.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9680eb85bd0..32bab28432f7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev)
>  }
>  
>  /* Master function to enable/disable CRTC and corresponding power wells */
> -void intel_crtc_control(struct drm_crtc *crtc, bool enable)
> +int intel_crtc_control(struct drm_crtc *crtc, bool enable)

We change the return value to 'int' here, but we never check the error
code anywhere.  Maybe we should hold this off for another patch and then
also propagate the return value back up the call chain?  It seems like
at least some of the connector-specific DPMS functions should recognize
the failure of intel_crtc_update_dpms() before trying to continue with
other operations.


>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum intel_display_power_domain domain;
> -	unsigned long domains;
> +	struct intel_crtc_state *pipe_config;
> +	struct drm_atomic_state *state;
> +	int ret;
>  
>  	if (enable == intel_crtc->active)
> -		return;
> +		return 0;
>  
>  	if (enable && !crtc->state->enable)
> -		return;
> +		return 0;
>  
> -	crtc->state->active = enable;
> -	if (enable) {
> -		domains = get_crtc_power_domains(crtc);
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_get(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = domains;
> +	/* this function should be called with drm_modeset_lock_all for now */
> +	if (WARN_ON(!ctx))
> +		return -EIO;

EIO seems like an unusual choice here (but I'm not really sure what the
best option would be).


Matt

> +	lockdep_assert_held(&ctx->ww_ctx);
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> -	} else {
> -		intel_crtc_disable_planes(crtc);
> -		dev_priv->display.crtc_disable(crtc);
> +	state = drm_atomic_state_alloc(dev);
> +	if (WARN_ON(!state))
> +		return -ENOMEM;
>  
> -		domains = intel_crtc->enabled_power_domains;
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_put(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = 0;
> +	state->acquire_ctx = ctx;
> +	state->allow_modeset = true;
> +
> +	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto err;
>  	}
> +	pipe_config->base.active = enable;
> +
> +	ret = intel_set_mode(state);
> +	if (!ret)
> +		return ret;
> +
> +err:
> +	DRM_ERROR("Updating crtc active failed with %i\n", ret);
> +	drm_atomic_state_free(state);
> +	return ret;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 60a29ff65f1f..c113f187090f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>  void intel_display_suspend(struct drm_device *dev);
> -void intel_crtc_control(struct drm_crtc *crtc, bool enable);
> +int intel_crtc_control(struct drm_crtc *crtc, bool enable);
>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx