| Message ID | 55506C1B.1030408@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 11, 2015 at 10:45:15AM +0200, Maarten Lankhorst wrote: > With the recent modeset internal rework, we wind up setting crtc_state->enable > to false, but leave crtc_state->active as true following a > drmModeSetCrtc(fb=0), which is incorrect. This mismatch gets caught by > drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane > updates while the CRTC is disabled) to fail. > > Bisect points to > > commit dad9a7d6d96630182fb52aae7c3856e9e7285e13 > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Date: Tue Apr 21 17:13:19 2015 +0300 > > drm/i915: Use atomic helpers for computing changed flags > > as the commit that actually triggers the regression. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Reported-and-Tested-by: Kenneth Graunke <kenneth@whitecape.org> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Yeah this makes more sense, in the legacy codepaths we always implicitly smash the dpms state (i.e. state->active) to the logical pipe state (i.e. state->enable). Queued for -next, thanks for the patch. > --- > The original patch for this is wrong, we should keep enable and active in sync when changed. > Please revert "drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)". I've squashed the revert right into this one, imo didn't make sense separetely. -Daniel > > drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 22e6644f755e..e223209e2d81 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9868,7 +9868,7 @@ retry: > goto fail; > } > > - crtc_state->base.enable = true; > + crtc_state->base.active = crtc_state->base.enable = true; > > if (!mode) > mode = &load_detect_mode; > @@ -9965,7 +9965,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > connector_state->best_encoder = NULL; > connector_state->crtc = NULL; > > - crtc_state->base.enable = false; > + crtc_state->base.enable = crtc_state->base.active = false; > > ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, > 0, 0); > @@ -12492,7 +12492,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > continue; > } > > - crtc_state->base.enable = intel_crtc->new_enabled; > + crtc_state->base.active = crtc_state->base.enable = > + intel_crtc->new_enabled; > > if (&intel_crtc->base == crtc) > drm_mode_copy(&crtc_state->base.mode, &crtc->mode); > @@ -12617,11 +12618,16 @@ intel_modeset_stage_output_state(struct drm_device *dev, > } > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > + bool has_connectors; > + > ret = drm_atomic_add_affected_connectors(state, crtc); > if (ret) > return ret; > > - crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc); > + has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc); > + if (has_connectors != crtc_state->enable) > + crtc_state->enable = > + crtc_state->active = has_connectors; > } > > ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode, > @@ -14598,6 +14604,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > WARN_ON(crtc->active); > crtc->base.state->enable = false; > + crtc->base.state->active = false; > crtc->base.enabled = false; > } > > @@ -14626,6 +14633,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > crtc->active ? "enabled" : "disabled"); > > crtc->base.state->enable = crtc->active; > + crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > > /* Because we only establish the connector -> encoder -> > @@ -14764,6 +14772,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > crtc->config); > > crtc->base.state->enable = crtc->active; > + crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > > plane_state = to_intel_plane_state(primary->state); > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 22e6644f755e..e223209e2d81 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9868,7 +9868,7 @@ retry: goto fail; } - crtc_state->base.enable = true; + crtc_state->base.active = crtc_state->base.enable = true; if (!mode) mode = &load_detect_mode; @@ -9965,7 +9965,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, connector_state->best_encoder = NULL; connector_state->crtc = NULL; - crtc_state->base.enable = false; + crtc_state->base.enable = crtc_state->base.active = false; ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, 0, 0); @@ -12492,7 +12492,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) continue; } - crtc_state->base.enable = intel_crtc->new_enabled; + crtc_state->base.active = crtc_state->base.enable = + intel_crtc->new_enabled; if (&intel_crtc->base == crtc) drm_mode_copy(&crtc_state->base.mode, &crtc->mode); @@ -12617,11 +12618,16 @@ intel_modeset_stage_output_state(struct drm_device *dev, } for_each_crtc_in_state(state, crtc, crtc_state, i) { + bool has_connectors; + ret = drm_atomic_add_affected_connectors(state, crtc); if (ret) return ret; - crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc); + has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc); + if (has_connectors != crtc_state->enable) + crtc_state->enable = + crtc_state->active = has_connectors; } ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode, @@ -14598,6 +14604,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(crtc->active); crtc->base.state->enable = false; + crtc->base.state->active = false; crtc->base.enabled = false; } @@ -14626,6 +14633,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) crtc->active ? "enabled" : "disabled"); crtc->base.state->enable = crtc->active; + crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; /* Because we only establish the connector -> encoder -> @@ -14764,6 +14772,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->config); crtc->base.state->enable = crtc->active; + crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; plane_state = to_intel_plane_state(primary->state);
With the recent modeset internal rework, we wind up setting crtc_state->enable to false, but leave crtc_state->active as true following a drmModeSetCrtc(fb=0), which is incorrect. This mismatch gets caught by drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane updates while the CRTC is disabled) to fail. Bisect points to commit dad9a7d6d96630182fb52aae7c3856e9e7285e13 Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Date: Tue Apr 21 17:13:19 2015 +0300 drm/i915: Use atomic helpers for computing changed flags as the commit that actually triggers the regression. Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Reported-and-Tested-by: Kenneth Graunke <kenneth@whitecape.org> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- The original patch for this is wrong, we should keep enable and active in sync when changed. Please revert "drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)". drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)