| Message ID | 20181003145017.4527-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/i915: Restore vblank interrupts earlier | expand |
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 >
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
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];