> -----Original Message-----
> From: Conselvan De Oliveira, Ander
> Sent: Wednesday, March 18, 2015 12:57 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> 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?
>
> Signed-off-by: Ander Conselvan de Oliveira
> <ander.conselvan.de.oliveira@xxxxxxxxx>
> ---
> 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()?
> 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.
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?
> + 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.
> }
> } else {
> intel_modeset_update_staged_output_state(dev);
> --
> 2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
