[PATCH 1/3] drm/i915: Restore vblank interrupts earlier

[PATCH 1/3] drm/i915: Restore vblank interrupts earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Plane sanitation needs vblank interrupts (on account of CxSR disable).
So let's restore vblank interrupts earlier.

v2: Make it actually build

Cc: stable@vger.kernel.org
Cc: Dennis <dennis.nezic@utoronto.ca>
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 | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4c5c2b39e65c..e018b37bed39 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15551,13 +15551,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 			   I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
 	}
 
-	/* restore vblank interrupts to correct state */
-	drm_crtc_vblank_reset(&crtc->base);
 	if (crtc->active) {
 		struct intel_plane *plane;
 
-		drm_crtc_vblank_on(&crtc->base);
-
 		/* Disable everything but the primary plane */
 		for_each_intel_plane_on_crtc(dev, crtc, plane) {
 			const struct intel_plane_state *plane_state =
@@ -15899,7 +15895,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 			     struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	enum pipe pipe;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	int i;
@@ -15912,15 +15907,19 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	/* HW state is read out, now we need to sanitize this mess. */
 	get_encoder_power_domains(dev_priv);
 
-	intel_sanitize_plane_mapping(dev_priv);
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		drm_crtc_vblank_reset(&crtc->base);
 
-	for_each_intel_encoder(dev, encoder) {
-		intel_sanitize_encoder(encoder);
+		if (crtc->active)
+			drm_crtc_vblank_on(&crtc->base);
 	}
 
-	for_each_pipe(dev_priv, pipe) {
-		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
+	intel_sanitize_plane_mapping(dev_priv);
 
+	for_each_intel_encoder(dev, encoder)
+		intel_sanitize_encoder(encoder);
+
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		intel_sanitize_crtc(crtc, ctx);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");
-- 
2.16.4

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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: 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>
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 | 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

Re: [PATCH 1/3] drm/i915: Restore vblank interrupts earlier

[Daniel Vetter]

On Mon, Oct 01, 2018 at 05:29:07PM +0300, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Plane sanitation needs vblank interrupts (on account of CxSR disable).
> So let's restore vblank interrupts earlier.
> 
> v2: Make it actually build
> 
> Cc: stable@vger.kernel.org
> Cc: Dennis <dennis.nezic@utoronto.ca>
> 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 | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4c5c2b39e65c..e018b37bed39 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15551,13 +15551,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  			   I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
>  	}
>  
> -	/* restore vblank interrupts to correct state */
> -	drm_crtc_vblank_reset(&crtc->base);
>  	if (crtc->active) {
>  		struct intel_plane *plane;
>  
> -		drm_crtc_vblank_on(&crtc->base);
> -
>  		/* Disable everything but the primary plane */
>  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
>  			const struct intel_plane_state *plane_state =
> @@ -15899,7 +15895,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  			     struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	enum pipe pipe;
>  	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
>  	int i;
> @@ -15912,15 +15907,19 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	/* HW state is read out, now we need to sanitize this mess. */
>  	get_encoder_power_domains(dev_priv);
>  
> -	intel_sanitize_plane_mapping(dev_priv);

If you add a comment here why we need to do this, this has my

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		drm_crtc_vblank_reset(&crtc->base);
>  
> -	for_each_intel_encoder(dev, encoder) {
> -		intel_sanitize_encoder(encoder);
> +		if (crtc->active)
> +			drm_crtc_vblank_on(&crtc->base);
>  	}
>  
> -	for_each_pipe(dev_priv, pipe) {
> -		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> +	intel_sanitize_plane_mapping(dev_priv);
>  
> +	for_each_intel_encoder(dev, encoder)
> +		intel_sanitize_encoder(encoder);
> +
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		intel_sanitize_crtc(crtc, ctx);
>  		intel_dump_pipe_config(crtc, crtc->config,
>  				       "[setup_hw_state]");
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

[Daniel Vetter]

On Mon, Oct 01, 2018 at 05:31:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> 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.

How often have we broken this :-/

Unfortunately I still don't have a good idea how to best CI this, since we
shut down everything on module unload. Maybe we should have a special mode
for module unload to leave the hw on, so that we can start testing various
fastboot scenarios ...

Some questions below.

> 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>
> 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 | 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);

I think we need to also update plane_mask here.

>  }
>  
>  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.
> +		 */

Hm, would be neat if we could retire intel_crtc_state->active_planes in
favour of drm_crtc_state->plane_mask. Except for that entire visible y/n
thing :-/

> +		fixup_active_planes(plane_crtc);

Bit a bikeshed, but what about throwing the plane state away and just
starting over, instead of trying to fix it up? We could then use that as a
consistency check, if the plane mappings are still wrong our code is
broken and we should bail out with a very loud warning.

But this here should work too, albeit a bit more fragile I think.

Cheers, Daniel
>  	}
>  }
>  
> @@ -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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

[Ville Syrjälä]

On Tue, Oct 02, 2018 at 02:11:34PM +0200, Daniel Vetter wrote:
> On Mon, Oct 01, 2018 at 05:31:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > 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.
> 
> How often have we broken this :-/
> 
> Unfortunately I still don't have a good idea how to best CI this, since we
> shut down everything on module unload. Maybe we should have a special mode
> for module unload to leave the hw on, so that we can start testing various
> fastboot scenarios ...

Yeah, that might be nice. Though wouldn't directly help here since
we'd still have to move the plane to the other pipe. But we could
of course make the driver unload do that for us as well.

Oh and to hit this bug we'd also need to make sure cxsr is enabled
when we unload as that's what leads to the vblank wait. That's actually
the reason I didn't catch this bug originally. None of my machines
have a VBIOS that enables cxsr.

> 
> Some questions below.
> 
> > 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>
> > 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 | 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);
> 
> I think we need to also update plane_mask here.

plane_mask will be correct since each plane has a unique bit there.
And in fact we use plane_mask to reconstruct active_planes.

What we could do is set active_planes=0 before the loop, as the loop
will populate it fully anyway.

> 
> >  }
> >  
> >  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.
> > +		 */
> 
> Hm, would be neat if we could retire intel_crtc_state->active_planes in
> favour of drm_crtc_state->plane_mask. Except for that entire visible y/n
> thing :-/

I'm a bit torn about this. active_planes is rather convenient for
watermark stuff and whatnot, but on the other hand it doesn't map
well to pre-g4x hardware, so in other ways it's not so great.

> 
> > +		fixup_active_planes(plane_crtc);
> 
> Bit a bikeshed, but what about throwing the plane state away and just
> starting over, instead of trying to fix it up?

You mean just zeroing the plane masks in the crtc state and
doing the plane_readout again? That should be doable.

> We could then use that as a
> consistency check, if the plane mappings are still wrong our code is
> broken and we should bail out with a very loud warning.

Indeed. That seems like a half decent sanity check.

> 
> But this here should work too, albeit a bit more fragile I think.
> 
> Cheers, Daniel
> >  	}
> >  }
> >  
> > @@ -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
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrj�l�
Intel

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

[Daniel Vetter]

On Tue, Oct 02, 2018 at 05:21:36PM +0300, Ville Syrj�l� wrote:
> On Tue, Oct 02, 2018 at 02:11:34PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 01, 2018 at 05:31:20PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > 
> > > 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.
> > 
> > How often have we broken this :-/
> > 
> > Unfortunately I still don't have a good idea how to best CI this, since we
> > shut down everything on module unload. Maybe we should have a special mode
> > for module unload to leave the hw on, so that we can start testing various
> > fastboot scenarios ...
> 
> Yeah, that might be nice. Though wouldn't directly help here since
> we'd still have to move the plane to the other pipe. But we could
> of course make the driver unload do that for us as well.
> 
> Oh and to hit this bug we'd also need to make sure cxsr is enabled
> when we unload as that's what leads to the vblank wait. That's actually
> the reason I didn't catch this bug originally. None of my machines
> have a VBIOS that enables cxsr.

Well that should be easy, as long as i915.ko enables cxsr ...

Just pondered this since with Hans' work fedora is now using fastbook, so
constantly breaking this stuff is kinda no longer a good option. But
definitely future work.

> > Some questions below.
> > 
> > > 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>
> > > 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 | 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);
> > 
> > I think we need to also update plane_mask here.
> 
> plane_mask will be correct since each plane has a unique bit there.
> And in fact we use plane_mask to reconstruct active_planes.
> 
> What we could do is set active_planes=0 before the loop, as the loop
> will populate it fully anyway.

That + comment explaining why we don't need to reconstruct plane_mask
would be good. Since I completely missed that. Something like:

	/* active_planes aliases if mutliple "primary" planes have been
	 * used on the same (or wrong) pipe. plane_mask uses unique ids,
	 * hence we can use that to reconstruct active_planes. */

Hm, maybe we want to remove the active_planes updating from
intel_set_plane_visible then, since it's just garbage anyway? And with the
3rd caller removed, we always follow up with a call to this fixup function
here.

> > 
> > >  }
> > >  
> > >  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.
> > > +		 */
> > 
> > Hm, would be neat if we could retire intel_crtc_state->active_planes in
> > favour of drm_crtc_state->plane_mask. Except for that entire visible y/n
> > thing :-/
> 
> I'm a bit torn about this. active_planes is rather convenient for
> watermark stuff and whatnot, but on the other hand it doesn't map
> well to pre-g4x hardware, so in other ways it's not so great.

Yeah. Maybe a for_each_visible_plane_on_crtc iterator could help, which is
essentially:
	
	for_each_plane_on_crtc()
		for_each_if(plane_state->visible)
	
We'd still need to frob the intel_plane->plane out for our wm code. But
the macro might be generally useful in other drivers too.

Anyway, all just an aside.
> 
> > 
> > > +		fixup_active_planes(plane_crtc);
> > 
> > Bit a bikeshed, but what about throwing the plane state away and just
> > starting over, instead of trying to fix it up?
> 
> You mean just zeroing the plane masks in the crtc state and
> doing the plane_readout again? That should be doable.

Nah strike that, on second thought I think the pattern of first doing
intel_set_plane_visible (but without the ->active_plane stuff), then
fixup_active_planes() sounds clear enough for me. If it works. I think
this was just me not entirely understanding the problem (and some
redundant code that threw me off the rails).

Cheers, Daniel

> > We could then use that as a
> > consistency check, if the plane mappings are still wrong our code is
> > broken and we should bail out with a very loud warning.
> 
> Indeed. That seems like a half decent sanity check.
> 
> > 
> > But this here should work too, albeit a bit more fragile I think.
> > 
> > Cheers, Daniel
> > >  	}
> > >  }
> > >  
> > > @@ -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
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrj�l�
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

[Ville Syrjälä]

On Wed, Oct 03, 2018 at 10:53:11AM +0200, Daniel Vetter wrote:
> On Tue, Oct 02, 2018 at 05:21:36PM +0300, Ville Syrj�l� wrote:
> > On Tue, Oct 02, 2018 at 02:11:34PM +0200, Daniel Vetter wrote:
> > > On Mon, Oct 01, 2018 at 05:31:20PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > 
> > > > 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.
> > > 
> > > How often have we broken this :-/
> > > 
> > > Unfortunately I still don't have a good idea how to best CI this, since we
> > > shut down everything on module unload. Maybe we should have a special mode
> > > for module unload to leave the hw on, so that we can start testing various
> > > fastboot scenarios ...
> > 
> > Yeah, that might be nice. Though wouldn't directly help here since
> > we'd still have to move the plane to the other pipe. But we could
> > of course make the driver unload do that for us as well.
> > 
> > Oh and to hit this bug we'd also need to make sure cxsr is enabled
> > when we unload as that's what leads to the vblank wait. That's actually
> > the reason I didn't catch this bug originally. None of my machines
> > have a VBIOS that enables cxsr.
> 
> Well that should be easy, as long as i915.ko enables cxsr ...
> 
> Just pondered this since with Hans' work fedora is now using fastbook, so
> constantly breaking this stuff is kinda no longer a good option. But
> definitely future work.
> 
> > > Some questions below.
> > > 
> > > > 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>
> > > > 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 | 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);
> > > 
> > > I think we need to also update plane_mask here.
> > 
> > plane_mask will be correct since each plane has a unique bit there.
> > And in fact we use plane_mask to reconstruct active_planes.
> > 
> > What we could do is set active_planes=0 before the loop, as the loop
> > will populate it fully anyway.
> 
> That + comment explaining why we don't need to reconstruct plane_mask
> would be good. Since I completely missed that. Something like:
> 
> 	/* active_planes aliases if mutliple "primary" planes have been
> 	 * used on the same (or wrong) pipe. plane_mask uses unique ids,
> 	 * hence we can use that to reconstruct active_planes. */

Sure.

> 
> Hm, maybe we want to remove the active_planes updating from
> intel_set_plane_visible then, since it's just garbage anyway? And with the
> 3rd caller removed, we always follow up with a call to this fixup function
> here.

There's still intel_plane_disable_noatomic(). Though I guess we could
just call the fixup from there as well.

> 
> > > 
> > > >  }
> > > >  
> > > >  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.
> > > > +		 */
> > > 
> > > Hm, would be neat if we could retire intel_crtc_state->active_planes in
> > > favour of drm_crtc_state->plane_mask. Except for that entire visible y/n
> > > thing :-/
> > 
> > I'm a bit torn about this. active_planes is rather convenient for
> > watermark stuff and whatnot, but on the other hand it doesn't map
> > well to pre-g4x hardware, so in other ways it's not so great.
> 
> Yeah. Maybe a for_each_visible_plane_on_crtc iterator could help, which is
> essentially:
> 	
> 	for_each_plane_on_crtc()
> 		for_each_if(plane_state->visible)
> 	
> We'd still need to frob the intel_plane->plane out for our wm code. But
> the macro might be generally useful in other drivers too.
> 
> Anyway, all just an aside.
> > 
> > > 
> > > > +		fixup_active_planes(plane_crtc);
> > > 
> > > Bit a bikeshed, but what about throwing the plane state away and just
> > > starting over, instead of trying to fix it up?
> > 
> > You mean just zeroing the plane masks in the crtc state and
> > doing the plane_readout again? That should be doable.
> 
> Nah strike that, on second thought I think the pattern of first doing
> intel_set_plane_visible (but without the ->active_plane stuff), then
> fixup_active_planes() sounds clear enough for me. If it works. I think
> this was just me not entirely understanding the problem (and some
> redundant code that threw me off the rails).
> 
> Cheers, Daniel
> 
> > > We could then use that as a
> > > consistency check, if the plane mappings are still wrong our code is
> > > broken and we should bail out with a very loud warning.
> > 
> > Indeed. That seems like a half decent sanity check.
> > 
> > > 
> > > But this here should work too, albeit a bit more fragile I think.
> > > 
> > > Cheers, Daniel
> > > >  	}
> > > >  }
> > > >  
> > > > @@ -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
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrj�l�
> > Intel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrj�l�
Intel

[PATCH v2 1/3] drm/i915: Restore vblank interrupts earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Plane sanitation needs vblank interrupts (on account of CxSR disable).
So let's restore vblank interrupts earlier.

v2: Make it actually build
v3: Add comment to explain why we need this (Daniel)

Cc: stable@vger.kernel.org
Cc: Dennis <dennis.nezic@utoronto.ca>
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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 36434c5359b1..d2828159f6c8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15570,13 +15570,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 			   I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
 	}
 
-	/* restore vblank interrupts to correct state */
-	drm_crtc_vblank_reset(&crtc->base);
 	if (crtc->active) {
 		struct intel_plane *plane;
 
-		drm_crtc_vblank_on(&crtc->base);
-
 		/* Disable everything but the primary plane */
 		for_each_intel_plane_on_crtc(dev, crtc, plane) {
 			const struct intel_plane_state *plane_state =
@@ -15918,7 +15914,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 			     struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	enum pipe pipe;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	int i;
@@ -15931,15 +15926,23 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	/* HW state is read out, now we need to sanitize this mess. */
 	get_encoder_power_domains(dev_priv);
 
-	intel_sanitize_plane_mapping(dev_priv);
+	/*
+	 * intel_sanitize_plane_mapping() may need to do vblank
+	 * waits, so we need vblank interrupts restored beforehand.
+	 */
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		drm_crtc_vblank_reset(&crtc->base);
 
-	for_each_intel_encoder(dev, encoder) {
-		intel_sanitize_encoder(encoder);
+		if (crtc->active)
+			drm_crtc_vblank_on(&crtc->base);
 	}
 
-	for_each_pipe(dev_priv, pipe) {
-		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
+	intel_sanitize_plane_mapping(dev_priv);
 
+	for_each_intel_encoder(dev, encoder)
+		intel_sanitize_encoder(encoder);
+
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		intel_sanitize_crtc(crtc, ctx);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");
-- 
2.16.4

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

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

[Daniel Vetter]

On Wed, Oct 03, 2018 at 05:50:17PM +0300, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> 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>

I have the illusion of understanding this stuff now.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But let's see whether testers and CI agree :-)
-Daniel

> ---
>  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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Wed, Oct 03, 2018 at 06:12:42PM +0200, Daniel Vetter wrote:
> On Wed, Oct 03, 2018 at 05:50:17PM +0300, Ville Syrjala wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > 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>
> 
> I have the illusion of understanding this stuff now.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But let's see whether testers and CI agree :-)

Seems to be reasonably happy :)

Picked up another tested-by from the bug report
Tested-by: Peter Nowee <peter.nowee@gmail.com>

and pushed the series to dinq. Thanks to everyone involved.

> -Daniel
> 
> > ---
> >  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
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrj�l�
Intel