| Message ID | 1433162483-25476-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-06-01 at 15:41 +0300, Ander Conselvan de Oliveira wrote: > Since the force restore logic will restore the CRTCs state one at a > time, it is possible that the state will be inconsistent until the whole > operation finishes. A call to intel_modeset_check_state() is done once > it's over, so don't check the state multiple times in between. This > regression was introduced in: > > commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Wed Nov 5 14:26:06 2014 -0800 > > drm/i915: factor out compute_config from __intel_set_mode v3 > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > > Hi, > > This patch applies on top of nightly, but it is only relevant without > Maarten's "drm/i915: Convert to atomic, part 2" series, because of the > changes to the hw state read out and force restore logic. > > The regression exists since 3.19. Same patch adapated for 4.1-rc6 here: https://bugzilla.kernel.org/attachment.cgi?id=178461 Ander > --- > drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 16e159d..24fb7ce 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -87,7 +87,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config); > > static int intel_set_mode(struct drm_crtc *crtc, > - struct drm_atomic_state *state); > + struct drm_atomic_state *state, > + bool check); > static int intel_framebuffer_init(struct drm_device *dev, > struct intel_framebuffer *ifb, > struct drm_mode_fb_cmd2 *mode_cmd, > @@ -10282,7 +10283,7 @@ retry: > > drm_mode_copy(&crtc_state->base.mode, mode); > > - if (intel_set_mode(crtc, state)) { > + if (intel_set_mode(crtc, state, true)) { > DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); > if (old->release_fb) > old->release_fb->funcs->destroy(old->release_fb); > @@ -10356,7 +10357,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > if (ret) > goto fail; > > - ret = intel_set_mode(crtc, state); > + ret = intel_set_mode(crtc, state, true); > if (ret) > goto fail; > > @@ -12832,20 +12833,22 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > } > > static int intel_set_mode_with_config(struct drm_crtc *crtc, > - struct intel_crtc_state *pipe_config) > + struct intel_crtc_state *pipe_config, > + bool check) > { > int ret; > > ret = __intel_set_mode(crtc, pipe_config); > > - if (ret == 0) > + if (ret == 0 && check) > intel_modeset_check_state(crtc->dev); > > return ret; > } > > static int intel_set_mode(struct drm_crtc *crtc, > - struct drm_atomic_state *state) > + struct drm_atomic_state *state, > + bool check) > { > struct intel_crtc_state *pipe_config; > int ret = 0; > @@ -12856,7 +12859,7 @@ static int intel_set_mode(struct drm_crtc *crtc, > goto out; > } > > - ret = intel_set_mode_with_config(crtc, pipe_config); > + ret = intel_set_mode_with_config(crtc, pipe_config, check); > if (ret) > goto out; > > @@ -12933,7 +12936,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > intel_modeset_setup_plane_state(state, crtc, &crtc->mode, > crtc->primary->fb, crtc->x, crtc->y); > > - ret = intel_set_mode(crtc, state); > + ret = intel_set_mode(crtc, state, false); > if (ret) > drm_atomic_state_free(state); > } > @@ -13133,7 +13136,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > primary_plane_was_visible = primary_plane_visible(set->crtc); > > - ret = intel_set_mode_with_config(set->crtc, pipe_config); > + ret = intel_set_mode_with_config(set->crtc, pipe_config, true); > > if (ret == 0 && > pipe_config->base.enable && --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6514
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 303/303 303/303
SNB -1 315/315 314/315
IVB 343/343 343/343
BYT 287/287 287/287
BDW -1 321/321 320/321
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*SNB igt@pm_rpm@dpms-mode-unset-non-lpsp PASS(1) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW igt@gem_flink@basic PASS(1) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > Since the force restore logic will restore the CRTCs state one at a > time, it is possible that the state will be inconsistent until the whole > operation finishes. A call to intel_modeset_check_state() is done once > it's over, so don't check the state multiple times in between. This > regression was introduced in: > > commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Wed Nov 5 14:26:06 2014 -0800 > > drm/i915: factor out compute_config from __intel_set_mode v3 > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > > Hi, > > This patch applies on top of nightly, but it is only relevant without > Maarten's "drm/i915: Convert to atomic, part 2" series, because of the > changes to the hw state read out and force restore logic. > > The regression exists since 3.19. Sooo, I think this should be applied to fixes, with cc: stable v3.19+, and IIUC Maarten's series makes this obsolete in dinq? Now we just need review... Maarten? BR, Jani. > > --- > drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 16e159d..24fb7ce 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -87,7 +87,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config); > > static int intel_set_mode(struct drm_crtc *crtc, > - struct drm_atomic_state *state); > + struct drm_atomic_state *state, > + bool check); > static int intel_framebuffer_init(struct drm_device *dev, > struct intel_framebuffer *ifb, > struct drm_mode_fb_cmd2 *mode_cmd, > @@ -10282,7 +10283,7 @@ retry: > > drm_mode_copy(&crtc_state->base.mode, mode); > > - if (intel_set_mode(crtc, state)) { > + if (intel_set_mode(crtc, state, true)) { > DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); > if (old->release_fb) > old->release_fb->funcs->destroy(old->release_fb); > @@ -10356,7 +10357,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > if (ret) > goto fail; > > - ret = intel_set_mode(crtc, state); > + ret = intel_set_mode(crtc, state, true); > if (ret) > goto fail; > > @@ -12832,20 +12833,22 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > } > > static int intel_set_mode_with_config(struct drm_crtc *crtc, > - struct intel_crtc_state *pipe_config) > + struct intel_crtc_state *pipe_config, > + bool check) > { > int ret; > > ret = __intel_set_mode(crtc, pipe_config); > > - if (ret == 0) > + if (ret == 0 && check) > intel_modeset_check_state(crtc->dev); > > return ret; > } > > static int intel_set_mode(struct drm_crtc *crtc, > - struct drm_atomic_state *state) > + struct drm_atomic_state *state, > + bool check) > { > struct intel_crtc_state *pipe_config; > int ret = 0; > @@ -12856,7 +12859,7 @@ static int intel_set_mode(struct drm_crtc *crtc, > goto out; > } > > - ret = intel_set_mode_with_config(crtc, pipe_config); > + ret = intel_set_mode_with_config(crtc, pipe_config, check); > if (ret) > goto out; > > @@ -12933,7 +12936,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > intel_modeset_setup_plane_state(state, crtc, &crtc->mode, > crtc->primary->fb, crtc->x, crtc->y); > > - ret = intel_set_mode(crtc, state); > + ret = intel_set_mode(crtc, state, false); > if (ret) > drm_atomic_state_free(state); > } > @@ -13133,7 +13136,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > primary_plane_was_visible = primary_plane_visible(set->crtc); > > - ret = intel_set_mode_with_config(set->crtc, pipe_config); > + ret = intel_set_mode_with_config(set->crtc, pipe_config, true); > > if (ret == 0 && > pipe_config->base.enable && > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 02-06-15 om 09:12 schreef Jani Nikula: > On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: >> Since the force restore logic will restore the CRTCs state one at a >> time, it is possible that the state will be inconsistent until the whole >> operation finishes. A call to intel_modeset_check_state() is done once >> it's over, so don't check the state multiple times in between. This >> regression was introduced in: >> >> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f >> Author: Jesse Barnes <jbarnes@virtuousgeek.org> >> Date: Wed Nov 5 14:26:06 2014 -0800 >> >> drm/i915: factor out compute_config from __intel_set_mode v3 >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> --- >> >> Hi, >> >> This patch applies on top of nightly, but it is only relevant without >> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the >> changes to the hw state read out and force restore logic. >> >> The regression exists since 3.19. > Sooo, I think this should be applied to fixes, with cc: stable v3.19+, > and IIUC Maarten's series makes this obsolete in dinq? > > Now we just need review... Maarten? > > BR, > Jani. Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Looks good to me, but it will conflict with my own patch series. :( ~Maarten
On Tue, 2015-06-02 at 09:27 +0200, Maarten Lankhorst wrote: > Op 02-06-15 om 09:12 schreef Jani Nikula: > > On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > >> Since the force restore logic will restore the CRTCs state one at a > >> time, it is possible that the state will be inconsistent until the whole > >> operation finishes. A call to intel_modeset_check_state() is done once > >> it's over, so don't check the state multiple times in between. This > >> regression was introduced in: > >> > >> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f > >> Author: Jesse Barnes <jbarnes@virtuousgeek.org> > >> Date: Wed Nov 5 14:26:06 2014 -0800 > >> > >> drm/i915: factor out compute_config from __intel_set_mode v3 > >> > >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 > >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > >> --- > >> > >> Hi, > >> > >> This patch applies on top of nightly, but it is only relevant without > >> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the > >> changes to the hw state read out and force restore logic. > >> > >> The regression exists since 3.19. > > Sooo, I think this should be applied to fixes, with cc: stable v3.19+, > > and IIUC Maarten's series makes this obsolete in dinq? > > > > Now we just need review... Maarten? > > > > BR, > > Jani. > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Looks good to me, but it will conflict with my own patch series. :( I think it's fine to skip this for dinq, since we move to a single modeset in the force restore path with your patch series. I just got confused with what branch to base this on. Thanks for reviewing. Ander
On Tue, 02 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: > On Tue, 2015-06-02 at 09:27 +0200, Maarten Lankhorst wrote: >> Op 02-06-15 om 09:12 schreef Jani Nikula: >> > On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: >> >> Since the force restore logic will restore the CRTCs state one at a >> >> time, it is possible that the state will be inconsistent until the whole >> >> operation finishes. A call to intel_modeset_check_state() is done once >> >> it's over, so don't check the state multiple times in between. This >> >> regression was introduced in: >> >> >> >> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f >> >> Author: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> Date: Wed Nov 5 14:26:06 2014 -0800 >> >> >> >> drm/i915: factor out compute_config from __intel_set_mode v3 >> >> >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 >> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> >> --- >> >> >> >> Hi, >> >> >> >> This patch applies on top of nightly, but it is only relevant without >> >> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the >> >> changes to the hw state read out and force restore logic. >> >> >> >> The regression exists since 3.19. >> > Sooo, I think this should be applied to fixes, with cc: stable v3.19+, >> > and IIUC Maarten's series makes this obsolete in dinq? >> > >> > Now we just need review... Maarten? >> > >> > BR, >> > Jani. >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> >> Looks good to me, but it will conflict with my own patch series. :( > > I think it's fine to skip this for dinq, since we move to a single > modeset in the force restore path with your patch series. I just got > confused with what branch to base this on. Thanks for reviewing. Looks like this conflicts even more between v4.1-rc and what's queued for v4.2. I'll pass this for now, I'm think it may be easier to fix this for v4.2 and backport from there. BR, Jani. > > Ander > > > >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 16e159d..24fb7ce 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -87,7 +87,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config); static int intel_set_mode(struct drm_crtc *crtc, - struct drm_atomic_state *state); + struct drm_atomic_state *state, + bool check); static int intel_framebuffer_init(struct drm_device *dev, struct intel_framebuffer *ifb, struct drm_mode_fb_cmd2 *mode_cmd, @@ -10282,7 +10283,7 @@ retry: drm_mode_copy(&crtc_state->base.mode, mode); - if (intel_set_mode(crtc, state)) { + if (intel_set_mode(crtc, state, true)) { DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); if (old->release_fb) old->release_fb->funcs->destroy(old->release_fb); @@ -10356,7 +10357,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (ret) goto fail; - ret = intel_set_mode(crtc, state); + ret = intel_set_mode(crtc, state, true); if (ret) goto fail; @@ -12832,20 +12833,22 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, } static int intel_set_mode_with_config(struct drm_crtc *crtc, - struct intel_crtc_state *pipe_config) + struct intel_crtc_state *pipe_config, + bool check) { int ret; ret = __intel_set_mode(crtc, pipe_config); - if (ret == 0) + if (ret == 0 && check) intel_modeset_check_state(crtc->dev); return ret; } static int intel_set_mode(struct drm_crtc *crtc, - struct drm_atomic_state *state) + struct drm_atomic_state *state, + bool check) { struct intel_crtc_state *pipe_config; int ret = 0; @@ -12856,7 +12859,7 @@ static int intel_set_mode(struct drm_crtc *crtc, goto out; } - ret = intel_set_mode_with_config(crtc, pipe_config); + ret = intel_set_mode_with_config(crtc, pipe_config, check); if (ret) goto out; @@ -12933,7 +12936,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) intel_modeset_setup_plane_state(state, crtc, &crtc->mode, crtc->primary->fb, crtc->x, crtc->y); - ret = intel_set_mode(crtc, state); + ret = intel_set_mode(crtc, state, false); if (ret) drm_atomic_state_free(state); } @@ -13133,7 +13136,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) primary_plane_was_visible = primary_plane_visible(set->crtc); - ret = intel_set_mode_with_config(set->crtc, pipe_config); + ret = intel_set_mode_with_config(set->crtc, pipe_config, true); if (ret == 0 && pipe_config->base.enable &&
Since the force restore logic will restore the CRTCs state one at a time, it is possible that the state will be inconsistent until the whole operation finishes. A call to intel_modeset_check_state() is done once it's over, so don't check the state multiple times in between. This regression was introduced in: commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Wed Nov 5 14:26:06 2014 -0800 drm/i915: factor out compute_config from __intel_set_mode v3 Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- Hi, This patch applies on top of nightly, but it is only relevant without Maarten's "drm/i915: Convert to atomic, part 2" series, because of the changes to the hw state read out and force restore logic. The regression exists since 3.19. --- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)