Re: [PATCH v2 11/17] drm/i915: Use global atomic state for staged pll config

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 13, 2015 at 10:23:41PM +0200, Maarten Lankhorst wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> Now that we can subclass drm_atomic_state we can also use it to keep
> track of all the pll settings. atomic_state is a better place to hold
> all shared state than keeping pll->new_config everywhere.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   1 -
>  drivers/gpu/drm/i915/intel_atomic.c  |  49 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
>  4 files changed, 95 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a0e4868653f2..0e200018c9aa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
>  
>  struct intel_shared_dpll {
>  	struct intel_shared_dpll_config config;
> -	struct intel_shared_dpll_config *new_config;
>  
>  	int active; /* count of number of active CRTCs (i.e. DPMS on) */
>  	bool on; /* is the PLL actually active? Disabled during modeset */
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 7ed8033aae60..aff87054406c 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -421,3 +421,52 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  
>  	return 0;
>  }
> +
> +static void intel_atomic_duplicate_dpll_state(struct drm_device *dev,
> +					      struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_shared_dpll *pll;
> +	enum intel_dpll_id i;
> +
> +	state->dpll_set = true;

The ww mutex locking dance here is missing. For simplicity I think we
could just repurpose the dev->mode_config.connection_mutex. We need that
anyway full modesets, and dpll changes should only be required in those.
Which means this wont introduce any unecessary parallelism.

It means though that we need to wire up a proper error code to all callers
of duplicate/get_dpll_state, like with all the other atomic states. Might
be best to follow the design for connector/crtc/planes an pass around
pointers with error codes explicitly (instead of returning
state->shared_dpll below since that can only cope with NULL and not with
-EDEADLK).

Sorry that I didn't spot this earlier.
-Daniel

> +
> +	/* Copy shared dpll state */
> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +		pll = &dev_priv->shared_dplls[i];
> +
> +		memcpy(&state->shared_dpll[i],
> +		       &pll->config, sizeof(pll->config));
> +	}
> +}
> +
> +struct intel_shared_dpll_config *
> +intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s)
> +{
> +	struct intel_atomic_state *state = to_intel_atomic_state(s);
> +
> +	if (!state->dpll_set)
> +		intel_atomic_duplicate_dpll_state(state->base.dev, state);
> +
> +	return state->shared_dpll;
> +}
> +
> +struct drm_atomic_state *
> +intel_atomic_state_alloc(struct drm_device *dev)
> +{
> +	struct intel_atomic_state *state = kzalloc(GFP_KERNEL, sizeof(*state));
> +
> +	if (!state || drm_atomic_state_init(dev, &state->base) < 0) {
> +		kfree(state);
> +		return NULL;
> +	}
> +
> +	return &state->base;
> +}
> +
> +void intel_atomic_state_clear(struct drm_atomic_state *s)
> +{
> +	struct intel_atomic_state *state = to_intel_atomic_state(s);
> +	drm_atomic_state_default_clear(&state->base);
> +	state->dpll_set = false;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0a2a7b6e198c..ed69eddf457e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4229,8 +4229,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  	struct intel_shared_dpll *pll;
> +	struct intel_shared_dpll_config *shared_dpll;
>  	enum intel_dpll_id i;
>  
> +	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
> +
>  	if (HAS_PCH_IBX(dev_priv->dev)) {
>  		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>  		i = (enum intel_dpll_id) crtc->pipe;
> @@ -4239,7 +4242,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>  			      crtc->base.base.id, pll->name);
>  
> -		WARN_ON(pll->new_config->crtc_mask);
> +		WARN_ON(shared_dpll[i].crtc_mask);
>  
>  		goto found;
>  	}
> @@ -4259,7 +4262,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  		pll = &dev_priv->shared_dplls[i];
>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>  			crtc->base.base.id, pll->name);
> -		WARN_ON(pll->new_config->crtc_mask);
> +		WARN_ON(shared_dpll[i].crtc_mask);
>  
>  		goto found;
>  	}
> @@ -4268,15 +4271,15 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> -		if (pll->new_config->crtc_mask == 0)
> +		if (shared_dpll[i].crtc_mask == 0)
>  			continue;
>  
>  		if (memcmp(&crtc_state->dpll_hw_state,
> -			   &pll->new_config->hw_state,
> -			   sizeof(pll->new_config->hw_state)) == 0) {
> +			   &shared_dpll[i].hw_state,
> +			   sizeof(crtc_state->dpll_hw_state)) == 0) {
>  			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>  				      crtc->base.base.id, pll->name,
> -				      pll->new_config->crtc_mask,
> +				      shared_dpll[i].crtc_mask,
>  				      pll->active);
>  			goto found;
>  		}
> @@ -4285,7 +4288,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  	/* Ok no matching timings, maybe there's a free one? */
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -		if (pll->new_config->crtc_mask == 0) {
> +		if (shared_dpll[i].crtc_mask == 0) {
>  			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>  				      crtc->base.base.id, pll->name);
>  			goto found;
> @@ -4295,83 +4298,33 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  	return NULL;
>  
>  found:
> -	if (pll->new_config->crtc_mask == 0)
> -		pll->new_config->hw_state = crtc_state->dpll_hw_state;
> +	if (shared_dpll[i].crtc_mask == 0)
> +		shared_dpll[i].hw_state =
> +			crtc_state->dpll_hw_state;
>  
>  	crtc_state->shared_dpll = i;
>  	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>  			 pipe_name(crtc->pipe));
>  
> -	pll->new_config->crtc_mask |= 1 << crtc->pipe;
> +	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
>  
>  	return pll;
>  }
>  
> -/**
> - * intel_shared_dpll_start_config - start a new PLL staged config
> - * @dev_priv: DRM device
> - * @clear_pipes: mask of pipes that will have their PLLs freed
> - *
> - * Starts a new PLL staged config, copying the current config but
> - * releasing the references of pipes specified in clear_pipes.
> - */
> -static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
> -					  unsigned clear_pipes)
> -{
> -	struct intel_shared_dpll *pll;
> -	enum intel_dpll_id i;
> -
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> -		pll = &dev_priv->shared_dplls[i];
> -
> -		pll->new_config = kmemdup(&pll->config, sizeof pll->config,
> -					  GFP_KERNEL);
> -		if (!pll->new_config)
> -			goto cleanup;
> -
> -		pll->new_config->crtc_mask &= ~clear_pipes;
> -	}
> -
> -	return 0;
> -
> -cleanup:
> -	while (--i >= 0) {
> -		pll = &dev_priv->shared_dplls[i];
> -		kfree(pll->new_config);
> -		pll->new_config = NULL;
> -	}
> -
> -	return -ENOMEM;
> -}
> -
> -static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
> +static void intel_shared_dpll_commit(struct drm_atomic_state *state)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_shared_dpll_config *shared_dpll;
>  	struct intel_shared_dpll *pll;
>  	enum intel_dpll_id i;
>  
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> -		pll = &dev_priv->shared_dplls[i];
> -
> -		WARN_ON(pll->new_config == &pll->config);
> -
> -		pll->config = *pll->new_config;
> -		kfree(pll->new_config);
> -		pll->new_config = NULL;
> -	}
> -}
> -
> -static void intel_shared_dpll_abort_config(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_shared_dpll *pll;
> -	enum intel_dpll_id i;
> +	if (!to_intel_atomic_state(state)->dpll_set)
> +		return;
>  
> +	shared_dpll = to_intel_atomic_state(state)->shared_dpll;
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -
> -		WARN_ON(pll->new_config == &pll->config);
> -
> -		kfree(pll->new_config);
> -		pll->new_config = NULL;
> +		pll->config = shared_dpll[i];
>  	}
>  }
>  
> @@ -11532,13 +11485,12 @@ static void
>  intel_modeset_update_state(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *connector;
>  
> -	intel_shared_dpll_commit(dev_priv);
> +	intel_shared_dpll_commit(state);
>  	drm_atomic_helper_swap_state(state->dev, state);
>  
>  	for_each_intel_encoder(dev, intel_encoder) {
> @@ -12171,9 +12123,13 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  		}
>  	}
>  
> -	ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
> -	if (ret)
> -		goto done;
> +	if (clear_pipes) {
> +		struct intel_shared_dpll_config *shared_dpll =
> +			intel_atomic_get_shared_dpll_state(state);
> +
> +		for (i = 0; i < dev_priv->num_shared_dpll; i++)
> +			shared_dpll[i].crtc_mask &= ~clear_pipes;
> +	}
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (!needs_modeset(crtc_state) || !crtc_state->enable)
> @@ -12184,13 +12140,10 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  
>  		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
>  							   intel_crtc_state);
> -		if (ret) {
> -			intel_shared_dpll_abort_config(dev_priv);
> -			goto done;
> -		}
> +		if (ret)
> +			return ret;
>  	}
>  
> -done:
>  	return ret;
>  }
>  
> @@ -13887,6 +13840,8 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
>  	.output_poll_changed = intel_fbdev_output_poll_changed,
>  	.atomic_check = intel_atomic_check,
>  	.atomic_commit = intel_atomic_commit,
> +	.atomic_state_alloc = intel_atomic_state_alloc,
> +	.atomic_state_clear = intel_atomic_state_clear,
>  };
>  
>  /* Set up chip specific display functions */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d0d99669aa00..7012c67de8d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -241,6 +241,13 @@ typedef struct dpll {
>  	int	p;
>  } intel_clock_t;
>  
> +struct intel_atomic_state {
> +	struct drm_atomic_state base;
> +
> +	bool dpll_set;
> +	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> +};
> +
>  struct intel_plane_state {
>  	struct drm_plane_state base;
>  	struct drm_rect src;
> @@ -627,6 +634,7 @@ struct cxsr_latency {
>  	unsigned long cursor_hpll_disable;
>  };
>  
> +#define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
>  #define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, base)
>  #define to_intel_connector(x) container_of(x, struct intel_connector, base)
> @@ -1402,6 +1410,11 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			       struct drm_crtc_state *state);
> +struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
> +void intel_atomic_state_clear(struct drm_atomic_state *);
> +struct intel_shared_dpll_config *
> +intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
> +
>  static inline struct intel_crtc_state *
>  intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  			    struct intel_crtc *crtc)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx