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]");
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];
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_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;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
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_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;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
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_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;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
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_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;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
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];
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_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;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
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_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;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
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
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]");
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Ville Syrjala -
Ville Syrjälä