On Wed, Oct 11, 2017 at 07:04:48PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <[email protected]> > > Unify the plane disabling during state readout by pulling the code into > a new helper intel_plane_disable_noatomic(). We'll also read out the > state of all planes, so that we know which planes really need to be > diabled. > > Additonally we change the plane<->pipe mapping sanitation to work by > simply disabling the offending planes instead of entire pipes. And > we do it before we otherwise sanitize the crtcs, which means we don't > have to worry about misassigned planes during crtc sanitation anymore. > > v2: Reoder patches to not depend on enum old_plane_id > > Cc: Thierry Reding <[email protected]> > Cc: Alex Villacís Lasso <[email protected]> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103223 > Signed-off-by: Ville Syrjälä <[email protected]> > --- > drivers/gpu/drm/i915/intel_display.c | 116 > ++++++++++++++++++++--------------- > 1 file changed, 67 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 825ab00b6639..a9fd3b8fa922 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2729,6 +2729,23 @@ intel_set_plane_visible(struct intel_crtc_state > *crtc_state, > crtc_state->active_planes); > } > > +static void intel_plane_disable_noatomic(struct intel_crtc *crtc, > + struct intel_plane *plane) > +{ > + struct intel_crtc_state *crtc_state = > + to_intel_crtc_state(crtc->base.state); > + struct intel_plane_state *plane_state = > + to_intel_plane_state(plane->base.state); > + > + intel_set_plane_visible(crtc_state, plane_state, false); > + > + if (plane->id == PLANE_PRIMARY) > + intel_pre_disable_primary_noatomic(&crtc->base); > + > + trace_intel_disable_plane(&plane->base, crtc); > + plane->disable_plane(plane, crtc); > +}
Wooot, I asked Maarten ages ago to extract this, thanks for doing this! Just for that: Reviewed-by: Daniel Vetter <[email protected]> (ah no I'm kidding, I did check a few things, but thankfully CI now at least covers some gen3 fun). Cheers, Daniel > + > static void > intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > struct intel_initial_plane_config *plane_config) > @@ -2786,12 +2803,7 @@ intel_find_initial_plane_obj(struct intel_crtc > *intel_crtc, > * simplest solution is to just disable the primary plane now and > * pretend the BIOS never had it enabled. > */ > - intel_set_plane_visible(to_intel_crtc_state(crtc_state), > - to_intel_plane_state(plane_state), > - false); > - intel_pre_disable_primary_noatomic(&intel_crtc->base); > - trace_intel_disable_plane(primary, intel_crtc); > - intel_plane->disable_plane(intel_plane, intel_crtc); > + intel_plane_disable_noatomic(intel_crtc, intel_plane); > > return; > > @@ -5923,6 +5935,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc > *crtc, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > enum intel_display_power_domain domain; > + struct intel_plane *plane; > u64 domains; > struct drm_atomic_state *state; > struct intel_crtc_state *crtc_state; > @@ -5931,11 +5944,12 @@ static void intel_crtc_disable_noatomic(struct > drm_crtc *crtc, > if (!intel_crtc->active) > return; > > - if (crtc->primary->state->visible) { > - intel_pre_disable_primary_noatomic(crtc); > + for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) { > + const struct intel_plane_state *plane_state = > + to_intel_plane_state(plane->base.state); > > - intel_crtc_disable_planes(crtc, 1 << > drm_plane_index(crtc->primary)); > - crtc->primary->state->visible = false; > + if (plane_state->base.visible) > + intel_plane_disable_noatomic(intel_crtc, plane); > } > > state = drm_atomic_state_alloc(crtc->dev); > @@ -14678,22 +14692,38 @@ void i830_disable_pipe(struct drm_i915_private > *dev_priv, enum pipe pipe) > POSTING_READ(DPLL(pipe)); > } > > -static bool > -intel_check_plane_mapping(struct intel_crtc *crtc) > +static bool intel_plane_mapping_ok(struct intel_crtc *crtc, > + struct intel_plane *primary) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - u32 val; > + enum plane plane = primary->plane; > + u32 val = I915_READ(DSPCNTR(plane)); > > - if (INTEL_INFO(dev_priv)->num_pipes == 1) > - return true; > + return (val & DISPLAY_PLANE_ENABLE) == 0 || > + (val & DISPPLANE_SEL_PIPE_MASK) == > DISPPLANE_SEL_PIPE(crtc->pipe); > +} > > - val = I915_READ(DSPCNTR(!crtc->plane)); > +static void > +intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv) > +{ > + enum pipe pipe; > > - if ((val & DISPLAY_PLANE_ENABLE) && > - (!!(val & DISPPLANE_SEL_PIPE_MASK) == crtc->pipe)) > - return false; > + if (INTEL_GEN(dev_priv) >= 4) > + return; > > - return true; > + for_each_pipe(dev_priv, pipe) { > + struct intel_crtc *crtc = > + intel_get_crtc_for_pipe(dev_priv, pipe); > + struct intel_plane *plane = > + to_intel_plane(crtc->base.primary); > + > + if (intel_plane_mapping_ok(crtc, plane)) > + continue; > + > + DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling > plane\n", > + plane->base.name); > + intel_plane_disable_noatomic(crtc, plane); > + } > } > > static bool intel_crtc_has_encoders(struct intel_crtc *crtc) > @@ -14749,33 +14779,15 @@ static void intel_sanitize_crtc(struct intel_crtc > *crtc, > > /* Disable everything but the primary plane */ > for_each_intel_plane_on_crtc(dev, crtc, plane) { > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > - continue; > + const struct intel_plane_state *plane_state = > + to_intel_plane_state(plane->base.state); > > - trace_intel_disable_plane(&plane->base, crtc); > - plane->disable_plane(plane, crtc); > + if (plane_state->base.visible && > + plane->base.type != DRM_PLANE_TYPE_PRIMARY) > + intel_plane_disable_noatomic(crtc, plane); > } > } > > - /* We need to sanitize the plane -> pipe mapping first because this will > - * disable the crtc (and hence change the state) if it is wrong. Note > - * that gen4+ has a fixed plane -> pipe mapping. */ > - if (INTEL_GEN(dev_priv) < 4 && !intel_check_plane_mapping(crtc)) { > - bool plane; > - > - DRM_DEBUG_KMS("[CRTC:%d:%s] wrong plane connection detected!\n", > - crtc->base.base.id, crtc->base.name); > - > - /* Pipe has the wrong plane attached and the plane is active. > - * Temporarily change the plane mapping and disable everything > - * ... */ > - plane = crtc->plane; > - crtc->base.primary->state->visible = true; > - crtc->plane = !plane; > - intel_crtc_disable_noatomic(&crtc->base, ctx); > - crtc->plane = plane; > - } > - > /* Adjust the state of the output pipe according to whether we > * have active connectors/encoders. */ > if (crtc->active && !intel_crtc_has_encoders(crtc)) > @@ -14883,14 +14895,18 @@ 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) > { > - struct intel_plane *primary = to_intel_plane(crtc->base.primary); > - bool visible; > + 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; > > - visible = crtc->active && primary->get_hw_state(primary); > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) { > + struct intel_plane_state *plane_state = > + to_intel_plane_state(plane->base.state); > + bool visible = plane->get_hw_state(plane); > > - intel_set_plane_visible(to_intel_crtc_state(crtc->base.state), > - to_intel_plane_state(primary->base.state), > - visible); > + intel_set_plane_visible(crtc_state, plane_state, visible); > + } > } > > static void intel_modeset_readout_hw_state(struct drm_device *dev) > @@ -15079,6 +15095,8 @@ 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_encoder(dev, encoder) { > intel_sanitize_encoder(encoder); > } > -- > 2.13.6 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
