> -----Original Message----- > From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx] > Sent: Friday, March 20, 2015 12:00 AM > To: Konduru, Chandra > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 03/19] drm/i915: Allocate a drm_atomic_state for the > legacy modeset code > > On Thu, 2015-03-19 at 21:08 +0000, Konduru, Chandra wrote: > > > > > -----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? > > I indicated I have the intention of killing crtc->new_config, and I do have > patches in my private branch to that extent. However, I don't think those > patches are needed by this series, so I rather get this in first. That is fine if they are coming in next one. > > > 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 > > I'm working on removing the usage of all those variables, and I'll follow up with > more patches. But we can't completely remove the staged config util the > hardware state readout code is converted to atomic, since the force_restore > path relies on the staged config containing the "user requested" state. > > This patch series is not all that is needed to remove the staged config, but a big > step in the right direction. The important bit here is not whether or not the > staged config still exists, but the fact that new code can be written to not rely on > it. If these things are binned for next series that is fine. > > > 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(). > > I have the following patch in a private branch: Looks ok, but it would have less typing and code if crtc_state is simply passed to it rather than calling helper to get it. > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 43d0e43..1f1878f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11403,10 +11403,11 @@ intel_modeset_compute_config(struct drm_crtc > *crtc, > return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); } > > -static int __intel_set_mode_setup_plls(struct drm_device *dev, > +static int __intel_set_mode_setup_plls(struct drm_atomic_state *state, > unsigned modeset_pipes, > unsigned disable_pipes) { > + struct drm_device *dev = state->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > unsigned clear_pipes = modeset_pipes | disable_pipes; > struct intel_crtc *intel_crtc; > @@ -11420,9 +11421,11 @@ static int __intel_set_mode_setup_plls(struct > drm_device *dev, > goto done; > > for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) { > - struct intel_crtc_state *state = intel_crtc->new_config; > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_crtc_state(state, intel_crtc); > + > ret = dev_priv->display.crtc_compute_clock(intel_crtc, > - state); > + crtc_state); > if (ret) { > intel_shared_dpll_abort_config(dev_priv); > goto done; > @@ -11480,7 +11483,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > prepare_pipes &= ~disable_pipes; > } > > - ret = __intel_set_mode_setup_plls(dev, modeset_pipes, disable_pipes); > + ret = __intel_set_mode_setup_plls(state, modeset_pipes, > + disable_pipes); > if (ret) > goto done; > > Ander > > > > > > > > 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
