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@xxxxxxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxxxxxx # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe > > > > Cc: stable@xxxxxxxxxxxxxxx > > > > Cc: Dennis <dennis.nezic@xxxxxxxxxxx> > > > > Tested-by: Dennis <dennis.nezic@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > > 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
