Message ID | 1431034288-3160-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, 2015-05-07 at 14:31 -0700, Matt Roper wrote: > With the recent modeset internal rework, we wind up setting > crtc_state->enable to false, but leave crtc_state->active as true, 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. > > v2: Update to alter in-flight state rather than already-committed state > (first version was accidentally based on a midpoint of Ander's > modeset rework series, before his final patches that add proper > state swapping to the legacy modeset path). > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Testcase: igt/kms_universal_plane > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index c297cdc..981478a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > continue; > > if (!crtc_state->enable) { > + crtc_state->active = false; I believe a more appropriate fix would be to set crtc_state->active when we set crtc_state->enable to false in intel_modeset_stage_output_state() and the HW state read out. But I think Maarten does that in his later patch series, so as a stopgap solution this is fine. Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > intel_crtc_disable(crtc); > } else if (crtc->state->enable) { > intel_crtc_disable_planes(crtc);
On Fri, May 08, 2015 at 10:56:03AM +0300, Ander Conselvan De Oliveira wrote: > On Thu, 2015-05-07 at 14:31 -0700, Matt Roper wrote: > > With the recent modeset internal rework, we wind up setting > > crtc_state->enable to false, but leave crtc_state->active as true, 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. > > > > v2: Update to alter in-flight state rather than already-committed state > > (first version was accidentally based on a midpoint of Ander's > > modeset rework series, before his final patches that add proper > > state swapping to the legacy modeset path). > > > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Testcase: igt/kms_universal_plane > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index c297cdc..981478a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > > continue; > > > > if (!crtc_state->enable) { > > + crtc_state->active = false; > > I believe a more appropriate fix would be to set crtc_state->active when > we set crtc_state->enable to false in intel_modeset_stage_output_state() > and the HW state read out. But I think Maarten does that in his later > patch series, so as a stopgap solution this is fine. > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> Queued for -next, thanks for the patch. -Daniel
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6352
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 302/302 302/302
SNB -1 314/314 313/314
IVB 338/338 338/338
BYT 286/286 286/286
BDW 320/320 320/320
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
SNB igt@pm_rpm@dpms-mode-unset-non-lpsp DMESG_WARN(2)PASS(1) DMESG_WARN(2)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c297cdc..981478a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, continue; if (!crtc_state->enable) { + crtc_state->active = false; intel_crtc_disable(crtc); } else if (crtc->state->enable) { intel_crtc_disable_planes(crtc);
With the recent modeset internal rework, we wind up setting crtc_state->enable to false, but leave crtc_state->active as true, 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. v2: Update to alter in-flight state rather than already-committed state (first version was accidentally based on a midpoint of Ander's modeset rework series, before his final patches that add proper state swapping to the legacy modeset path). Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Testcase: igt/kms_universal_plane Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 1 + 1 file changed, 1 insertion(+)