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

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[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

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

[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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

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

[PATCH 3/3] drm/i915: Clean up early plane debugs

[Ville Syrjala]

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

Print the plane hw state readout results in the common format
we already use for pipes and encoders. Also print some clearer
debug messages when we disable planes during the early phases
of state readout/sanitization.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c72be8cd1f54..1e9628f0cb47 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2772,10 +2772,6 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
 		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 intel_plane_disable_noatomic(struct intel_crtc *crtc,
@@ -2786,6 +2782,10 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(plane->base.state);
 
+	DRM_DEBUG_KMS("Disabling [PLANE:%d:%s] on [CRTC:%d:%s]\n",
+		      plane->base.base.id, plane->base.name,
+		      crtc->base.base.id, crtc->base.name);
+
 	intel_set_plane_visible(crtc_state, plane_state, false);
 
 	if (plane->id == PLANE_PRIMARY)
@@ -15507,8 +15507,8 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
 		if (pipe == crtc->pipe)
 			continue;
 
-		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
-			      plane->base.name);
+		DRM_DEBUG_KMS("[PLANE:%d:%s] attached to the wrong pipe, disabling plane\n",
+			      plane->base.base.id, plane->base.name);
 
 		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 		intel_plane_disable_noatomic(plane_crtc, plane);
@@ -15705,6 +15705,10 @@ static void readout_plane_state(struct drm_i915_private *dev_priv)
 		crtc_state = to_intel_crtc_state(crtc->base.state);
 
 		intel_set_plane_visible(crtc_state, plane_state, visible);
+
+		DRM_DEBUG_KMS("[PLANE:%d:%s] hw state readout: %s, pipe %c\n",
+			      plane->base.base.id, plane->base.name,
+			      enableddisabled(visible), pipe_name(pipe));
 	}
 
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Clean up early plane debugs

[Daniel Vetter]

On Mon, Oct 01, 2018 at 05:31:27PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Print the plane hw state readout results in the common format
> we already use for pipes and encoders. Also print some clearer
> debug messages when we disable planes during the early phases
> of state readout/sanitization.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c72be8cd1f54..1e9628f0cb47 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2772,10 +2772,6 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
>  		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);

There's a 3rd call for intel_set_plane_visible in the plane_obj code. Do
we not care? Is that call redundant (kinda a comment on the previous patch
really, just only realized it here)?
-Daniel

>  }
>  
>  static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> @@ -2786,6 +2782,10 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>  	struct intel_plane_state *plane_state =
>  		to_intel_plane_state(plane->base.state);
>  
> +	DRM_DEBUG_KMS("Disabling [PLANE:%d:%s] on [CRTC:%d:%s]\n",
> +		      plane->base.base.id, plane->base.name,
> +		      crtc->base.base.id, crtc->base.name);
> +
>  	intel_set_plane_visible(crtc_state, plane_state, false);
>  
>  	if (plane->id == PLANE_PRIMARY)
> @@ -15507,8 +15507,8 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
>  		if (pipe == crtc->pipe)
>  			continue;
>  
> -		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
> -			      plane->base.name);
> +		DRM_DEBUG_KMS("[PLANE:%d:%s] attached to the wrong pipe, disabling plane\n",
> +			      plane->base.base.id, plane->base.name);
>  
>  		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>  		intel_plane_disable_noatomic(plane_crtc, plane);
> @@ -15705,6 +15705,10 @@ static void readout_plane_state(struct drm_i915_private *dev_priv)
>  		crtc_state = to_intel_crtc_state(crtc->base.state);
>  
>  		intel_set_plane_visible(crtc_state, plane_state, visible);
> +
> +		DRM_DEBUG_KMS("[PLANE:%d:%s] hw state readout: %s, pipe %c\n",
> +			      plane->base.base.id, plane->base.name,
> +			      enableddisabled(visible), pipe_name(pipe));
>  	}
>  
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
> -- 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Clean up early plane debugs

[Ville Syrjälä]

On Tue, Oct 02, 2018 at 02:21:32PM +0200, Daniel Vetter wrote:
> On Mon, Oct 01, 2018 at 05:31:27PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Print the plane hw state readout results in the common format
> > we already use for pipes and encoders. Also print some clearer
> > debug messages when we disable planes during the early phases
> > of state readout/sanitization.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c72be8cd1f54..1e9628f0cb47 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2772,10 +2772,6 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
> >  		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);
> 
> There's a 3rd call for intel_set_plane_visible in the plane_obj code. Do
> we not care? Is that call redundant (kinda a comment on the previous patch
> really, just only realized it here)?

Yeah, looks like that call is redundant. The only way to reach it
is if the plane was already enabled.

> -Daniel
> 
> >  }
> >  
> >  static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> > @@ -2786,6 +2782,10 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> >  	struct intel_plane_state *plane_state =
> >  		to_intel_plane_state(plane->base.state);
> >  
> > +	DRM_DEBUG_KMS("Disabling [PLANE:%d:%s] on [CRTC:%d:%s]\n",
> > +		      plane->base.base.id, plane->base.name,
> > +		      crtc->base.base.id, crtc->base.name);
> > +
> >  	intel_set_plane_visible(crtc_state, plane_state, false);
> >  
> >  	if (plane->id == PLANE_PRIMARY)
> > @@ -15507,8 +15507,8 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
> >  		if (pipe == crtc->pipe)
> >  			continue;
> >  
> > -		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
> > -			      plane->base.name);
> > +		DRM_DEBUG_KMS("[PLANE:%d:%s] attached to the wrong pipe, disabling plane\n",
> > +			      plane->base.base.id, plane->base.name);
> >  
> >  		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> >  		intel_plane_disable_noatomic(plane_crtc, plane);
> > @@ -15705,6 +15705,10 @@ static void readout_plane_state(struct drm_i915_private *dev_priv)
> >  		crtc_state = to_intel_crtc_state(crtc->base.state);
> >  
> >  		intel_set_plane_visible(crtc_state, plane_state, visible);
> > +
> > +		DRM_DEBUG_KMS("[PLANE:%d:%s] hw state readout: %s, pipe %c\n",
> > +			      plane->base.base.id, plane->base.name,
> > +			      enableddisabled(visible), pipe_name(pipe));
> >  	}
> >  
> >  	for_each_intel_crtc(&dev_priv->drm, crtc) {
> > -- 
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/i915: Clean up early plane debugs

[Ville Syrjala]

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

Print the plane hw state readout results in the common format
we already use for pipes and encoders. Also print some clearer
debug messages when we disable planes during the early phases
of state readout/sanitization.

v2: Rebase

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f0d004641b0d..24fe3b1fb2a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2768,10 +2768,6 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
 		crtc_state->base.plane_mask |= drm_plane_mask(&plane->base);
 	else
 		crtc_state->base.plane_mask &= ~drm_plane_mask(&plane->base);
-
-	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)
@@ -2799,6 +2795,10 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(plane->base.state);
 
+	DRM_DEBUG_KMS("Disabling [PLANE:%d:%s] on [CRTC:%d:%s]\n",
+		      plane->base.base.id, plane->base.name,
+		      crtc->base.base.id, crtc->base.name);
+
 	intel_set_plane_visible(crtc_state, plane_state, false);
 	fixup_active_planes(crtc_state);
 
@@ -15523,8 +15523,8 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
 		if (pipe == crtc->pipe)
 			continue;
 
-		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
-			      plane->base.name);
+		DRM_DEBUG_KMS("[PLANE:%d:%s] attached to the wrong pipe, disabling plane\n",
+			      plane->base.base.id, plane->base.name);
 
 		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 		intel_plane_disable_noatomic(plane_crtc, plane);
@@ -15713,6 +15713,10 @@ static void readout_plane_state(struct drm_i915_private *dev_priv)
 		crtc_state = to_intel_crtc_state(crtc->base.state);
 
 		intel_set_plane_visible(crtc_state, plane_state, visible);
+
+		DRM_DEBUG_KMS("[PLANE:%d:%s] hw state readout: %s, pipe %c\n",
+			      plane->base.base.id, plane->base.name,
+			      enableddisabled(visible), pipe_name(pipe));
 	}
 
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Clean up early plane debugs

[Daniel Vetter]

On Wed, Oct 03, 2018 at 05:50:52PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Print the plane hw state readout results in the common format
> we already use for pipes and encoders. Also print some clearer
> debug messages when we disable planes during the early phases
> of state readout/sanitization.
> 
> v2: Rebase
> 
> 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 | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f0d004641b0d..24fe3b1fb2a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2768,10 +2768,6 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
>  		crtc_state->base.plane_mask |= drm_plane_mask(&plane->base);
>  	else
>  		crtc_state->base.plane_mask &= ~drm_plane_mask(&plane->base);
> -
> -	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)
> @@ -2799,6 +2795,10 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>  	struct intel_plane_state *plane_state =
>  		to_intel_plane_state(plane->base.state);
>  
> +	DRM_DEBUG_KMS("Disabling [PLANE:%d:%s] on [CRTC:%d:%s]\n",
> +		      plane->base.base.id, plane->base.name,
> +		      crtc->base.base.id, crtc->base.name);
> +
>  	intel_set_plane_visible(crtc_state, plane_state, false);
>  	fixup_active_planes(crtc_state);
>  
> @@ -15523,8 +15523,8 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
>  		if (pipe == crtc->pipe)
>  			continue;
>  
> -		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
> -			      plane->base.name);
> +		DRM_DEBUG_KMS("[PLANE:%d:%s] attached to the wrong pipe, disabling plane\n",
> +			      plane->base.base.id, plane->base.name);
>  
>  		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>  		intel_plane_disable_noatomic(plane_crtc, plane);
> @@ -15713,6 +15713,10 @@ static void readout_plane_state(struct drm_i915_private *dev_priv)
>  		crtc_state = to_intel_crtc_state(crtc->base.state);
>  
>  		intel_set_plane_visible(crtc_state, plane_state, visible);
> +
> +		DRM_DEBUG_KMS("[PLANE:%d:%s] hw state readout: %s, pipe %c\n",
> +			      plane->base.base.id, plane->base.name,
> +			      enableddisabled(visible), pipe_name(pipe));
>  	}
>  
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Restore vblank interrupts earlier

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Restore vblank interrupts earlier
URL   : https://patchwork.freedesktop.org/series/50393/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3d421aa28f28 drm/i915: Restore vblank interrupts earlier
ab4d85399a55 drm/i915: Use the correct crtc when sanitizing plane mapping
-:30: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#30: 
Cc: stable@vger.kernel.org # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe

total: 0 errors, 1 warnings, 0 checks, 111 lines checked
56da01b165ac drm/i915: Clean up early plane debugs

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Restore vblank interrupts earlier

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Restore vblank interrupts earlier
URL   : https://patchwork.freedesktop.org/series/50393/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4907 -> Patchwork_10307 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50393/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10307 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-bdw-samus:       PASS -> INCOMPLETE (fdo#107773)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-icl-u:           PASS -> INCOMPLETE (fdo#107713)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106725, fdo#106248) -> PASS

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-skl-caroline:    INCOMPLETE (fdo#104108, fdo#107556, fdo#107773) -> PASS

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       FAIL (fdo#104767) -> PASS

    
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104767 https://bugs.freedesktop.org/show_bug.cgi?id=104767
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773


== Participating hosts (52 -> 47) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4907 -> Patchwork_10307

  CI_DRM_4907: d628a99df39be84d6fd0a608bf2ab2185dcd6a26 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4658: cab89ce2c5da684d01deff402d4e8e11441beadb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10307: 56da01b165ac0fbb4f3ae68d0ef9aa15075b2cd5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

56da01b165ac drm/i915: Clean up early plane debugs
ab4d85399a55 drm/i915: Use the correct crtc when sanitizing plane mapping
3d421aa28f28 drm/i915: Restore vblank interrupts earlier

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10307/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915: Restore vblank interrupts earlier

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Restore vblank interrupts earlier
URL   : https://patchwork.freedesktop.org/series/50393/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4907_full -> Patchwork_10307_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10307_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_await@wide-contexts:
      shard-apl:          PASS -> FAIL (fdo#106680)

    igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
      shard-glk:          PASS -> DMESG-WARN (fdo#106538, fdo#105763)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#102887, fdo#105363)

    igt@kms_vblank@pipe-b-ts-continuation-suspend:
      shard-kbl:          PASS -> DMESG-WARN (fdo#105602, fdo#103558) +1

    
    ==== Possible fixes ====

    igt@gem_exec_big:
      shard-hsw:          TIMEOUT (fdo#107937) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#103665, fdo#106023) -> PASS

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-skl 


== Build changes ==

    * Linux: CI_DRM_4907 -> Patchwork_10307

  CI_DRM_4907: d628a99df39be84d6fd0a608bf2ab2185dcd6a26 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4658: cab89ce2c5da684d01deff402d4e8e11441beadb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10307: 56da01b165ac0fbb4f3ae68d0ef9aa15075b2cd5 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10307/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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

✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915: Restore vblank interrupts earlier (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Restore vblank interrupts earlier (rev4)
URL   : https://patchwork.freedesktop.org/series/50393/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c6a938a3e0a7 drm/i915: Restore vblank interrupts earlier
d5a5d74e82b0 drm/i915: Use the correct crtc when sanitizing plane mapping
-:34: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#34: 
Cc: stable@vger.kernel.org # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe

total: 0 errors, 1 warnings, 0 checks, 155 lines checked
b1aa286906d2 drm/i915: Clean up early plane debugs

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915: Restore vblank interrupts earlier (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Restore vblank interrupts earlier (rev4)
URL   : https://patchwork.freedesktop.org/series/50393/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4922 -> Patchwork_10345 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50393/revisions/4/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10345 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107556, fdo#107774, fdo#107859)

    igt@kms_addfb_basic@unused-pitches:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-cfl-8109u:       PASS -> INCOMPLETE (fdo#108126, fdo#106070)

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#107726)

    
    ==== Possible fixes ====

    igt@gem_tiled_fence_blits@basic:
      fi-glk-j4005:       DMESG-WARN (fdo#105719) -> PASS

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107726 https://bugs.freedesktop.org/show_bug.cgi?id=107726
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
  fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126


== Participating hosts (48 -> 43) ==

  Additional (2): fi-kbl-soraka fi-snb-2520m 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-kbl-7560u 


== Build changes ==

    * Linux: CI_DRM_4922 -> Patchwork_10345

  CI_DRM_4922: 97b376a5213463a70eb977282f5486ded096648f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4665: 267870165d9ef66b4ab423e4efe7bacba023d75e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10345: b1aa286906d2a13955c417342a2df5db4943a39a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b1aa286906d2 drm/i915: Clean up early plane debugs
d5a5d74e82b0 drm/i915: Use the correct crtc when sanitizing plane mapping
c6a938a3e0a7 drm/i915: Restore vblank interrupts earlier

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10345/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v2,1/3] drm/i915: Restore vblank interrupts earlier (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Restore vblank interrupts earlier (rev4)
URL   : https://patchwork.freedesktop.org/series/50393/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4922_full -> Patchwork_10345_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10345_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_await@wide-contexts:
      shard-glk:          PASS -> FAIL (fdo#106680)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-skl:          NOTRUN -> TIMEOUT (fdo#108039, fdo#108130)

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-skl:          NOTRUN -> FAIL (fdo#106641)

    igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-b:
      shard-apl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +5

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108)

    igt@kms_cursor_crc@cursor-128x42-random:
      shard-skl:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-256x85-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +4

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-glk:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)
      shard-apl:          PASS -> FAIL (fdo#103232, fdo#103191)

    igt@kms_draw_crc@draw-method-xrgb2101010-render-ytiled:
      shard-skl:          NOTRUN -> FAIL (fdo#103184)

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc:
      shard-skl:          NOTRUN -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
      shard-glk:          PASS -> FAIL (fdo#103167) +3

    igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
      shard-skl:          NOTRUN -> FAIL (fdo#105683)

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-glk:          PASS -> FAIL (fdo#103166) +1

    {igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max}:
      shard-glk:          PASS -> FAIL (fdo#108145) +2

    {igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min}:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +2

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    igt@pm_rpm@system-suspend:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807, fdo#104108)

    igt@pm_rpm@universal-planes:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@gem_userptr_blits@readonly-unsync:
      shard-skl:          INCOMPLETE (fdo#108074) -> PASS

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          FAIL (fdo#103232) -> PASS +3

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-skl:          INCOMPLETE (fdo#104108) -> PASS

    igt@kms_cursor_crc@cursor-256x85-onscreen:
      shard-apl:          FAIL (fdo#103232) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-pwrite:
      shard-glk:          FAIL (fdo#103167) -> PASS +7

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
      shard-glk:          FAIL (fdo#103166) -> PASS +2

    igt@kms_rotation_crc@sprite-rotation-90-pos-100-0:
      shard-apl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +57

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS

    igt@kms_universal_plane@universal-plane-pipe-b-functional:
      shard-apl:          FAIL (fdo#103166) -> PASS

    
    ==== Warnings ====

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-apl:          DMESG-WARN (fdo#103558, fdo#105602) -> FAIL (fdo#105458, fdo#106510)

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-apl:          DMESG-WARN (fdo#103558, fdo#105602) -> FAIL (fdo#103166)

    {igt@kms_plane_alpha_blend@pipe-a-alpha-7efc}:
      shard-apl:          FAIL (fdo#108145) -> DMESG-FAIL (fdo#103558, fdo#105602)

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-apl:          DMESG-FAIL (fdo#103558, fdo#105602, fdo#103166) -> FAIL (fdo#103166) +1

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105458 https://bugs.freedesktop.org/show_bug.cgi?id=105458
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105683 https://bugs.freedesktop.org/show_bug.cgi?id=105683
  fdo#106510 https://bugs.freedesktop.org/show_bug.cgi?id=106510
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  fdo#108074 https://bugs.freedesktop.org/show_bug.cgi?id=108074
  fdo#108130 https://bugs.freedesktop.org/show_bug.cgi?id=108130
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4922 -> Patchwork_10345

  CI_DRM_4922: 97b376a5213463a70eb977282f5486ded096648f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4665: 267870165d9ef66b4ab423e4efe7bacba023d75e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10345: b1aa286906d2a13955c417342a2df5db4943a39a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10345/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx