> -----Original Message-----
> From: Conselvan De Oliveira, Ander
> Sent: Friday, March 13, 2015 2:49 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
> Subject: [PATCH 03/19] 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 state 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)
>
> Signed-off-by: Ander Conselvan de Oliveira
> <ander.conselvan.de.oliveira@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 168 +++++++++++++++++++++++++++----
> ----
> 1 file changed, 133 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 78ea122..b61e3f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -37,6 +37,7 @@
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> #include "i915_trace.h"
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_dp_helper.h>
> #include <drm/drm_crtc_helper.h>
> @@ -82,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 +8804,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 +8886,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 +8914,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 +8933,11 @@ retry:
> else
> intel_crtc->new_config = NULL;
There are still occurrences of new_config, which you indicated killing them.
Will you be sending out another version?
Same applies to any new_xxx variables where they supposed to be in
respective states or take them out.
crtc->new_crtc
crtc->new_config
crtc->new_enabled
encoder->new_encoder
dpll->new_config
In __intel_set_mode_setup_plls() before calling crtc_compute_clock()
it is picking crtc_state from crtc->new_config. I think crtc_state should
be a parameter to __intel_set_mode_setup_plls() and then pass
further it to compute_clocks().
> fail_unlock:
> + if (state) {
> + drm_atomic_state_free(state);
> + state = NULL;
> + }
> +
> if (ret == -EDEADLK) {
> drm_modeset_backoff(ctx);
> goto retry;
> @@ -8936,22 +8950,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 +10371,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;
> +}
> +
> +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 +10396,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 +10503,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 +11183,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 +11203,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;
> +
> intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> "[modeset]");
>
> -out:
> return pipe_config;
> }
>
> @@ -11214,6 +11258,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 +11266,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 +11358,19 @@ 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 = intel_crtc->new_config = crtc_state_copy;
> + intel_crtc->base.state = &crtc_state_copy->base;
> + } else {
> + kfree(crtc_state_copy);
> + }
> +
> kfree(saved_mode);
> return ret;
> }
> @@ -11332,27 +11396,51 @@ 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;
> +
> + 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;
> +
> + 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 +11765,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
> +11810,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 +11843,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 +11888,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 +11902,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 +13951,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);
> }
> } 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
