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

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

v2-0002-drm-i915-Use-the-correct-crtc-when-sanitizing-pla.patch (text/plain), 7.51 KB, created by Ville Syrjala on 2018-10-03 16:31:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ville Syrjala

Created: 2018-10-03 16:31:53 UTC

Size: 7.51 KB

patch

obsolete

>From bfebd23836b05c7edff6a63c3b64f9bdd53ebd1d 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 v2 2/3] 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.
>
>v2: Rely on fixup_active_planes() to populate active_planes fully (Daniel)
>    Add Daniel's proposed comment to better document why we do this
>    Drop the redundant intel_set_plane_visible() call
>
>Cc: stable@vger.kernel.org # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
>Cc: stable@vger.kernel.org
>Cc: Dennis <dennis.nezic@utoronto.ca>
>Cc: Daniel Vetter <daniel@ffwll.ch>
>Tested-by: Dennis <dennis.nezic@utoronto.ca>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105637
>Fixes: b1e01595a66d ("drm/i915: Redo plane sanitation during readout")
>Signed-off-by: Ville SyrjÃ¤lÃ¤ <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_display.c | 78 +++++++++++++++++++++---------------
> 1 file changed, 46 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index d2828159f6c8..f0d004641b0d 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -2764,20 +2764,33 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
> 
> 	plane_state->base.visible = visible;
> 
>-	/* FIXME pre-g4x don't work like this */
>-	if (visible) {
>+	if (visible)
> 		crtc_state->base.plane_mask |= drm_plane_mask(&plane->base);
>-		crtc_state->active_planes |= BIT(plane->id);
>-	} else {
>+	else
> 		crtc_state->base.plane_mask &= ~drm_plane_mask(&plane->base);
>-		crtc_state->active_planes &= ~BIT(plane->id);
>-	}
> 
> 	DRM_DEBUG_KMS("%s active planes 0x%x\n",
> 		      crtc_state->base.crtc->name,
> 		      crtc_state->active_planes);
> }
> 
>+static void fixup_active_planes(struct intel_crtc_state *crtc_state)
>+{
>+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>+	struct drm_plane *plane;
>+
>+	/*
>+	 * Active_planes aliases if multiple "primary" or cursor planes
>+	 * have been used on the same (or wrong) pipe. plane_mask uses
>+	 * unique ids, hence we can use that to reconstruct active_planes.
>+	 */
>+	crtc_state->active_planes = 0;
>+
>+	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 intel_plane_disable_noatomic(struct intel_crtc *crtc,
> 					 struct intel_plane *plane)
> {
>@@ -2787,6 +2800,7 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> 		to_intel_plane_state(plane->base.state);
> 
> 	intel_set_plane_visible(crtc_state, plane_state, false);
>+	fixup_active_planes(crtc_state);
> 
> 	if (plane->id == PLANE_PRIMARY)
> 		intel_pre_disable_primary_noatomic(&crtc->base);
>@@ -2805,7 +2819,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> 	struct drm_i915_gem_object *obj;
> 	struct drm_plane *primary = intel_crtc->base.primary;
> 	struct drm_plane_state *plane_state = primary->state;
>-	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
> 	struct intel_plane *intel_plane = to_intel_plane(primary);
> 	struct intel_plane_state *intel_state =
> 		to_intel_plane_state(plane_state);
>@@ -2900,10 +2913,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> 	plane_state->fb = fb;
> 	plane_state->crtc = &intel_crtc->base;
> 
>-	intel_set_plane_visible(to_intel_crtc_state(crtc_state),
>-				to_intel_plane_state(plane_state),
>-				true);
>-
> 	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
> 		  &obj->frontbuffer_bits);
> }
>@@ -15494,17 +15503,6 @@ 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)
>-{
>-	enum pipe pipe;
>-
>-	if (!plane->get_hw_state(plane, &pipe))
>-		return true;
>-
>-	return pipe == crtc->pipe;
>-}
>-
> static void
> intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
> {
>@@ -15516,13 +15514,20 @@ 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 (intel_plane_mapping_ok(crtc, plane))
>+		if (!plane->get_hw_state(plane, &pipe))
>+			continue;
>+
>+		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);
> 	}
> }
> 
>@@ -15690,23 +15695,32 @@ 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);
>-		enum pipe pipe;
>+		struct intel_crtc_state *crtc_state;
>+		enum pipe pipe = PIPE_A;
> 		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) {
>+		struct intel_crtc_state *crtc_state =
>+			to_intel_crtc_state(crtc->base.state);
>+
>+		fixup_active_planes(crtc_state);
>+	}
> }
> 
> static void intel_modeset_readout_hw_state(struct drm_device *dev)
>@@ -15738,13 +15752,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.