Re: [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Wed, 2015-03-18 at 23:57 +0000, Konduru, Chandra wrote:
> 
> > -----Original Message-----
> > From: Conselvan De Oliveira, Ander
> > Sent: Wednesday, March 18, 2015 12:57 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
> > Subject: [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy
> > modeset code
> > 
> > For the atomic conversion, the mode set paths need to be changed to rely on an
> > atomic state instead of using the staged config. By using an atomic state for the
> > legacy code, we will be able to convert the code base in small chunks.
> > 
> > v2: Squash patch that adds stat argument to intel_set_mode(). (Ander)
> >     Make every caller of intel_set_mode() allocate state. (Daniel)
> >     Call drm_atomic_state_clear() in set config's error path. (Daniel)
> > 
> > v3: Copy staged config to atomic state in force restore path. (Ander)
> > 
> > v4: Don't update ->new_config for disabled pipes in __intel_set_mode(),
> >     since it is expected to be NULL in that case. (Ander)
> 
> Can you clarify why it is expected to be NULL?

This is part of the sanity checking implemented when the
intel_crtc->new_config pointer was introduced. If I understand
correctly, the idea was to reduce the usage of that pointer to only the
brief moment when it pointed at something different than the current
config. That way, if some piece of code relied on that pointer outside
of modeset, it would be easier to catch that bug.

> > 
> > Signed-off-by: Ander Conselvan de Oliveira
> > <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 200 +++++++++++++++++++++++++++++-
> > -----
> >  1 file changed, 165 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 8458bf5..ce35647 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -83,7 +83,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> >  				   struct intel_crtc_state *pipe_config);
> > 
> >  static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode
> > *mode,
> > -			  int x, int y, struct drm_framebuffer *old_fb);
> > +			  int x, int y, struct drm_framebuffer *old_fb,
> > +			  struct drm_atomic_state *state);
> >  static int intel_framebuffer_init(struct drm_device *dev,
> >  				  struct intel_framebuffer *ifb,
> >  				  struct drm_mode_fb_cmd2 *mode_cmd, @@
> > -8802,6 +8803,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
> > *connector,
> >  	struct drm_device *dev = encoder->dev;
> >  	struct drm_framebuffer *fb;
> >  	struct drm_mode_config *config = &dev->mode_config;
> > +	struct drm_atomic_state *state = NULL;
> >  	int ret, i = -1;
> > 
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", @@
> > -8883,6 +8885,12 @@ retry:
> >  	old->load_detect_temp = true;
> >  	old->release_fb = NULL;
> > 
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state)
> > +		return false;
> > +
> > +	state->acquire_ctx = ctx;
> > +
> >  	if (!mode)
> >  		mode = &load_detect_mode;
> > 
> > @@ -8905,7 +8913,7 @@ retry:
> >  		goto fail;
> >  	}
> > 
> > -	if (intel_set_mode(crtc, mode, 0, 0, fb)) {
> > +	if (intel_set_mode(crtc, mode, 0, 0, fb, state)) {
> >  		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
> >  		if (old->release_fb)
> >  			old->release_fb->funcs->destroy(old->release_fb);
> > @@ -8924,6 +8932,11 @@ retry:
> >  	else
> >  		intel_crtc->new_config = NULL;
> >  fail_unlock:
> > +	if (state) {
> > +		drm_atomic_state_free(state);
> > +		state = NULL;
> > +	}
> > +
> >  	if (ret == -EDEADLK) {
> >  		drm_modeset_backoff(ctx);
> >  		goto retry;
> > @@ -8936,22 +8949,34 @@ void intel_release_load_detect_pipe(struct
> > drm_connector *connector,
> >  				    struct intel_load_detect_pipe *old,
> >  				    struct drm_modeset_acquire_ctx *ctx)  {
> > +	struct drm_device *dev = connector->dev;
> >  	struct intel_encoder *intel_encoder =
> >  		intel_attached_encoder(connector);
> >  	struct drm_encoder *encoder = &intel_encoder->base;
> >  	struct drm_crtc *crtc = encoder->crtc;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct drm_atomic_state *state;
> > 
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> >  		      connector->base.id, connector->name,
> >  		      encoder->base.id, encoder->name);
> > 
> >  	if (old->load_detect_temp) {
> > +		state = drm_atomic_state_alloc(dev);
> > +		if (!state) {
> > +			DRM_DEBUG_KMS("can't release load detect pipe\n");
> > +			return;
> > +		}
> > +
> > +		state->acquire_ctx = ctx;
> > +
> >  		to_intel_connector(connector)->new_encoder = NULL;
> >  		intel_encoder->new_crtc = NULL;
> >  		intel_crtc->new_enabled = false;
> >  		intel_crtc->new_config = NULL;
> > -		intel_set_mode(crtc, NULL, 0, 0, NULL);
> > +		intel_set_mode(crtc, NULL, 0, 0, NULL, state);
> > +
> > +		drm_atomic_state_free(state);
> > 
> >  		if (old->release_fb) {
> >  			drm_framebuffer_unregister_private(old->release_fb);
> > @@ -10345,10 +10370,22 @@ static bool check_digital_port_conflicts(struct
> > drm_device *dev)
> >  	return true;
> >  }
> > 
> > -static struct intel_crtc_state *
> > +static void
> > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state) {
> > +	struct drm_crtc_state tmp_state;
> > +
> > +	/* Clear only the intel specific part of the crtc state */
> > +	tmp_state = crtc_state->base;
> > +	memset(crtc_state, 0, sizeof *crtc_state);
> > +	crtc_state->base = tmp_state;
> > +}
> In scalers patch above function has an update to preserve
> scaler_state. Hopefully your patch gets merged first then
> scalers patch.
> 
> > +
> > +static int
> >  intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  			  struct drm_framebuffer *fb,
> > -			  struct drm_display_mode *mode)
> > +			  struct drm_display_mode *mode,
> > +			  struct drm_atomic_state *state)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct intel_encoder *encoder;
> > @@ -10358,17 +10395,19 @@ intel_modeset_pipe_config(struct drm_crtc
> > *crtc,
> > 
> >  	if (!check_encoder_cloning(to_intel_crtc(crtc))) {
> >  		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> > -		return ERR_PTR(-EINVAL);
> > +		return -EINVAL;
> >  	}
> > 
> >  	if (!check_digital_port_conflicts(dev)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port
> > configuration\n");
> > -		return ERR_PTR(-EINVAL);
> > +		return -EINVAL;
> >  	}
> > 
> > -	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
> > -	if (!pipe_config)
> > -		return ERR_PTR(-ENOMEM);
> > +	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> > +	if (IS_ERR(pipe_config))
> > +		return PTR_ERR(pipe_config);
> > +
> > +	clear_intel_crtc_state(pipe_config);
> > 
> >  	pipe_config->base.crtc = crtc;
> >  	drm_mode_copy(&pipe_config->base.adjusted_mode, mode); @@ -
> > 10463,10 +10502,9 @@ encoder_retry:
> >  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
> >  		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> > 
> > -	return pipe_config;
> > +	return 0;
> >  fail:
> > -	kfree(pipe_config);
> > -	return ERR_PTR(ret);
> > +	return ret;
> >  }
> > 
> >  /* Computes which crtcs are affected and sets the relevant bits in the mask. For
> > @@ -11144,17 +11182,19 @@ 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 intel_crtc_state *pipe_config = NULL;
> > +	int ret = 0;
> > 
> >  	intel_modeset_affected_pipes(crtc, modeset_pipes,
> >  				     prepare_pipes, disable_pipes);
> > 
> >  	if ((*modeset_pipes) == 0)
> > -		goto out;
> > +		return NULL;
> > 
> >  	/*
> >  	 * Note this needs changes when we start tracking multiple modes @@ -
> > 11162,14 +11202,17 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
> >  	 * (i.e. one pipe_config for each crtc) rather than just the one
> >  	 * for this crtc.
> >  	 */
> > -	pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> > -	if (IS_ERR(pipe_config)) {
> > -		goto out;
> > -	}
> > +	ret = intel_modeset_pipe_config(crtc, fb, mode, state);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> > +	if (IS_ERR(pipe_config))
> > +		return pipe_config;
> > +
> 
> Why don't intel_modeset_pipe_config() return pipe_config as it used to, 
> instead of not returning and then calling intel_atomic_get_crtc_state()?

No particular reason. I think it made sense at some intermediate design
step, and I just left it like that.

> >  	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> >  			       "[modeset]");
> > 
> > -out:
> >  	return pipe_config;
> >  }
> > 
> > @@ -11214,6 +11257,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_display_mode *saved_mode;
> > +	struct intel_crtc_state *crtc_state_copy = NULL;
> >  	struct intel_crtc *intel_crtc;
> >  	int ret = 0;
> > 
> > @@ -11221,6 +11265,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  	if (!saved_mode)
> >  		return -ENOMEM;
> > 
> > +	crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL);
> > +	if (!crtc_state_copy) {
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> >  	*saved_mode = crtc->mode;
> > 
> >  	if (modeset_pipes)
> > @@ -11307,6 +11357,22 @@ done:
> >  	if (ret && crtc->state->enable)
> >  		crtc->mode = *saved_mode;
> > 
> > +	if (ret == 0 && pipe_config) {
> > +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +		/* The pipe_config will be freed with the atomic state, so
> > +		 * make a copy. */
> > +		memcpy(crtc_state_copy, intel_crtc->config,
> > +		       sizeof *crtc_state_copy);
> > +		intel_crtc->config = crtc_state_copy;
> > +		intel_crtc->base.state = &crtc_state_copy->base;
> 
> Why don't you avoid creating a crtc_state_copy by not freeing intel_crtc->config
> and copying pipe_config directly into intel_crtc->config?
> This should be fine when drm atomic free function frees crtc_state inside drm_state.

Except that intel_crtc->new_config will still point to the freed
pipe_config, so we would have to add logic to fix that too. And on patch
4 make sure the pipe_config is updated even if modeset_pipes is 0.

Since what in the end what we really want is to have a simple swap, I
rather avoid extra churn here, perfecting code that is going to be
deleted.

> Once atomic_crtc fully done, I guess this copy should go away
> and swap of crtc_states should take care of this.
> 
> > +
> > +		if (modeset_pipes)
> > +			intel_crtc->new_config = intel_crtc->config;
> > +	} else {
> > +		kfree(crtc_state_copy);
> > +	}
> > +
> >  	kfree(saved_mode);
> >  	return ret;
> >  }
> > @@ -11332,27 +11398,81 @@ static int intel_set_mode_pipes(struct drm_crtc
> > *crtc,
> > 
> >  static int intel_set_mode(struct drm_crtc *crtc,
> >  			  struct drm_display_mode *mode,
> > -			  int x, int y, struct drm_framebuffer *fb)
> > +			  int x, int y, struct drm_framebuffer *fb,
> > +			  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,
> > +	pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
> >  						   &modeset_pipes,
> >  						   &prepare_pipes,
> >  						   &disable_pipes);
> > 
> > -	if (IS_ERR(pipe_config))
> > -		return PTR_ERR(pipe_config);
> > +	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);
> > +	if (ret)
> > +		goto out;
> > 
> > -	return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> > -				    modeset_pipes, prepare_pipes,
> > -				    disable_pipes);
> > +out:
> > +	return ret;
> >  }
> > 
> >  void intel_crtc_restore_mode(struct drm_crtc *crtc)  {
> > -	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb);
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_atomic_state *state;
> > +	struct intel_encoder *encoder;
> > +	struct intel_connector *connector;
> > +	struct drm_connector_state *connector_state;
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state) {
> > +		DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of
> > memory",
> > +			      crtc->base.id);
> > +		return;
> > +	}
> > +
> > +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> > +
> > +	/* The force restore path in the HW readout code relies on the staged
> > +	 * config still keeping the user requested config while the actual
> > +	 * state has been overwritten by the configuration read from HW. We
> > +	 * need to copy the staged config to the atomic state, otherwise the
> > +	 * mode set will just reapply the state the HW is already in. */
> 
> We have discussed this in our call that
> drm_connector->crtc (state read out from hw)
> intel_encoder->new_crtc (that still has the old value)
> 
> but can you clarify why  below isn't 
> required before?

Before this patch series, the mode set code relies on the staged config,
i.e., intel_crtc->new_enable, intel_connector->new_encoder and
intel_encoder->new_crtc. These values are not changed during HW state
read out. After the series, the same code will look at the crtc and
connector states it gets from the drm_atomic_state.

The atomic state used for this mode set call was just allocated above.
Since the HW readout state replaces the current state
(drm_crtc->enabled; drm_connector->encoder; drm_encoder->crtc), the end
result is that any call to drm_atomic_get_{crtc,connector}_state will
use the *current HW state*. We want to write the old state (the state
requested by the user) to the drm_atomic_struct, so that after this the
HW state is the same as the old user requested state.

> > +	for_each_intel_encoder(dev, encoder) {
> > +		if (&encoder->new_crtc->base != crtc)
> > +			continue;
> > +
> > +		for_each_intel_connector(dev, connector) {
> > +			if (connector->new_encoder != encoder)
> > +				continue;
> > +
> > +			connector_state =
> > drm_atomic_get_connector_state(state, &connector->base);
> > +			if (IS_ERR(connector_state)) {
> > +				DRM_DEBUG_KMS("Failed to add
> > [CONNECTOR:%d:%s] to state: %ld\n",
> > +					      connector->base.base.id,
> > +					      connector->base.name,
> > +					      PTR_ERR(connector_state));
> > +				continue;
> > +			}
> > +
> > +			connector_state->crtc = crtc;
> > +			connector_state->best_encoder = &encoder->base;
> > +		}
> > +	}
> > +
> > +	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb,
> > +		       state);
> > +
> > +	drm_atomic_state_free(state);
> >  }
> > 
> >  #undef for_each_intel_crtc_masked
> > @@ -11677,6 +11797,7 @@ static int intel_crtc_set_config(struct
> > drm_mode_set *set)  {
> >  	struct drm_device *dev;
> >  	struct drm_mode_set save_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; @@ -11721,12
> > +11842,20 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  	 * such cases. */
> >  	intel_set_config_compute_mode_changes(set, config);
> > 
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state) {
> > +		ret = -ENOMEM;
> > +		goto out_config;
> > +	}
> > +
> > +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> > +
> >  	ret = intel_modeset_stage_output_state(dev, set, config);
> >  	if (ret)
> >  		goto fail;
> > 
> >  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
> > -						   set->fb,
> > +						   set->fb, state,
> >  						   &modeset_pipes,
> >  						   &prepare_pipes,
> >  						   &disable_pipes);
> > @@ -11746,10 +11875,6 @@ static int intel_crtc_set_config(struct
> > drm_mode_set *set)
> >  		 */
> >  	}
> > 
> > -	/* set_mode will free it in the mode_changed case */
> > -	if (!config->mode_changed)
> > -		kfree(pipe_config);
> > -
> >  	intel_update_pipe_size(to_intel_crtc(set->crtc));
> > 
> >  	if (config->mode_changed) {
> > @@ -11795,6 +11920,8 @@ static int intel_crtc_set_config(struct
> > drm_mode_set *set)
> >  fail:
> >  		intel_set_config_restore_state(dev, config);
> > 
> > +		drm_atomic_state_clear(state);
> > +
> >  		/*
> >  		 * HACK: if the pipe was on, but we didn't have a framebuffer,
> >  		 * force the pipe off to avoid oopsing in the modeset code @@
> > -11807,11 +11934,15 @@ fail:
> >  		/* Try to restore the config */
> >  		if (config->mode_changed &&
> >  		    intel_set_mode(save_set.crtc, save_set.mode,
> > -				   save_set.x, save_set.y, save_set.fb))
> > +				   save_set.x, save_set.y, save_set.fb,
> > +				   state))
> >  			DRM_ERROR("failed to restore config after modeset
> > failure\n");
> >  	}
> > 
> >  out_config:
> > +	if (state)
> > +		drm_atomic_state_free(state);
> > +
> >  	intel_set_config_free(config);
> >  	return ret;
> >  }
> > @@ -13852,8 +13983,7 @@ void intel_modeset_setup_hw_state(struct
> > drm_device *dev,
> >  			struct drm_crtc *crtc =
> >  				dev_priv->pipe_to_crtc_mapping[pipe];
> > 
> > -			intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> > -				       crtc->primary->fb);
> > +			intel_crtc_restore_mode(crtc);
> I see you are using intel_crtc_restore_mode instead of intel_set_mode()
> to acquire 
> In the code there is comment before above hunk:
>         /*
>          * We need to use raw interfaces for restoring state to avoid
>          * checking (bogus) intermediate states.
>          */
> May be this needs some refinement.

We have a separate task to convert this part of the code to atomic. Once
that is done, at this point we'll read the HW state into a
drm_atomic_struct and swap that with the current state. Then, we'll do
an atomic mode set with the swapped out state. This is just an
intermediary step to let us begin the conversion without breaking
things.

Ander

> >  		}
> >  	} else {
> >  		intel_modeset_update_staged_output_state(dev);
> > --
> > 2.1.0
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx