Re: [PATCH 07/11] drm/i915: Remove all *_pipes flags from modeset

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 10, 2015 at 11:38:36AM +0300, Ander Conselvan de Oliveira wrote:
> Set the mode_changed field on the crtc_states and use that instead.
> 
> Note that even though this patch doesn't completely replace the logic in
> intel_modeset_affected_pipes(), that logic was never fully used to its
> full extent. Since the commit mentioned below, modeset_pipes and
> prepare_pipes would only contain at most the pipe for which the set_crtc
> ioctl was called. We can grow back that logic when the time comes.
> 
> commit b6c5164d7bf624f3e1b750787ddb983150c5117c
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Apr 12 18:48:43 2013 +0200
> 
>     drm/i915: Fixup Oops in the pipe config computation

Bikeshed: Instead of switching to pipe_config I'd just switch to the
overall state for these functions. Or is there something preventing this?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   8 +
>  drivers/gpu/drm/i915/intel_display.c | 287 ++++++++++++++---------------------
>  2 files changed, 126 insertions(+), 169 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e12bc5a..08ea6d0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -272,6 +272,14 @@ enum hpd_pin {
>  	     (__i)++)							\
>  		if (connector)
>  
> +#define for_each_intel_crtc_in_state(state, crtc, crtc_state, __i)	\
> +	for ((__i) = 0;							\
> +	     (crtc) = to_intel_crtc((state)->crtcs[__i]),		\
> +	     (crtc_state) = to_intel_crtc_state((state)->crtc_states[__i]), \
> +	     (__i) < (state)->dev->mode_config.num_crtc;		\
> +	     (__i)++)							\
> +		if (crtc_state)
> +
>  struct drm_i915_private;
>  struct i915_mm_struct;
>  struct i915_mmu_object;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 232ef56..a255b24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5179,12 +5179,13 @@ static int intel_mode_max_pixclk(struct drm_atomic_state *state)
>  	return max_pixclk;
>  }
>  
> -static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
> -					    unsigned *prepare_pipes)
> +static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	int max_pixclk = intel_mode_max_pixclk(state);
> +	int i;
>  
>  	if (max_pixclk < 0)
>  		return max_pixclk;
> @@ -5193,10 +5194,20 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
>  	    dev_priv->vlv_cdclk_freq)
>  		return 0;
>  
> +	/* add all active pipes to the state */
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		if (!intel_crtc->base.state->enable)
> +			continue;
> +
> +		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +	}
> +
>  	/* disable/enable all currently active pipes while we change cdclk */
> -	for_each_intel_crtc(state->dev, intel_crtc)
> -		if (intel_crtc->base.state->enable)
> -			*prepare_pipes |= (1 << intel_crtc->pipe);
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> +		if (crtc_state->base.enable)
> +			crtc_state->base.mode_changed = true;
>  
>  	return 0;
>  }
> @@ -10879,94 +10890,6 @@ fail:
>  	return ERR_PTR(ret);
>  }
>  
> -/* Computes which crtcs are affected and sets the relevant bits in the mask. For
> - * simplicity we use the crtc's pipe number (because it's easier to obtain). */
> -static void
> -intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
> -			     unsigned *prepare_pipes, unsigned *disable_pipes)
> -{
> -	struct intel_crtc *intel_crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> -	struct drm_crtc *tmp_crtc;
> -
> -	*disable_pipes = *modeset_pipes = *prepare_pipes = 0;
> -
> -	/* Check which crtcs have changed outputs connected to them, these need
> -	 * to be part of the prepare_pipes mask. We don't (yet) support global
> -	 * modeset across multiple crtcs, so modeset_pipes will only have one
> -	 * bit set at most. */
> -	for_each_intel_connector(dev, connector) {
> -		if (connector->base.encoder == &connector->new_encoder->base)
> -			continue;
> -
> -		if (connector->base.encoder) {
> -			tmp_crtc = connector->base.encoder->crtc;
> -
> -			*prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
> -		}
> -
> -		if (connector->new_encoder)
> -			*prepare_pipes |=
> -				1 << connector->new_encoder->new_crtc->pipe;
> -	}
> -
> -	for_each_intel_encoder(dev, encoder) {
> -		if (encoder->base.crtc == &encoder->new_crtc->base)
> -			continue;
> -
> -		if (encoder->base.crtc) {
> -			tmp_crtc = encoder->base.crtc;
> -
> -			*prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
> -		}
> -
> -		if (encoder->new_crtc)
> -			*prepare_pipes |= 1 << encoder->new_crtc->pipe;
> -	}
> -
> -	/* Check for pipes that will be enabled/disabled ... */
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->base.state->enable == intel_crtc->new_enabled)
> -			continue;
> -
> -		if (!intel_crtc->new_enabled)
> -			*disable_pipes |= 1 << intel_crtc->pipe;
> -		else
> -			*prepare_pipes |= 1 << intel_crtc->pipe;
> -	}
> -
> -
> -	/* set_mode is also used to update properties on life display pipes. */
> -	intel_crtc = to_intel_crtc(crtc);
> -	if (intel_crtc->new_enabled)
> -		*prepare_pipes |= 1 << intel_crtc->pipe;
> -
> -	/*
> -	 * For simplicity do a full modeset on any pipe where the output routing
> -	 * changed. We could be more clever, but that would require us to be
> -	 * more careful with calling the relevant encoder->mode_set functions.
> -	 */
> -	if (*prepare_pipes)
> -		*modeset_pipes = *prepare_pipes;
> -
> -	/* ... and mask these out. */
> -	*modeset_pipes &= ~(*disable_pipes);
> -	*prepare_pipes &= ~(*disable_pipes);
> -
> -	/*
> -	 * HACK: We don't (yet) fully support global modesets. intel_set_config
> -	 * obies this rule, but the modeset restore mode of
> -	 * intel_modeset_setup_hw_state does not.
> -	 */
> -	*modeset_pipes &= 1 << intel_crtc->pipe;
> -	*prepare_pipes &= 1 << intel_crtc->pipe;
> -
> -	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
> -		      *modeset_pipes, *prepare_pipes, *disable_pipes);
> -}
> -
>  static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  {
>  	struct drm_encoder *encoder;
> @@ -10979,13 +10902,22 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  	return false;
>  }
>  
> +static bool
> +needs_modeset(struct intel_crtc_state *state)
> +{
> +	return state->base.mode_changed || state->base.active_changed;
> +}
> +
>  static void
> -intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
> +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 intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	struct drm_connector *connector;
> +	int i;
>  
>  	intel_shared_dpll_commit(dev_priv);
>  
> @@ -10993,9 +10925,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  		if (!intel_encoder->base.crtc)
>  			continue;
>  
> -		intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> +		for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> +			if (&intel_crtc->base == intel_encoder->base.crtc)
> +				break;
> +
> +		if (&intel_crtc->base != intel_encoder->base.crtc)
> +			continue;
>  
> -		if (prepare_pipes & (1 << intel_crtc->pipe))
> +		if (crtc_state->base.enable && needs_modeset(crtc_state))
>  			intel_encoder->connectors_active = false;
>  	}
>  
> @@ -11010,9 +10947,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  		if (!connector->encoder || !connector->encoder->crtc)
>  			continue;
>  
> -		intel_crtc = to_intel_crtc(connector->encoder->crtc);
> +		for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> +			if (&intel_crtc->base == connector->encoder->crtc)
> +				break;
> +
> +		if (&intel_crtc->base != connector->encoder->crtc)
> +			continue;
>  
> -		if (prepare_pipes & (1 << intel_crtc->pipe)) {
> +		if (crtc_state->base.enable && needs_modeset(crtc_state)) {
>  			struct drm_property *dpms_property =
>  				dev->mode_config.dpms_property;
>  
> @@ -11547,26 +11489,41 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  		crtc->scanline_offset = 1;
>  }
>  
> +static void
> +intel_atomic_modeset_compute_changed_flags(struct drm_atomic_state *state,
> +					   struct drm_crtc *modeset_crtc)
> +{
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_intel_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->base.enable != crtc->base.state->enable)
> +			crtc_state->base.mode_changed = true;
> +
> +		/* FIXME: move comment about changing properties in live pipes
> +		 * from affectec_pipes() here */
> +		if (&crtc->base == modeset_crtc && crtc_state->base.enable)
> +			crtc_state->base.mode_changed = true;
> +	}
> +}
> +
>  static struct intel_crtc_state *
>  intel_modeset_compute_config(struct drm_crtc *crtc,
>  			     struct drm_display_mode *mode,
>  			     struct drm_framebuffer *fb,
> -			     struct drm_atomic_state *state,
> -			     unsigned *modeset_pipes,
> -			     unsigned *prepare_pipes,
> -			     unsigned *disable_pipes)
> +			     struct drm_atomic_state *state)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc_state *pipe_config = NULL;
> +	struct intel_crtc_state *crtc_state;
>  	struct intel_crtc *intel_crtc;
>  	int ret = 0;
> +	int i;
>  
>  	ret = drm_atomic_add_affected_connectors(state, crtc);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	intel_modeset_affected_pipes(crtc, modeset_pipes,
> -				     prepare_pipes, disable_pipes);
> +	intel_atomic_modeset_compute_changed_flags(state, crtc);
>  
>  	/*
>  	 * Note this needs changes when we start tracking multiple modes
> @@ -11574,46 +11531,49 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	 * (i.e. one pipe_config for each crtc) rather than just the one
>  	 * for this crtc.
>  	 */
> -	for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) {
> -		/* FIXME: For now we still expect modeset_pipes has at most
> -		 * one bit set. */
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!crtc_state->base.enable || !needs_modeset(crtc_state))
> +			continue;
> +
> +		/* FIXME: For now we still expect modeset on only one pipe. */
>  		if (WARN_ON(&intel_crtc->base != crtc))
>  			continue;
>  
> -		pipe_config = intel_modeset_pipe_config(crtc, fb, mode, state);
> -		if (IS_ERR(pipe_config))
> -			return pipe_config;
> +		crtc_state = intel_modeset_pipe_config(crtc, fb, mode, state);
> +		if (IS_ERR(crtc_state))
> +			return crtc_state;
>  
> -		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +		intel_dump_pipe_config(to_intel_crtc(crtc), crtc_state,
>  				       "[modeset]");
>  	}
>  
>  	return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));;
>  }
>  
> -static int __intel_set_mode_setup_plls(struct drm_atomic_state *state,
> -				       unsigned modeset_pipes,
> -				       unsigned disable_pipes)
> +static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	unsigned clear_pipes = modeset_pipes | disable_pipes;
> +	unsigned clear_pipes = 0;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	int ret = 0;
> +	int i;
>  
>  	if (!dev_priv->display.crtc_compute_clock)
>  		return 0;
>  
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (needs_modeset(crtc_state))
> +			clear_pipes |= 1 << intel_crtc->pipe;
> +	}
> +
>  	ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
>  	if (ret)
>  		goto done;
>  
> -	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> -		struct intel_crtc_state *crtc_state =
> -			intel_atomic_get_crtc_state(state, intel_crtc);
> -
> -		/* Modeset pipes should have a new state by now */
> -		if (WARN_ON(IS_ERR(crtc_state)))
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!crtc_state->base.enable)
>  			continue;
>  
>  		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
> @@ -11631,10 +11591,7 @@ done:
>  static int __intel_set_mode(struct drm_crtc *crtc,
>  			    struct drm_display_mode *mode,
>  			    int x, int y, struct drm_framebuffer *fb,
> -			    struct intel_crtc_state *pipe_config,
> -			    unsigned modeset_pipes,
> -			    unsigned prepare_pipes,
> -			    unsigned disable_pipes)
> +			    struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -11642,7 +11599,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	struct drm_atomic_state *state = pipe_config->base.state;
>  	struct intel_crtc_state *crtc_state_copy = NULL;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state;
>  	int ret = 0;
> +	int i;
>  
>  	saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
>  	if (!saved_mode)
> @@ -11664,23 +11623,22 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
>  	if (IS_VALLEYVIEW(dev)) {
> -		ret = valleyview_modeset_global_pipes(state, &prepare_pipes);
> +		ret = valleyview_modeset_global_pipes(state);
>  		if (ret)
>  			goto done;
> -
> -		/* may have added more to prepare_pipes than we should */
> -		prepare_pipes &= ~disable_pipes;
>  	}
>  
> -	ret = __intel_set_mode_setup_plls(state, modeset_pipes, disable_pipes);
> +	ret = __intel_set_mode_setup_plls(state);
>  	if (ret)
>  		goto done;
>  
> -	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> -		intel_crtc_disable(&intel_crtc->base);
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!needs_modeset(crtc_state))
> +			continue;
>  
> -	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> -		if (intel_crtc->base.state->enable)
> +		if (!crtc_state->base.enable)
> +			intel_crtc_disable(&intel_crtc->base);
> +		else if (intel_crtc->base.state->enable)
>  			dev_priv->display.crtc_disable(&intel_crtc->base);
>  	}
>  
> @@ -11691,7 +11649,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * pipes; here we assume a single modeset_pipe and only track the
>  	 * single crtc and mode.
>  	 */
> -	if (modeset_pipes) {
> +	if (pipe_config->base.enable && needs_modeset(pipe_config)) {
>  		crtc->mode = *mode;
>  		/* mode_set/enable/disable functions rely on a correct pipe
>  		 * config. */
> @@ -11708,16 +11666,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
> -	intel_modeset_update_state(dev, prepare_pipes);
> +	intel_modeset_update_state(state);
>  
>  	modeset_update_crtc_power_domains(state);
>  
> -	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> -		struct drm_plane *primary = intel_crtc->base.primary;
> +	if (pipe_config->base.enable && needs_modeset(pipe_config)) {
> +		struct drm_plane *primary;
>  		int vdisplay, hdisplay;
>  
> +		intel_crtc = to_intel_crtc(crtc);
> +		primary = intel_crtc->base.primary;
> +
>  		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> -		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> +
> +		ret = primary->funcs->update_plane(primary, crtc,
>  						   fb, 0, 0,
>  						   hdisplay, vdisplay,
>  						   x << 16, y << 16,
> @@ -11725,7 +11687,10 @@ 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) {
> +	for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> +		if (!crtc_state->base.enable)
> +			continue;
> +
>  		update_scanline_offset(intel_crtc);
>  
>  		dev_priv->display.crtc_enable(&intel_crtc->base);
> @@ -11753,18 +11718,14 @@ done:
>  	return ret;
>  }
>  
> -static int intel_set_mode_pipes(struct drm_crtc *crtc,
> -				struct drm_display_mode *mode,
> -				int x, int y, struct drm_framebuffer *fb,
> -				struct intel_crtc_state *pipe_config,
> -				unsigned modeset_pipes,
> -				unsigned prepare_pipes,
> -				unsigned disable_pipes)
> +static int intel_set_mode_with_config(struct drm_crtc *crtc,
> +				      struct drm_display_mode *mode,
> +				      int x, int y, struct drm_framebuffer *fb,
> +				      struct intel_crtc_state *pipe_config)
>  {
>  	int ret;
>  
> -	ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config, modeset_pipes,
> -			       prepare_pipes, disable_pipes);
> +	ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config);
>  
>  	if (ret == 0)
>  		intel_modeset_check_state(crtc->dev);
> @@ -11778,22 +11739,15 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_atomic_state *state)
>  {
>  	struct intel_crtc_state *pipe_config;
> -	unsigned modeset_pipes, prepare_pipes, disable_pipes;
>  	int ret = 0;
>  
> -	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
> -						   &modeset_pipes,
> -						   &prepare_pipes,
> -						   &disable_pipes);
> -
> +	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state);
>  	if (IS_ERR(pipe_config)) {
>  		ret = PTR_ERR(pipe_config);
>  		goto out;
>  	}
>  
> -	ret = intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> -				   modeset_pipes, prepare_pipes,
> -				   disable_pipes);
> +	ret = intel_set_mode_with_config(crtc, mode, x, y, fb, pipe_config);
>  	if (ret)
>  		goto out;
>  
> @@ -12217,7 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	struct drm_atomic_state *state = NULL;
>  	struct intel_set_config *config;
>  	struct intel_crtc_state *pipe_config;
> -	unsigned modeset_pipes, prepare_pipes, disable_pipes;
>  	int ret;
>  
>  	BUG_ON(!set);
> @@ -12272,10 +12225,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	intel_set_config_compute_mode_changes(set, config);
>  
>  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
> -						   set->fb, state,
> -						   &modeset_pipes,
> -						   &prepare_pipes,
> -						   &disable_pipes);
> +						   set->fb, state);
>  	if (IS_ERR(pipe_config)) {
>  		ret = PTR_ERR(pipe_config);
>  		goto fail;
> @@ -12295,10 +12245,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	intel_update_pipe_size(to_intel_crtc(set->crtc));
>  
>  	if (config->mode_changed) {
> -		ret = intel_set_mode_pipes(set->crtc, set->mode,
> -					   set->x, set->y, set->fb, pipe_config,
> -					   modeset_pipes, prepare_pipes,
> -					   disable_pipes);
> +		ret = intel_set_mode_with_config(set->crtc, set->mode,
> +						 set->x, set->y, set->fb,
> +						 pipe_config);
>  	} else if (config->fb_changed) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
>  		struct drm_plane *primary = set->crtc->primary;
> -- 
> 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