[RFC,17/37] DRM: Atomic: Use pointer for mode in CRTC state

Commit Message

Daniel Stone March 19, 2015, 4:33 a.m. UTC

Holding a pointer to the mode, rather than an embed, allows us to get
towards sharing refcounted modes.

XXX: atomic_destroy_state does _not_ seem to be optional - so we should
     remove any fallback paths which compensate for its lack!
     the crtc_state->mode handling is particularly ugly here :\

Signed-off-by: Daniel Stone <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c       | 35 +++++++++++++++++++++++--------
 drivers/gpu/drm/drm_crtc.c                |  2 +-
 drivers/gpu/drm/drm_crtc_helper.c         | 32 ++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_atomic.c       | 18 +++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c |  2 +-
 drivers/gpu/drm/tegra/dc.c                |  8 +++++++
 drivers/gpu/drm/tegra/dsi.c               |  4 ++--
 drivers/gpu/drm/tegra/hdmi.c              |  2 +-
 drivers/gpu/drm/tegra/rgb.c               |  2 +-
 drivers/gpu/drm/tegra/sor.c               |  2 +-
 include/drm/drm_crtc.h                    |  2 +-
 12 files changed, 88 insertions(+), 23 deletions(-)

Comments

Daniel Vetter March 20, 2015, 4:16 p.m. UTC | #1

On Thu, Mar 19, 2015 at 04:33:16AM +0000, Daniel Stone wrote:
> Holding a pointer to the mode, rather than an embed, allows us to get
> towards sharing refcounted modes.
> 
> XXX: atomic_destroy_state does _not_ seem to be optional - so we should
>      remove any fallback paths which compensate for its lack!
>      the crtc_state->mode handling is particularly ugly here :\

duplicate/destroy callbacks are optional in the transitional helpers. And
that's fairly intentional to avoid the need for switching to the full
state scaffolding at once.

For these couldn't we instead just store a pointer to crtc->mode instead?
Lifetimes should be fully in sync.

> @@ -2058,11 +2058,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>   */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
>  {
> +	if (crtc->state)
> +		kfree(crtc->state->mode);
> +
>  	kfree(crtc->state);
>  	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
>  
> -	if (crtc->state)
> +	if (crtc->state) {
>  		crtc->state->crtc = crtc;
> +		crtc->state->mode =
> +			kzalloc(sizeof(*crtc->state->mode), GFP_KERNEL);

Allocating an empty mode object seems superflous. Why do we need this?

> +	}
> +
> +	if (crtc->state && !crtc->state->mode) {
> +		kfree(crtc->state);
> +		crtc->state = NULL;
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
>  
> @@ -2088,6 +2099,11 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
>  		state->active_changed = false;
>  		state->planes_changed = false;
>  		state->event = NULL;
> +		state->mode = drm_mode_duplicate(crtc->dev, crtc->state->mode);
> +		if (!state->mode) {
> +			kfree(state);
> +			state = NULL;
> +		}
>  	}
>  
>  	return state;
> @@ -2105,6 +2121,7 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>  void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  					  struct drm_crtc_state *state)
>  {
> +	kfree(state->mode);
>  	kfree(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5785336..6023851 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2008,7 +2008,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  		crtc_resp->x = crtc->primary->state->src_x >> 16;
>  		crtc_resp->y = crtc->primary->state->src_y >> 16;
>  		if (crtc->state->enable) {
> -			drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> +			drm_crtc_convert_to_umode(&crtc_resp->mode, crtc->state->mode);
>  			crtc_resp->mode_valid = 1;
>  
>  		} else {
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index c6063ff..8a9a045 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -943,11 +943,32 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	if (crtc->funcs->atomic_duplicate_state)
>  		crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
> -	else if (crtc->state)
> +	else if (crtc->state) {
>  		crtc_state = kmemdup(crtc->state, sizeof(*crtc_state),
>  				     GFP_KERNEL);
> -	else
> +		/* XXX: this is unpleasant: we should mandate dup instead */
> +		if (crtc_state) {
> +			crtc_state->mode =
> +				drm_mode_duplicate(crtc->dev,
> +				                   crtc->state->mode);
> +			if (!crtc_state->mode) {
> +				kfree(crtc_state);
> +				crtc_state = NULL;
> +			}
> +		}
> +	}
> +	else {
>  		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> +		if (crtc_state) {
> +			crtc_state->mode = kzalloc(sizeof(*crtc_state->mode),
> +			                           GFP_KERNEL);
> +			/* XXX: as above, but mandate a new_state */
> +			if (!crtc_state->mode) {
> +				kfree(crtc_state);
> +				crtc_state = NULL;
> +			}
> +		}
> +	}

I think for transitional helpers if we just do a crtc_state->mode =
crtc->mode that should be all that's needed.
-Daniel

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5bcb4ba..962443d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -260,7 +260,7 @@  mode_fixup(struct drm_atomic_state *state)
 		if (!crtc_state || !crtc_state->mode_changed)
 			continue;
 
-		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
+		drm_mode_copy(&crtc_state->adjusted_mode, crtc_state->mode);
 	}
 
 	for (i = 0; i < state->num_connector; i++) {
@@ -289,7 +289,7 @@  mode_fixup(struct drm_atomic_state *state)
 
 		if (encoder->bridge && encoder->bridge->funcs->mode_fixup) {
 			ret = encoder->bridge->funcs->mode_fixup(
-					encoder->bridge, &crtc_state->mode,
+					encoder->bridge, crtc_state->mode,
 					&crtc_state->adjusted_mode);
 			if (!ret) {
 				DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
@@ -306,7 +306,7 @@  mode_fixup(struct drm_atomic_state *state)
 				return ret;
 			}
 		} else {
-			ret = funcs->mode_fixup(encoder, &crtc_state->mode,
+			ret = funcs->mode_fixup(encoder, crtc_state->mode,
 						&crtc_state->adjusted_mode);
 			if (!ret) {
 				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] fixup failed\n",
@@ -327,7 +327,7 @@  mode_fixup(struct drm_atomic_state *state)
 			continue;
 
 		funcs = crtc->helper_private;
-		ret = funcs->mode_fixup(crtc, &crtc_state->mode,
+		ret = funcs->mode_fixup(crtc, crtc_state->mode,
 					&crtc_state->adjusted_mode);
 		if (!ret) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d] fixup failed\n",
@@ -383,7 +383,7 @@  drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (!crtc)
 			continue;
 
-		if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {
+		if (!drm_mode_equal(crtc->state->mode, crtc_state->mode)) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d] mode changed\n",
 					 crtc->base.id);
 			crtc_state->mode_changed = true;
@@ -699,7 +699,7 @@  set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (!crtc)
 			continue;
 
-		crtc->mode = crtc->state->mode;
+		crtc->mode = *crtc->state->mode;
 		crtc->enabled = crtc->state->enable;
 		crtc->x = crtc->primary->state->src_x >> 16;
 		crtc->y = crtc->primary->state->src_y >> 16;
@@ -746,7 +746,7 @@  crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
 		new_crtc_state = connector->state->crtc->state;
-		mode = &new_crtc_state->mode;
+		mode = new_crtc_state->mode;
 		adjusted_mode = &new_crtc_state->adjusted_mode;
 
 		if (!new_crtc_state->mode_changed)
@@ -1643,7 +1643,7 @@  retry:
 
 	crtc_state->enable = true;
 	crtc_state->active = true;
-	drm_mode_copy(&crtc_state->mode, set->mode);
+	drm_mode_copy(crtc_state->mode, set->mode);
 
 	ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
 	if (ret != 0)
@@ -2058,11 +2058,22 @@  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
  */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
 {
+	if (crtc->state)
+		kfree(crtc->state->mode);
+
 	kfree(crtc->state);
 	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
 
-	if (crtc->state)
+	if (crtc->state) {
 		crtc->state->crtc = crtc;
+		crtc->state->mode =
+			kzalloc(sizeof(*crtc->state->mode), GFP_KERNEL);
+	}
+
+	if (crtc->state && !crtc->state->mode) {
+		kfree(crtc->state);
+		crtc->state = NULL;
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
 
@@ -2088,6 +2099,11 @@  drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
 		state->active_changed = false;
 		state->planes_changed = false;
 		state->event = NULL;
+		state->mode = drm_mode_duplicate(crtc->dev, crtc->state->mode);
+		if (!state->mode) {
+			kfree(state);
+			state = NULL;
+		}
 	}
 
 	return state;
@@ -2105,6 +2121,7 @@  EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
 void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 					  struct drm_crtc_state *state)
 {
+	kfree(state->mode);
 	kfree(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5785336..6023851 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2008,7 +2008,7 @@  int drm_mode_getcrtc(struct drm_device *dev,
 		crtc_resp->x = crtc->primary->state->src_x >> 16;
 		crtc_resp->y = crtc->primary->state->src_y >> 16;
 		if (crtc->state->enable) {
-			drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
+			drm_crtc_convert_to_umode(&crtc_resp->mode, crtc->state->mode);
 			crtc_resp->mode_valid = 1;
 
 		} else {
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index c6063ff..8a9a045 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -943,11 +943,32 @@  int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
 
 	if (crtc->funcs->atomic_duplicate_state)
 		crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
-	else if (crtc->state)
+	else if (crtc->state) {
 		crtc_state = kmemdup(crtc->state, sizeof(*crtc_state),
 				     GFP_KERNEL);
-	else
+		/* XXX: this is unpleasant: we should mandate dup instead */
+		if (crtc_state) {
+			crtc_state->mode =
+				drm_mode_duplicate(crtc->dev,
+				                   crtc->state->mode);
+			if (!crtc_state->mode) {
+				kfree(crtc_state);
+				crtc_state = NULL;
+			}
+		}
+	}
+	else {
 		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
+		if (crtc_state) {
+			crtc_state->mode = kzalloc(sizeof(*crtc_state->mode),
+			                           GFP_KERNEL);
+			/* XXX: as above, but mandate a new_state */
+			if (!crtc_state->mode) {
+				kfree(crtc_state);
+				crtc_state = NULL;
+			}
+		}
+	}
 	if (!crtc_state)
 		return -ENOMEM;
 	crtc_state->crtc = crtc;
@@ -955,12 +976,13 @@  int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
 	crtc_state->enable = true;
 	crtc_state->planes_changed = true;
 	crtc_state->mode_changed = true;
-	drm_mode_copy(&crtc_state->mode, mode);
+	drm_mode_copy(crtc_state->mode, mode);
 	drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
 
 	if (crtc_funcs->atomic_check) {
 		ret = crtc_funcs->atomic_check(crtc, crtc_state);
 		if (ret) {
+			kfree(crtc_state->mode);
 			kfree(crtc_state);
 
 			return ret;
@@ -974,8 +996,10 @@  int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
 	if (crtc_state) {
 		if (crtc->funcs->atomic_destroy_state)
 			crtc->funcs->atomic_destroy_state(crtc, crtc_state);
-		else
+		else {
+			kfree(crtc_state->mode);
 			kfree(crtc_state);
+		}
 	}
 
 	return drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3903b90..c479386 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -222,9 +222,25 @@  intel_crtc_duplicate_state(struct drm_crtc *crtc)
 		crtc_state = kmemdup(intel_crtc->config,
 				     sizeof(*intel_crtc->config), GFP_KERNEL);
 
-	if (crtc_state)
+	if (crtc_state) {
 		crtc_state->base.crtc = crtc;
 
+		/* XXX: this is tedious */
+		if (intel_crtc->config) {
+			crtc_state->mode =
+				drm_mode_duplicate(crtc->dev,
+						   intel_crtc->config->mode);
+		} else {
+			crtc_state->mode =
+				kzalloc(sizeof(*crtc_state->mode), GFP_KERNEL);
+		}
+
+		if (!crtc_state->mode) {
+			kfree(crtc_state);
+			crtc_state = NULL;
+		}
+	}
+
 	return &crtc_state->base;
 }
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index 0db6a54..14d74b4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -64,7 +64,7 @@  static int rcar_du_encoder_atomic_check(struct drm_encoder *encoder,
 {
 	struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
-	const struct drm_display_mode *mode = &crtc_state->mode;
+	const struct drm_display_mode *mode = crtc_state->mode;
 	const struct drm_display_mode *panel_mode;
 	struct drm_connector *connector = conn_state->connector;
 	struct drm_device *dev = encoder->dev;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c b/drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c
index c000850..1c9b6f0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c
@@ -69,7 +69,7 @@  static int rcar_du_hdmienc_atomic_check(struct drm_encoder *encoder,
 	struct rcar_du_hdmienc *hdmienc = to_rcar_hdmienc(encoder);
 	struct drm_encoder_slave_funcs *sfuncs = to_slave_funcs(encoder);
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
-	const struct drm_display_mode *mode = &crtc_state->mode;
+	const struct drm_display_mode *mode = crtc_state->mode;
 
 	/* The internal LVDS encoder has a clock frequency operating range of
 	 * 30MHz to 150MHz. Clamp the clock accordingly.
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b7f7815..40f6e74 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1015,6 +1015,13 @@  tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
 	if (!copy)
 		return NULL;
 
+	/* XXX: tedium */
+	copy->base.mode = drm_mode_duplicate(crtc->dev, state->base.mode);
+	if (!copy->base.mode) {
+		kfree(copy);
+		return NULL;
+	}
+
 	copy->base.mode_changed = false;
 	copy->base.active_changed = false;
 	copy->base.planes_changed = false;
@@ -1026,6 +1033,7 @@  tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
 static void tegra_crtc_atomic_destroy_state(struct drm_crtc *crtc,
 					    struct drm_crtc_state *state)
 {
+	kfree(state->mode);
 	kfree(state);
 }
 
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 4714eb4..cfd4da7 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -887,7 +887,7 @@  tegra_dsi_encoder_atomic_check(struct drm_encoder *encoder,
 	unsigned long plld;
 	int err;
 
-	state->pclk = crtc_state->mode.clock * 1000;
+	state->pclk = crtc_state->mode->clock * 1000;
 
 	err = tegra_dsi_get_muldiv(dsi->format, &state->mul, &state->div);
 	if (err < 0)
@@ -899,7 +899,7 @@  tegra_dsi_encoder_atomic_check(struct drm_encoder *encoder,
 	if (err < 0)
 		return err;
 
-	state->vrefresh = drm_mode_vrefresh(&crtc_state->mode);
+	state->vrefresh = drm_mode_vrefresh(crtc_state->mode);
 
 	/* compute byte clock */
 	state->bclk = (state->pclk * state->mul) / (state->div * state->lanes);
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index 68d315e..d80de91 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1060,7 +1060,7 @@  tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 {
 	struct tegra_output *output = encoder_to_output(encoder);
 	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
-	unsigned long pclk = crtc_state->mode.clock * 1000;
+	unsigned long pclk = crtc_state->mode->clock * 1000;
 	struct tegra_hdmi *hdmi = to_hdmi(output);
 	int err;
 
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 745759a..5b4d86a 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -196,7 +196,7 @@  tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
 {
 	struct tegra_output *output = encoder_to_output(encoder);
 	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
-	unsigned long pclk = crtc_state->mode.clock * 1000;
+	unsigned long pclk = crtc_state->mode->clock * 1000;
 	struct tegra_rgb *rgb = to_rgb(output);
 	unsigned int div;
 	int err;
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index fe86207..d0eaa48 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1282,7 +1282,7 @@  tegra_sor_encoder_atomic_check(struct drm_encoder *encoder,
 {
 	struct tegra_output *output = encoder_to_output(encoder);
 	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
-	unsigned long pclk = crtc_state->mode.clock * 1000;
+	unsigned long pclk = crtc_state->mode->clock * 1000;
 	struct tegra_sor *sor = to_sor(output);
 	int err;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7b141d837..2bce96e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -294,7 +294,7 @@  struct drm_crtc_state {
 	/* adjusted_mode: for use by helpers and drivers */
 	struct drm_display_mode adjusted_mode;
 
-	struct drm_display_mode mode;
+	struct drm_display_mode *mode;
 
 	struct drm_pending_vblank_event *event;