Attachment 141775 Details for Bug 105637 – [PATCH] drm/i915: Use the correct crtc when sanitizing plane mapping

[patch] [PATCH] drm/i915: Use the correct crtc when sanitizing plane mapping

0001-drm-i915-Use-the-correct-crtc-when-sanitizing-plane-.patch (text/plain), 5.55 KB, created by Ville Syrjala on 2018-09-28 12:57:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ville Syrjala

Created: 2018-09-28 12:57:32 UTC

Size: 5.55 KB

patch

obsolete

>From 248003bf56d04456c6c4b74865a2ccdc1ce75f87 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <ville.syrjala@linux.intel.com>
>Date: Fri, 28 Sep 2018 15:01:26 +0300
>Subject: [PATCH] drm/i915: Use the correct crtc when sanitizing plane mapping
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>When we decide that a plane is attached to the wrong pipe we try
>to turn off said plane. However we are passing around the crtc we
>think that the plane is supposed to be using rather than the crtc
>it is currently using. That doesn't work all that well because
>we may have to do vblank waits etc. and the other pipe might
>not even be enabled here. So let's pass the plane's current crtc to
>intel_plane_disable_noatomic() so that it can its job correctly.
>
>To do that semi-cleanly we also have to change the plane readout
>to record the plane's visibility into the bitmasks of the crtc
>where the plane is currently enabled rather than to the crtc
>we want to use for the plane.
>
>One caveat here is that our active_planes bitmask will get confused
>if both planes are enabled on the same pipe. Fortunately we can use
>plane_mask to reconstruct active_planes sufficiently since
>plane_mask still has the same meaning (is the plane visible?)
>during readout. We also have to do the same during the initial
>plane readout as the second plane could clear the active_planes
>bit the first plane had already set.
>
>Cc: Dennis <dennis.nezic@utoronto.ca>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105637
>Signed-off-by: Ville SyrjÃ¤lÃ¤ <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++++++---------
> 1 file changed, 47 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index e018b37bed39..c72be8cd1f54 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -15475,15 +15475,16 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> 	POSTING_READ(DPLL(pipe));
> }
> 
>-static bool intel_plane_mapping_ok(struct intel_crtc *crtc,
>-				   struct intel_plane *plane)
>+static void fixup_active_planes(struct intel_crtc *crtc)
> {
>-	enum pipe pipe;
>-
>-	if (!plane->get_hw_state(plane, &pipe))
>-		return true;
>+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>+	struct intel_crtc_state *crtc_state =
>+		to_intel_crtc_state(crtc->base.state);
>+	struct drm_plane *plane;
> 
>-	return pipe == crtc->pipe;
>+	drm_for_each_plane_mask(plane, &dev_priv->drm,
>+				crtc_state->base.plane_mask)
>+		crtc_state->active_planes |= BIT(to_intel_plane(plane)->id);
> }
> 
> static void
>@@ -15497,13 +15498,28 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
> 	for_each_intel_crtc(&dev_priv->drm, crtc) {
> 		struct intel_plane *plane =
> 			to_intel_plane(crtc->base.primary);
>+		struct intel_crtc *plane_crtc;
>+		enum pipe pipe;
>+
>+		if (!plane->get_hw_state(plane, &pipe))
>+			continue;
> 
>-		if (intel_plane_mapping_ok(crtc, plane))
>+		if (pipe == crtc->pipe)
> 			continue;
> 
> 		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
> 			      plane->base.name);
>-		intel_plane_disable_noatomic(crtc, plane);
>+
>+		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>+		intel_plane_disable_noatomic(plane_crtc, plane);
>+
>+		/*
>+		 * Our active_planes tracking will get confused here
>+		 * if both planes A and B are enabled on the same pipe
>+		 * (since both planes map to BIT(PLANE_PRIMARY)).
>+		 * Reconstruct active_planes after disabling the plane.
>+		 */
>+		fixup_active_planes(plane_crtc);
> 	}
> }
> 
>@@ -15671,23 +15687,38 @@ void i915_redisable_vga(struct drm_i915_private *dev_priv)
> }
> 
> /* FIXME read out full plane state for all planes */
>-static void readout_plane_state(struct intel_crtc *crtc)
>+static void readout_plane_state(struct drm_i915_private *dev_priv)
> {
>-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>-	struct intel_crtc_state *crtc_state =
>-		to_intel_crtc_state(crtc->base.state);
> 	struct intel_plane *plane;
>+	struct intel_crtc *crtc;
> 
>-	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>+	for_each_intel_plane(&dev_priv->drm, plane) {
> 		struct intel_plane_state *plane_state =
> 			to_intel_plane_state(plane->base.state);
>+		struct intel_crtc_state *crtc_state;
> 		enum pipe pipe;
> 		bool visible;
> 
> 		visible = plane->get_hw_state(plane, &pipe);
> 
>+		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>+		crtc_state = to_intel_crtc_state(crtc->base.state);
>+
> 		intel_set_plane_visible(crtc_state, plane_state, visible);
> 	}
>+
>+	for_each_intel_crtc(&dev_priv->drm, crtc) {
>+		/*
>+		 * Our active_planes tracking may get confused here
>+		 * on gen2/3 if the first plane is enabled but the
>+		 * second one isn't but both indicate the same pipe.
>+		 * The second plane would clear the active_planes
>+		 * bit for the first plane (since both map to
>+		 * BIT(PLANE_PRIMARY). Reconstruct active_planes
>+		 * after plane readout is done.
>+		 */
>+		fixup_active_planes(crtc);
>+	}
> }
> 
> static void intel_modeset_readout_hw_state(struct drm_device *dev)
>@@ -15719,13 +15750,13 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> 		if (crtc_state->base.active)
> 			dev_priv->active_crtcs |= 1 << crtc->pipe;
> 
>-		readout_plane_state(crtc);
>-
> 		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
> 			      crtc->base.base.id, crtc->base.name,
> 			      enableddisabled(crtc_state->base.active));
> 	}
> 
>+	readout_plane_state(dev_priv);
>+
> 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> 
>-- 
>2.16.4
>

Actions: View | Splinter Review

Attachments on bug 105637: 138232 | 138266 | 139355 | 139646 | 139806 | 141595 | 141596 | 141672 | 141751 | 141764 | 141765 | 141768 | 141775 | 141782 | 141858

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.