drm/i915: Postpone plane readout until after encoder readout

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Hey,

Op 31-07-15 om 15:04 schreef Patrik Jakobsson:

Quoted 8 lines

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Running this on skylake it looks like like it's a partial fix. You would need to move hwmode like below too?

Also:

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428

drivers/gpu/drm/i915/intel_display.c +1 -2

index add7f807a82d..49907f4e939a 100644
@@ -14888,8 +14888,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
                        crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
                }
 
-               crtc->base.hwmode = crtc->config->base.adjusted_mode;
-
                DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
                              crtc->base.base.id,
                              crtc->active ? "enabled" : "disabled");

@@ -14971,6 +14969,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 
        for_each_pipe(dev_priv, pipe) {
                crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+               crtc->base.hwmode = crtc->config->base.adjusted_mode;
 
                for_each_intel_plane(crtc->base.dev, plane) {
                        if (crtc->pipe != plane->pipe)

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Patrik Jakobsson <[email protected]>

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Changes since v1:
- Set all modes after all state is read out. (Maarten)

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428

Signed-off-by Patrik Jakobsson <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

---

drivers/gpu/drm/i915/intel_display.c +48 -44

index df237ad4a780..85d52158d476 100644
@@ -15042,27 +15042,25 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 	return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
 }
 
-static void readout_plane_state(struct intel_crtc *crtc,
-				struct intel_crtc_state *crtc_state)
+static void intel_sanitize_plane(struct intel_plane *plane)
 {
-	struct intel_plane *p;
 	struct intel_plane_state *plane_state;
-	bool active = crtc_state->base.active;
+	struct intel_crtc *crtc;
 
-	for_each_intel_plane(crtc->base.dev, p) {
-		if (crtc->pipe != p->pipe)
-			continue;
+	plane_state = to_intel_plane_state(plane->base.state);
 
-		plane_state = to_intel_plane_state(p->base.state);
+	if (!plane_state->base.crtc)
+		return;
 
-		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
-			plane_state->visible = primary_get_hw_state(crtc);
-		else {
-			if (active)
-				p->disable_plane(&p->base, &crtc->base);
+	crtc = to_intel_crtc(plane_state->base.crtc);
 
-			plane_state->visible = false;
-		}
+	if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) {
+		plane_state->visible = primary_get_hw_state(crtc);
+	} else {
+		if (crtc->base.state->active)
+			plane->disable_plane(&plane->base, &crtc->base);
+
+		plane_state->visible = false;
 	}
 }
 

@@ -15086,35 +15084,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
-		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
-		if (crtc->base.state->active) {
-			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
-			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
-			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
-
-			/*
-			 * The initial mode needs to be set in order to keep
-			 * the atomic core happy. It wants a valid mode if the
-			 * crtc's enabled, so we do the above call.
-			 *
-			 * At this point some state updated by the connectors
-			 * in their ->detect() callback has not run yet, so
-			 * no recalculation can be done yet.
-			 *
-			 * Even if we could do a recalculation and modeset
-			 * right now it would cause a double modeset if
-			 * fbdev or userspace chooses a different initial mode.
-			 *
-			 * If that happens, someone indicated they wanted a
-			 * mode change, which means it's safe to do a full
-			 * recalculation.
-			 */
-			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
-		}
-
-		crtc->base.hwmode = crtc->config->base.adjusted_mode;
-		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
-
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,
 			      crtc->active ? "enabled" : "disabled");

@@ -15184,6 +15153,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 	enum pipe pipe;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
+	struct intel_plane *plane;
 	int i;
 
 	intel_modeset_readout_hw_state(dev);

@@ -15195,6 +15165,40 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 
 	for_each_pipe(dev_priv, pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
+		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
+		if (crtc->base.state->active) {
+			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
+			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
+			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
+
+			/*
+			 * The initial mode needs to be set in order to keep
+			 * the atomic core happy. It wants a valid mode if the
+			 * crtc's enabled, so we do the above call.
+			 *
+			 * At this point some state updated by the connectors
+			 * in their ->detect() callback has not run yet, so
+			 * no recalculation can be done yet.
+			 *
+			 * Even if we could do a recalculation and modeset
+			 * right now it would cause a double modeset if
+			 * fbdev or userspace chooses a different initial mode.
+			 *
+			 * If that happens, someone indicated they wanted a
+			 * mode change, which means it's safe to do a full
+			 * recalculation.
+			 */
+			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
+		}
+
+		for_each_intel_plane(crtc->base.dev, plane) {
+			if (crtc->pipe != plane->pipe)
+				continue;
+
+			intel_sanitize_plane(plane);
+		}
+
 		intel_sanitize_crtc(crtc);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 10 lines

From: Patrik Jakobsson <[email protected]>

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Quoted 6 lines

Changes since v1:
- Set all modes after all state is read out. (Maarten)

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428

Signed-off-by Patrik Jakobsson <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

Hm I still think we have a bit a mess here - the plane readout code is still spread out between modeset_readout_hw_state and the per-plane loop in intel_modeset_init after setup_hw_state.

I thought that we only ever need to do the plane state readout on initial load (they're all off on resume anyway and we force a full modeset to make sure plane state is correct again). Can't we just have a setup_plane_state which has that loop from the end of modeset_init with all the other plane state unified there?

Hey,

Op 26-08-15 om 16:43 schreef Daniel Vetter:

Quoted 21 lines · Maarten Lankhorst

Quoted 10 lines

From: Patrik Jakobsson <[email protected]>

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Quoted 6 lines

Changes since v1:
- Set all modes after all state is read out. (Maarten)

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428

Signed-off-by Patrik Jakobsson <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

Hm I still think we have a bit a mess here - the plane readout code is still spread out between modeset_readout_hw_state and the per-plane loop in intel_modeset_init after setup_hw_state.

Quoted 6 lines

I thought that we only ever need to do the plane state readout on initial load (they're all off on resume anyway and we force a full modeset to make sure plane state is correct again). Can't we just have a setup_plane_state which has that loop from the end of modeset_init with all the other plane state unified there?

Perhaps, but intel_crtc_disable_noatomic cares about visibility state.

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Signed-off-by Patrik Jakobsson <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428
---

drivers/gpu/drm/i915/intel_display.c +0 -45

index e3922d973db0..118b205e7bd5 100644
@@ -15033,30 +15033,6 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 	return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
 }
 
-static void readout_plane_state(struct intel_crtc *crtc,
-				struct intel_crtc_state *crtc_state)
-{
-	struct intel_plane *p;
-	struct intel_plane_state *plane_state;
-	bool active = crtc_state->base.active;
-
-	for_each_intel_plane(crtc->base.dev, p) {
-		if (crtc->pipe != p->pipe)
-			continue;
-
-		plane_state = to_intel_plane_state(p->base.state);
-
-		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
-			plane_state->visible = primary_get_hw_state(crtc);
-		else {
-			if (active)
-				p->disable_plane(&p->base, &crtc->base);
-
-			plane_state->visible = false;
-		}
-	}
-}
-
 static void intel_modeset_readout_hw_state(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -15076,35 +15052,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
-
-		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
-		if (crtc->base.state->active) {
-			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
-			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
-			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
-
-			/*
-			 * The initial mode needs to be set in order to keep
-			 * the atomic core happy. It wants a valid mode if the
-			 * crtc's enabled, so we do the above call.
-			 *
-			 * At this point some state updated by the connectors
-			 * in their ->detect() callback has not run yet, so
-			 * no recalculation can be done yet.
-			 *
-			 * Even if we could do a recalculation and modeset
-			 * right now it would cause a double modeset if
-			 * fbdev or userspace chooses a different initial mode.
-			 *
-			 * If that happens, someone indicated they wanted a

drivers/gpu/drm/i915/intel_display.c +17 -0

 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,

@@ -15186,6 +15135,33 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 
 	for_each_pipe(dev_priv, pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
+		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
+		if (crtc->base.state->active) {
+			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
+			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
+			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
+
+			/*
+			 * The initial mode needs to be set in order to keep
+			 * the atomic core happy. It wants a valid mode if the
+			 * crtc's enabled, so we do the above call.
+			 *
+			 * At this point some state updated by the connectors
+			 * in their ->detect() callback has not run yet, so
+			 * no recalculation can be done yet.
+			 *
+			 * Even if we could do a recalculation and modeset

drivers/gpu/drm/i915/intel_display.c +0 -0

 		intel_sanitize_crtc(crtc);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Signed-off-by Patrik Jakobsson <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428

---

drivers/gpu/drm/i915/intel_display.c +29 -53

index e3922d973db0..118b205e7bd5 100644
@@ -15033,30 +15033,6 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 	return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
 }
 
-static void readout_plane_state(struct intel_crtc *crtc,
-				struct intel_crtc_state *crtc_state)
-{
-	struct intel_plane *p;
-	struct intel_plane_state *plane_state;
-	bool active = crtc_state->base.active;
-
-	for_each_intel_plane(crtc->base.dev, p) {
-		if (crtc->pipe != p->pipe)
-			continue;
-
-		plane_state = to_intel_plane_state(p->base.state);
-
-		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
-			plane_state->visible = primary_get_hw_state(crtc);
-		else {
-			if (active)
-				p->disable_plane(&p->base, &crtc->base);
-
-			plane_state->visible = false;
-		}
-	}
-}
-
 static void intel_modeset_readout_hw_state(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -15076,35 +15052,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
-
-		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
-		if (crtc->base.state->active) {
-			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
-			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
-			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
-
-			/*
-			 * The initial mode needs to be set in order to keep
-			 * the atomic core happy. It wants a valid mode if the
-			 * crtc's enabled, so we do the above call.
-			 *
-			 * At this point some state updated by the connectors
-			 * in their ->detect() callback has not run yet, so
-			 * no recalculation can be done yet.
-			 *
-			 * Even if we could do a recalculation and modeset
-			 * right now it would cause a double modeset if
-			 * fbdev or userspace chooses a different initial mode.
-			 *
-			 * If that happens, someone indicated they wanted a
-			 * mode change, which means it's safe to do a full
-			 * recalculation.
-			 */
-			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
-		}
-
-		crtc->base.hwmode = crtc->config->base.adjusted_mode;
-		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
+		to_intel_plane_state(crtc->base.primary->state)->visible =
+			primary_get_hw_state(crtc);
 
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,

@@ -15186,6 +15135,33 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 
 	for_each_pipe(dev_priv, pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
+		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
+		if (crtc->base.state->active) {
+			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
+			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
+			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
+
+			/*
+			 * The initial mode needs to be set in order to keep
+			 * the atomic core happy. It wants a valid mode if the
+			 * crtc's enabled, so we do the above call.
+			 *
+			 * At this point some state updated by the connectors
+			 * in their ->detect() callback has not run yet, so
+			 * no recalculation can be done yet.
+			 *
+			 * Even if we could do a recalculation and modeset
+			 * right now it would cause a double modeset if
+			 * fbdev or userspace chooses a different initial mode.
+			 *
+			 * If that happens, someone indicated they wanted a
+			 * mode change, which means it's safe to do a full
+			 * recalculation.
+			 */
+			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
+		}
+
 		intel_sanitize_crtc(crtc);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 5 lines

When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout.

Quoted 6 lines

This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0.

Signed-off-by Patrik Jakobsson <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

References: https://bugs.freedesktop.org/show_bug.cgi?id=91428

Maarten, I presume this patch is superseded by the commits referenced in that bug. Is that correct?