> -----Original Message-----
> From: Ander Conselvan De Oliveira [mailto:conselv...@gmail.com]
> Sent: Friday, March 20, 2015 12:00 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org
> 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@lists.freedesktop.org
> > > 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.olive...@intel.com>
> > > ---
> > > 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
