[09/42] drm/i915: Make intel_modeset_fixup_state similar to the atomic helper.
diff mbox

Message ID 1431354318-11995-10-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 11, 2015, 2:24 p.m. UTC
This should be safe.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 82 ++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

Comments

Daniel Vetter May 12, 2015, 6:59 a.m. UTC | #1
On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote:
> This should be safe.

Usual request: A few more details about what you've changed to help guide
the review would be great. E.g. which functions from the atomic helpers
you're trying to copy here exactly.

It looks like this models set_routing_links. I think it would be rather
useful to expose this to drivers as a helper function, maybe with a more
useful name like drm_atomic_helper_update_legacy_state or similar.

Another thing I've noticed is that atomic helpers lost the call to
drm_calc_timestamping_constants. Would be good to add that to the same
function.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 82 ++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a21b2e51c054..956c9964275d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11084,36 +11084,48 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>  	}
>  }
>  
> -/* Fixup legacy state after an atomic state swap.
> +/*
> + * Fixup legacy state in a similar way to
> + * drm_atomic_helper.c:set_routing_links.
>   */
>  static void intel_modeset_fixup_state(struct drm_atomic_state *state)
>  {
> -	struct intel_crtc *crtc;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> +	struct drm_connector *connector;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	int i;
>  
> -	for_each_intel_connector(state->dev, connector) {
> -		connector->base.encoder = connector->base.state->best_encoder;
> -		if (connector->base.encoder)
> -			connector->base.encoder->crtc =
> -				connector->base.state->crtc;
> +	/*
> +	 * swap crtc and connector and update legacy state,
> +	 * plane state already gets swapped
> +	 * by the plane helpers. Once .crtc_disable is fixed
> +	 * all state should be swapped before disabling crtc's.
> +	 */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		crtc->enabled = crtc->state->enable;
> +		crtc->mode = crtc->state->mode;
>  	}
>  
> -	/* Update crtc of disabled encoders */
> -	for_each_intel_encoder(state->dev, encoder) {
> -		int num_connectors = 0;
> -
> -		for_each_intel_connector(state->dev, connector)
> -			if (connector->base.encoder == &encoder->base)
> -				num_connectors++;
> +	/* clear out existing links */
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		if (!connector->encoder)
> +			continue;
>  
> -		if (num_connectors == 0)
> -			encoder->base.crtc = NULL;
> +		WARN_ON(!connector->encoder->crtc);
> +		connector->encoder->crtc = NULL;
> +		connector->encoder = NULL;
>  	}
>  
> -	for_each_intel_crtc(state->dev, crtc) {
> -		crtc->base.enabled = crtc->base.state->enable;
> -		crtc->config = to_intel_crtc_state(crtc->base.state);
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		if (!connector->state->crtc)
> +			continue;
> +
> +		if (WARN_ON(!connector->state->best_encoder))
> +			continue;
> +
> +		connector->encoder = connector->state->best_encoder;
> +		connector->encoder->crtc = connector->state->crtc;
>  	}
>  }
>  
> @@ -11559,9 +11571,16 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  
>  	intel_modeset_fixup_state(state);
>  
> -	/* Double check state. */
>  	for_each_crtc(dev, crtc) {
> +		/* Double check state. */
>  		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
> +
> +		if (!state->crtcs[drm_crtc_index(crtc)])
> +			continue;
> +
> +		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
> +		if (crtc->state->active && needs_modeset(crtc->state))
> +			drm_calc_timestamping_constants(crtc, &crtc->state->adjusted_mode);
>  	}
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -12277,25 +12296,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  			drm_plane_helper_disable(crtc->primary);
>  	}
>  
> -	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
> -	 * to set it here already despite that we pass it down the callchain.
> -	 *
> -	 * Note we'll need to fix this up when we start tracking multiple
> -	 * pipes; here we assume a single modeset_pipe and only track the
> -	 * single crtc and mode.
> -	 */
> -	if (pipe_config->base.active && needs_modeset(&pipe_config->base)) {
> -		modeset_crtc->mode = pipe_config->base.mode;
> -
> -		/*
> -		 * Calculate and store various constants which
> -		 * are later needed by vblank and swap-completion
> -		 * timestamping. They are derived from true hwmode.
> -		 */
> -		drm_calc_timestamping_constants(modeset_crtc,
> -						&pipe_config->base.adjusted_mode);
> -	}
> -
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
>  	intel_modeset_update_state(state);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst May 12, 2015, 12:41 p.m. UTC | #2
Op 12-05-15 om 08:59 schreef Daniel Vetter:
> On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote:
>> This should be safe.
> Usual request: A few more details about what you've changed to help guide
> the review would be great. E.g. which functions from the atomic helpers
> you're trying to copy here exactly.
That's in the diff. :-)
> It looks like this models set_routing_links. I think it would be rather
> useful to expose this to drivers as a helper function, maybe with a more
> useful name like drm_atomic_helper_update_legacy_state or similar.
I think you're right about this one.
> Another thing I've noticed is that atomic helpers lost the call to
> drm_calc_timestamping_constants. Would be good to add that to the same
> function.
Yeah, but the precise vblanking stuff is not really atomic friendly,
so I'm not sure it should be done outside the driver.
Daniel Vetter May 12, 2015, 1:18 p.m. UTC | #3
On Tue, May 12, 2015 at 02:41:17PM +0200, Maarten Lankhorst wrote:
> Op 12-05-15 om 08:59 schreef Daniel Vetter:
> > On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote:
> >> This should be safe.
> > Usual request: A few more details about what you've changed to help guide
> > the review would be great. E.g. which functions from the atomic helpers
> > you're trying to copy here exactly.
> That's in the diff. :-)

Indeed ;-)

> > It looks like this models set_routing_links. I think it would be rather
> > useful to expose this to drivers as a helper function, maybe with a more
> > useful name like drm_atomic_helper_update_legacy_state or similar.
> I think you're right about this one.
> > Another thing I've noticed is that atomic helpers lost the call to
> > drm_calc_timestamping_constants. Would be good to add that to the same
> > function.
> Yeah, but the precise vblanking stuff is not really atomic friendly,
> so I'm not sure it should be done outside the driver.

Hm what's unfriendly about it? It's definitely a regression to not update
the constants needed by it compared to crtc helpers. I'll go about and fix
that now.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a21b2e51c054..956c9964275d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11084,36 +11084,48 @@  static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 	}
 }
 
-/* Fixup legacy state after an atomic state swap.
+/*
+ * Fixup legacy state in a similar way to
+ * drm_atomic_helper.c:set_routing_links.
  */
 static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 {
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
+	struct drm_connector *connector;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc;
+	int i;
 
-	for_each_intel_connector(state->dev, connector) {
-		connector->base.encoder = connector->base.state->best_encoder;
-		if (connector->base.encoder)
-			connector->base.encoder->crtc =
-				connector->base.state->crtc;
+	/*
+	 * swap crtc and connector and update legacy state,
+	 * plane state already gets swapped
+	 * by the plane helpers. Once .crtc_disable is fixed
+	 * all state should be swapped before disabling crtc's.
+	 */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		crtc->enabled = crtc->state->enable;
+		crtc->mode = crtc->state->mode;
 	}
 
-	/* Update crtc of disabled encoders */
-	for_each_intel_encoder(state->dev, encoder) {
-		int num_connectors = 0;
-
-		for_each_intel_connector(state->dev, connector)
-			if (connector->base.encoder == &encoder->base)
-				num_connectors++;
+	/* clear out existing links */
+	for_each_connector_in_state(state, connector, conn_state, i) {
+		if (!connector->encoder)
+			continue;
 
-		if (num_connectors == 0)
-			encoder->base.crtc = NULL;
+		WARN_ON(!connector->encoder->crtc);
+		connector->encoder->crtc = NULL;
+		connector->encoder = NULL;
 	}
 
-	for_each_intel_crtc(state->dev, crtc) {
-		crtc->base.enabled = crtc->base.state->enable;
-		crtc->config = to_intel_crtc_state(crtc->base.state);
+	for_each_connector_in_state(state, connector, conn_state, i) {
+		if (!connector->state->crtc)
+			continue;
+
+		if (WARN_ON(!connector->state->best_encoder))
+			continue;
+
+		connector->encoder = connector->state->best_encoder;
+		connector->encoder->crtc = connector->state->crtc;
 	}
 }
 
@@ -11559,9 +11571,16 @@  intel_modeset_update_state(struct drm_atomic_state *state)
 
 	intel_modeset_fixup_state(state);
 
-	/* Double check state. */
 	for_each_crtc(dev, crtc) {
+		/* Double check state. */
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
+
+		if (!state->crtcs[drm_crtc_index(crtc)])
+			continue;
+
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+		if (crtc->state->active && needs_modeset(crtc->state))
+			drm_calc_timestamping_constants(crtc, &crtc->state->adjusted_mode);
 	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -12277,25 +12296,6 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			drm_plane_helper_disable(crtc->primary);
 	}
 
-	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
-	 * to set it here already despite that we pass it down the callchain.
-	 *
-	 * Note we'll need to fix this up when we start tracking multiple
-	 * pipes; here we assume a single modeset_pipe and only track the
-	 * single crtc and mode.
-	 */
-	if (pipe_config->base.active && needs_modeset(&pipe_config->base)) {
-		modeset_crtc->mode = pipe_config->base.mode;
-
-		/*
-		 * Calculate and store various constants which
-		 * are later needed by vblank and swap-completion
-		 * timestamping. They are derived from true hwmode.
-		 */
-		drm_calc_timestamping_constants(modeset_crtc,
-						&pipe_config->base.adjusted_mode);
-	}
-
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
 	intel_modeset_update_state(state);