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

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

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.oliveira@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.

> 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.

> 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:

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