| Message ID | 1457135979-23727-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matt, On Fri, 2016-03-04 at 15:59 -0800, Matt Roper wrote: > At the end of an atomic commit, we currently wait for vblanks to > complete, call put() on the various runtime PM references, and then > try > to optimize our watermarks (on platforms that need two-step watermark > programming). This can lead to watermark registers being programmed > while the power well is powered down. We need to wait until after > watermark optimization is complete before dropping our runtime power > references. > > Note that in the future the watermark optimization is probably going > to > move to an asynchronous workqueue task that happens at some arbitrary > point after vblank. When we make that change, we'll no longer > necessarily be operating under the power reference held here, so > we'll > need to wrap the watermark register programmin in a call to > intel_runtime_pm_get_if_in_use() or similar. > > Cc: arun.siluvery@linux.intel.com > Cc: ville.syrjala@linux.intel.com > Cc: maarten.lankhorst@linux.intel.com > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark > programming (v11)") > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 62d36a7..0af08d7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct > drm_device *dev, > if (!state->legacy_cursor_update) > intel_atomic_wait_for_vblanks(dev, dev_priv, > crtc_vblank_mask); > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > - intel_post_plane_update(to_intel_crtc(crtc)); > - > - if (put_domains[i]) > - modeset_put_power_domains(dev_priv, > put_domains[i]); > - } > - > - if (intel_state->modeset) > - intel_display_power_put(dev_priv, > POWER_DOMAIN_MODESET); > - > /* > * Now that the vblank has passed, we can go ahead and > program the > * optimal watermarks on platforms that need two-step > watermark > @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct > drm_device *dev, > dev_priv- > >display.optimize_watermarks(intel_cstate); > } > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + intel_post_plane_update(to_intel_crtc(crtc)); > + > + if (put_domains[i]) > + modeset_put_power_domains(dev_priv, > put_domains[i]); > + } > + > + if (intel_state->modeset) > +> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > + Since the error was caused by writing some WM register while the HW is suspended, I don't see what would be the point of programming it if the register loses its value anyway after dropping the power reference. What would make sense to me is to avoid programming the WM registers if all the outputs are disabled (like they were when the bug triggered) and program them next time around when any output gets enabled. --Imre > mutex_lock(&dev->struct_mutex); > drm_atomic_helper_cleanup_planes(dev, state); > mutex_unlock(&dev->struct_mutex);
Op 05-03-16 om 00:59 schreef Matt Roper: > At the end of an atomic commit, we currently wait for vblanks to > complete, call put() on the various runtime PM references, and then try > to optimize our watermarks (on platforms that need two-step watermark > programming). This can lead to watermark registers being programmed > while the power well is powered down. We need to wait until after > watermark optimization is complete before dropping our runtime power > references. > > Note that in the future the watermark optimization is probably going to > move to an asynchronous workqueue task that happens at some arbitrary > point after vblank. When we make that change, we'll no longer > necessarily be operating under the power reference held here, so we'll > need to wrap the watermark register programmin in a call to > intel_runtime_pm_get_if_in_use() or similar. > > Cc: arun.siluvery@linux.intel.com > Cc: ville.syrjala@linux.intel.com > Cc: maarten.lankhorst@linux.intel.com > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)") > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> post_plane_update can call intel_update_watermarks, will this cause any unintended behavioral changes if intel_update_watermarks is called before optimize_watermarks? ~Maarten
On Sat, Mar 05, 2016 at 03:25:05AM +0200, Imre Deak wrote: > Hi Matt, > > On Fri, 2016-03-04 at 15:59 -0800, Matt Roper wrote: > > At the end of an atomic commit, we currently wait for vblanks to > > complete, call put() on the various runtime PM references, and then > > try > > to optimize our watermarks (on platforms that need two-step watermark > > programming). This can lead to watermark registers being programmed > > while the power well is powered down. We need to wait until after > > watermark optimization is complete before dropping our runtime power > > references. > > > > Note that in the future the watermark optimization is probably going > > to > > move to an asynchronous workqueue task that happens at some arbitrary > > point after vblank. When we make that change, we'll no longer > > necessarily be operating under the power reference held here, so > > we'll > > need to wrap the watermark register programmin in a call to > > intel_runtime_pm_get_if_in_use() or similar. > > > > Cc: arun.siluvery@linux.intel.com > > Cc: ville.syrjala@linux.intel.com > > Cc: maarten.lankhorst@linux.intel.com > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark > > programming (v11)") > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 62d36a7..0af08d7 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > if (!state->legacy_cursor_update) > > intel_atomic_wait_for_vblanks(dev, dev_priv, > > crtc_vblank_mask); > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > - intel_post_plane_update(to_intel_crtc(crtc)); > > - > > - if (put_domains[i]) > > - modeset_put_power_domains(dev_priv, > > put_domains[i]); > > - } > > - > > - if (intel_state->modeset) > > - intel_display_power_put(dev_priv, > > POWER_DOMAIN_MODESET); > > - > > /* > > * Now that the vblank has passed, we can go ahead and > > program the > > * optimal watermarks on platforms that need two-step > > watermark > > @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct > > drm_device *dev, > > dev_priv- > > >display.optimize_watermarks(intel_cstate); > > } > > > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > + intel_post_plane_update(to_intel_crtc(crtc)); > > + > > + if (put_domains[i]) > > + modeset_put_power_domains(dev_priv, > > put_domains[i]); > > + } > > + > > + if (intel_state->modeset) > > +> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > + > > Since the error was caused by writing some WM register while the HW is > suspended, I don't see what would be the point of programming it if the > register loses its value anyway after dropping the power reference. > What would make sense to me is to avoid programming the WM registers if > all the outputs are disabled (like they were when the bug triggered) > and program them next time around when any output gets enabled. Yeah, that was actually the first approach I went with: https://patchwork.freedesktop.org/patch/75772/ Ville felt like it was more of a workaround than a fix though, so I posted this alternative instead. Matt > > --Imre > > > > mutex_lock(&dev->struct_mutex); > > drm_atomic_helper_cleanup_planes(dev, state); > > mutex_unlock(&dev->struct_mutex);
On Mon, Mar 07, 2016 at 12:53:38PM +0100, Maarten Lankhorst wrote: > Op 05-03-16 om 00:59 schreef Matt Roper: > > At the end of an atomic commit, we currently wait for vblanks to > > complete, call put() on the various runtime PM references, and then try > > to optimize our watermarks (on platforms that need two-step watermark > > programming). This can lead to watermark registers being programmed > > while the power well is powered down. We need to wait until after > > watermark optimization is complete before dropping our runtime power > > references. > > > > Note that in the future the watermark optimization is probably going to > > move to an asynchronous workqueue task that happens at some arbitrary > > point after vblank. When we make that change, we'll no longer > > necessarily be operating under the power reference held here, so we'll > > need to wrap the watermark register programmin in a call to > > intel_runtime_pm_get_if_in_use() or similar. > > > > Cc: arun.siluvery@linux.intel.com > > Cc: ville.syrjala@linux.intel.com > > Cc: maarten.lankhorst@linux.intel.com > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)") > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > post_plane_update can call intel_update_watermarks, will this cause any unintended behavioral changes > if intel_update_watermarks is called before optimize_watermarks? It shouldn't; intel_update_watermarks is the legacy watermark programming function. Any platform that that's been converted to atomic shouldn't have a dev_priv->display.update_wm vfunc, so intel_update_watermarks will be a noop. Matt > > ~Maarten
On ma, 2016-03-07 at 08:10 -0800, Matt Roper wrote: > On Sat, Mar 05, 2016 at 03:25:05AM +0200, Imre Deak wrote: > > Hi Matt, > > > > On Fri, 2016-03-04 at 15:59 -0800, Matt Roper wrote: > > > At the end of an atomic commit, we currently wait for vblanks to > > > complete, call put() on the various runtime PM references, and then > > > try > > > to optimize our watermarks (on platforms that need two-step watermark > > > programming). This can lead to watermark registers being programmed > > > while the power well is powered down. We need to wait until after > > > watermark optimization is complete before dropping our runtime power > > > references. > > > > > > Note that in the future the watermark optimization is probably going > > > to > > > move to an asynchronous workqueue task that happens at some arbitrary > > > point after vblank. When we make that change, we'll no longer > > > necessarily be operating under the power reference held here, so > > > we'll > > > need to wrap the watermark register programmin in a call to > > > intel_runtime_pm_get_if_in_use() or similar. > > > > > > Cc: arun.siluvery@linux.intel.com > > > Cc: ville.syrjala@linux.intel.com > > > Cc: maarten.lankhorst@linux.intel.com > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark > > > programming (v11)") > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 62d36a7..0af08d7 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct > > > drm_device *dev, > > > if (!state->legacy_cursor_update) > > > intel_atomic_wait_for_vblanks(dev, dev_priv, > > > crtc_vblank_mask); > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > - intel_post_plane_update(to_intel_crtc(crtc)); > > > - > > > - if (put_domains[i]) > > > - modeset_put_power_domains(dev_priv, > > > put_domains[i]); > > > - } > > > - > > > - if (intel_state->modeset) > > > - intel_display_power_put(dev_priv, > > > POWER_DOMAIN_MODESET); > > > - > > > /* > > > * Now that the vblank has passed, we can go ahead and > > > program the > > > * optimal watermarks on platforms that need two-step > > > watermark > > > @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct > > > drm_device *dev, > > > dev_priv- > > > > display.optimize_watermarks(intel_cstate); > > > } > > > > > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > + intel_post_plane_update(to_intel_crtc(crtc)); > > > + > > > + if (put_domains[i]) > > > + modeset_put_power_domains(dev_priv, > > > put_domains[i]); > > > + } > > > + > > > + if (intel_state->modeset) > > > +> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > > + > > > > Since the error was caused by writing some WM register while the HW is > > suspended, I don't see what would be the point of programming it if the > > register loses its value anyway after dropping the power reference. > > What would make sense to me is to avoid programming the WM registers if > > all the outputs are disabled (like they were when the bug triggered) > > and program them next time around when any output gets enabled. > > Yeah, that was actually the first approach I went with: > > https://patchwork.freedesktop.org/patch/75772/ > > Ville felt like it was more of a workaround than a fix though, so I > posted this alternative instead. Ok, I haven't noticed that patch. One other thing that I realized only after sending my email is that - on some old platforms at least - it would still make sense to program the WM state even with all outputs disabled: the description of FW_BLC_Self[15] on GEN3 for example could mean that memory self refresh is disabled whenever we set this bit to 0, independently of the display output state, although this description is somewhat vague. --Imre
On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Wait until after wm optimization to drop runtime PM reference > URL : https://patchwork.freedesktop.org/series/4135/ > State : failure > > == Summary == > > Series 4135v1 drm/i915: Wait until after wm optimization to drop runtime PM reference > http://patchwork.freedesktop.org/api/1.0/series/4135/revisions/1/mbox/ > > Test kms_flip: > Subgroup basic-flip-vs-wf_vblank: > fail -> PASS (bsw-nuc-2) > Subgroup basic-plain-flip: > dmesg-warn -> PASS (hsw-gt2) > Test kms_force_connector_basic: > Subgroup force-load-detect: > pass -> SKIP (ivb-t430s) https://bugs.freedesktop.org/show_bug.cgi?id=93769 > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-c: > dmesg-warn -> PASS (hsw-gt2) > Subgroup suspend-read-crc-pipe-a: > dmesg-warn -> PASS (skl-i5k-2) > skip -> PASS (hsw-brixbox) > Subgroup suspend-read-crc-pipe-c: > pass -> DMESG-WARN (bsw-nuc-2) https://bugs.freedesktop.org/show_bug.cgi?id=93294 > pass -> INCOMPLETE (hsw-gt2) Seems like the machine just died completely? No stdout/stderr/command/dmesg output available from CI and time is given as 0:00:00. Doesn't seem like it could be related to this patch. > Test pm_rpm: > Subgroup basic-pci-d3-state: > dmesg-warn -> PASS (snb-dellxps) > Subgroup basic-rte: > pass -> DMESG-WARN (bsw-nuc-2) https://bugs.freedesktop.org/show_bug.cgi?id=94164 Matt > > bdw-nuci7 total:183 pass:172 dwarn:0 dfail:0 fail:0 skip:11 > bdw-ultra total:183 pass:165 dwarn:0 dfail:0 fail:0 skip:18 > bsw-nuc-2 total:183 pass:147 dwarn:2 dfail:0 fail:0 skip:34 > byt-nuc total:183 pass:152 dwarn:0 dfail:0 fail:0 skip:31 > hsw-brixbox total:183 pass:164 dwarn:0 dfail:0 fail:0 skip:19 > hsw-gt2 total:119 pass:109 dwarn:0 dfail:0 fail:0 skip:9 > ilk-hp8440p total:183 pass:125 dwarn:0 dfail:0 fail:0 skip:58 > ivb-t430s total:183 pass:161 dwarn:0 dfail:0 fail:0 skip:22 > skl-i5k-2 total:183 pass:163 dwarn:0 dfail:0 fail:0 skip:20 > skl-i7k-2 total:183 pass:163 dwarn:0 dfail:0 fail:0 skip:20 > snb-dellxps total:183 pass:154 dwarn:0 dfail:0 fail:0 skip:29 > > Results at /archive/results/CI_IGT_test/Patchwork_1532/ > > d369e0096716c6000139162b3b340f684f0a51da drm-intel-nightly: 2016y-03m-04d-17h-18m-08s UTC integration manifest > b0744e82af66e41a8dca48bfa6ff8f38e1d9454f drm/i915: Wait until after wm optimization to drop runtime PM reference >
On Fri, Mar 04, 2016 at 03:59:39PM -0800, Matt Roper wrote: > At the end of an atomic commit, we currently wait for vblanks to > complete, call put() on the various runtime PM references, and then try > to optimize our watermarks (on platforms that need two-step watermark > programming). This can lead to watermark registers being programmed > while the power well is powered down. We need to wait until after > watermark optimization is complete before dropping our runtime power > references. > > Note that in the future the watermark optimization is probably going to > move to an asynchronous workqueue task that happens at some arbitrary > point after vblank. When we make that change, we'll no longer > necessarily be operating under the power reference held here, so we'll > need to wrap the watermark register programmin in a call to > intel_runtime_pm_get_if_in_use() or similar. > > Cc: arun.siluvery@linux.intel.com > Cc: ville.syrjala@linux.intel.com > Cc: maarten.lankhorst@linux.intel.com > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)") > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 62d36a7..0af08d7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct drm_device *dev, > if (!state->legacy_cursor_update) > intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > - intel_post_plane_update(to_intel_crtc(crtc)); > - > - if (put_domains[i]) > - modeset_put_power_domains(dev_priv, put_domains[i]); > - } > - > - if (intel_state->modeset) > - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > - > /* > * Now that the vblank has passed, we can go ahead and program the > * optimal watermarks on platforms that need two-step watermark > @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct drm_device *dev, > dev_priv->display.optimize_watermarks(intel_cstate); > } > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + intel_post_plane_update(to_intel_crtc(crtc)); > + > + if (put_domains[i]) > + modeset_put_power_domains(dev_priv, put_domains[i]); > + } > + > + if (intel_state->modeset) > + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > + > mutex_lock(&dev->struct_mutex); > drm_atomic_helper_cleanup_planes(dev, state); > mutex_unlock(&dev->struct_mutex); > -- > 2.1.4
On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: > On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: > > == Series Details == > > > > Series: drm/i915: Wait until after wm optimization to drop runtime > > PM reference > > URL : https://patchwork.freedesktop.org/series/4135/ > > State : failure > > > > == Summary == > > > > Series 4135v1 drm/i915: Wait until after wm optimization to drop > > runtime PM reference > > http://patchwork.freedesktop.org/api/1.0/series/4135/revisions/1/mb > > ox/ > > > > Test kms_flip: > > Subgroup basic-flip-vs-wf_vblank: > > fail -> PASS (bsw-nuc-2) > > Subgroup basic-plain-flip: > > dmesg-warn -> PASS (hsw-gt2) > > Test kms_force_connector_basic: > > Subgroup force-load-detect: > > pass -> SKIP (ivb-t430s) > > https://bugs.freedesktop.org/show_bug.cgi?id=93769 > > > Test kms_pipe_crc_basic: > > Subgroup read-crc-pipe-c: > > dmesg-warn -> PASS (hsw-gt2) > > Subgroup suspend-read-crc-pipe-a: > > dmesg-warn -> PASS (skl-i5k-2) > > skip -> PASS (hsw-brixbox) > > Subgroup suspend-read-crc-pipe-c: > > pass -> DMESG-WARN (bsw-nuc-2) > > https://bugs.freedesktop.org/show_bug.cgi?id=93294 > > > pass -> INCOMPLETE (hsw-gt2) > > Seems like the machine just died completely? No > stdout/stderr/command/dmesg output available from CI and time is > given > as 0:00:00. Doesn't seem like it could be related to this patch. > > > > Test pm_rpm: > > Subgroup basic-pci-d3-state: > > dmesg-warn -> PASS (snb-dellxps) > > Subgroup basic-rte: > > pass -> DMESG-WARN (bsw-nuc-2) > > https://bugs.freedesktop.org/show_bug.cgi?id=94164 > > > Matt Pushed to -dinq, thanks for the patch and the review. I had to rebase it on top of Ville's recent s/crtc_state/old_crtc_state/ change, please double check it. Jani, this is for -fixes. --Imre
On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > [ text/plain ] > On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: >> On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: >> > == Series Details == >> > >> > Series: drm/i915: Wait until after wm optimization to drop runtime >> > PM reference >> > URL : https://patchwork.freedesktop.org/series/4135/ >> > State : failure >> > >> > == Summary == >> > >> > Series 4135v1 drm/i915: Wait until after wm optimization to drop >> > runtime PM reference >> > http://patchwork.freedesktop.org/api/1.0/series/4135/revisions/1/mb >> > ox/ >> > >> > Test kms_flip: >> > Subgroup basic-flip-vs-wf_vblank: >> > fail -> PASS (bsw-nuc-2) >> > Subgroup basic-plain-flip: >> > dmesg-warn -> PASS (hsw-gt2) >> > Test kms_force_connector_basic: >> > Subgroup force-load-detect: >> > pass -> SKIP (ivb-t430s) >> >> https://bugs.freedesktop.org/show_bug.cgi?id=93769 >> >> > Test kms_pipe_crc_basic: >> > Subgroup read-crc-pipe-c: >> > dmesg-warn -> PASS (hsw-gt2) >> > Subgroup suspend-read-crc-pipe-a: >> > dmesg-warn -> PASS (skl-i5k-2) >> > skip -> PASS (hsw-brixbox) >> > Subgroup suspend-read-crc-pipe-c: >> > pass -> DMESG-WARN (bsw-nuc-2) >> >> https://bugs.freedesktop.org/show_bug.cgi?id=93294 >> >> > pass -> INCOMPLETE (hsw-gt2) >> >> Seems like the machine just died completely? No >> stdout/stderr/command/dmesg output available from CI and time is >> given >> as 0:00:00. Doesn't seem like it could be related to this patch. >> >> >> > Test pm_rpm: >> > Subgroup basic-pci-d3-state: >> > dmesg-warn -> PASS (snb-dellxps) >> > Subgroup basic-rte: >> > pass -> DMESG-WARN (bsw-nuc-2) >> >> https://bugs.freedesktop.org/show_bug.cgi?id=94164 >> >> >> Matt > > Pushed to -dinq, thanks for the patch and the review. > > I had to rebase it on top of Ville's recent > s/crtc_state/old_crtc_state/ change, please double check it. > > Jani, this is for -fixes. Surely you added either Cc: drm-intel-fixes@lists.freedesktop.org or Cc: stable@vger.kernel.org in the commit when you pushed, then? BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
On ti, 2016-03-22 at 15:51 +0200, Jani Nikula wrote: > On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > > [ text/plain ] > > On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: > > > On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: > > > > == Series Details == > > > > > > > > Series: drm/i915: Wait until after wm optimization to drop > > > > runtime > > > > PM reference > > > > URL : https://patchwork.freedesktop.org/series/4135/ > > > > State : failure > > > > > > > > == Summary == > > > > > > > > Series 4135v1 drm/i915: Wait until after wm optimization to > > > > drop > > > > runtime PM reference > > > > http://patchwork.freedesktop.org/api/1.0/series/4135/revisions/ > > > > 1/mb > > > > ox/ > > > > > > > > Test kms_flip: > > > > Subgroup basic-flip-vs-wf_vblank: > > > > fail -> PASS (bsw-nuc-2) > > > > Subgroup basic-plain-flip: > > > > dmesg-warn -> PASS (hsw-gt2) > > > > Test kms_force_connector_basic: > > > > Subgroup force-load-detect: > > > > pass -> SKIP (ivb-t430s) > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93769 > > > > > > > Test kms_pipe_crc_basic: > > > > Subgroup read-crc-pipe-c: > > > > dmesg-warn -> PASS (hsw-gt2) > > > > Subgroup suspend-read-crc-pipe-a: > > > > dmesg-warn -> PASS (skl-i5k-2) > > > > skip -> PASS (hsw-brixbox) > > > > Subgroup suspend-read-crc-pipe-c: > > > > pass -> DMESG-WARN (bsw-nuc-2) > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93294 > > > > > > > pass -> INCOMPLETE (hsw-gt2) > > > > > > Seems like the machine just died completely? No > > > stdout/stderr/command/dmesg output available from CI and time is > > > given > > > as 0:00:00. Doesn't seem like it could be related to this patch. > > > > > > > > > > Test pm_rpm: > > > > Subgroup basic-pci-d3-state: > > > > dmesg-warn -> PASS (snb-dellxps) > > > > Subgroup basic-rte: > > > > pass -> DMESG-WARN (bsw-nuc-2) > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=94164 > > > > > > > > > Matt > > > > Pushed to -dinq, thanks for the patch and the review. > > > > I had to rebase it on top of Ville's recent > > s/crtc_state/old_crtc_state/ change, please double check it. > > > > Jani, this is for -fixes. > > Surely you added either > > Cc: drm-intel-fixes@lists.freedesktop.org > > or > > Cc: stable@vger.kernel.org > > in the commit when you pushed, then? No, I haven't will do that in the future. Btw, what's the rule for deciding that something is for -fixes or stable only after it's been pushed? Just ping you in case for -fixes and resend it in case of stable? --Imre
On Tue, Mar 22, 2016 at 04:17:58PM +0200, Imre Deak wrote: > On ti, 2016-03-22 at 15:51 +0200, Jani Nikula wrote: > > On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > > > [ text/plain ] > > > On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: > > > > On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: > > > > > == Series Details == > > > > > > > > > > Series: drm/i915: Wait until after wm optimization to drop > > > > > runtime > > > > > PM reference > > > > > URL : https://patchwork.freedesktop.org/series/4135/ > > > > > State : failure > > > > > > > > > > == Summary == > > > > > > > > > > Series 4135v1 drm/i915: Wait until after wm optimization to > > > > > drop > > > > > runtime PM reference > > > > > http://patchwork.freedesktop.org/api/1.0/series/4135/revisions/ > > > > > 1/mb > > > > > ox/ > > > > > > > > > > Test kms_flip: > > > > > Subgroup basic-flip-vs-wf_vblank: > > > > > fail -> PASS (bsw-nuc-2) > > > > > Subgroup basic-plain-flip: > > > > > dmesg-warn -> PASS (hsw-gt2) > > > > > Test kms_force_connector_basic: > > > > > Subgroup force-load-detect: > > > > > pass -> SKIP (ivb-t430s) > > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93769 > > > > > > > > > Test kms_pipe_crc_basic: > > > > > Subgroup read-crc-pipe-c: > > > > > dmesg-warn -> PASS (hsw-gt2) > > > > > Subgroup suspend-read-crc-pipe-a: > > > > > dmesg-warn -> PASS (skl-i5k-2) > > > > > skip -> PASS (hsw-brixbox) > > > > > Subgroup suspend-read-crc-pipe-c: > > > > > pass -> DMESG-WARN (bsw-nuc-2) > > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93294 > > > > > > > > > pass -> INCOMPLETE (hsw-gt2) > > > > > > > > Seems like the machine just died completely? No > > > > stdout/stderr/command/dmesg output available from CI and time is > > > > given > > > > as 0:00:00. Doesn't seem like it could be related to this patch. > > > > > > > > > > > > > Test pm_rpm: > > > > > Subgroup basic-pci-d3-state: > > > > > dmesg-warn -> PASS (snb-dellxps) > > > > > Subgroup basic-rte: > > > > > pass -> DMESG-WARN (bsw-nuc-2) > > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=94164 > > > > > > > > > > > > Matt > > > > > > Pushed to -dinq, thanks for the patch and the review. > > > > > > I had to rebase it on top of Ville's recent > > > s/crtc_state/old_crtc_state/ change, please double check it. > > > > > > Jani, this is for -fixes. > > > > Surely you added either > > > > Cc: drm-intel-fixes@lists.freedesktop.org > > > > or > > > > Cc: stable@vger.kernel.org > > > > in the commit when you pushed, then? > > No, I haven't will do that in the future. Btw, what's the rule for > deciding that something is for -fixes or stable only after it's been > pushed? Just ping you in case for -fixes and resend it in case of > stable? > > --Imre Are you sure we need this for -fixes? The patch that introduced the regression isn't in drm-next/4.6 as far as I know. Matt
On ti, 2016-03-22 at 08:56 -0700, Matt Roper wrote: > On Tue, Mar 22, 2016 at 04:17:58PM +0200, Imre Deak wrote: > > On ti, 2016-03-22 at 15:51 +0200, Jani Nikula wrote: > > > On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > > > > [ text/plain ] > > > > On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: > > > > > On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: > > > > > > == Series Details == > > > > > > > > > > > > Series: drm/i915: Wait until after wm optimization to drop > > > > > > runtime > > > > > > PM reference > > > > > > URL : https://patchwork.freedesktop.org/series/4135/ > > > > > > State : failure > > > > > > > > > > > > == Summary == > > > > > > > > > > > > Series 4135v1 drm/i915: Wait until after wm optimization to > > > > > > drop > > > > > > runtime PM reference > > > > > > http://patchwork.freedesktop.org/api/1.0/series/4135/revisi > > > > > > ons/ > > > > > > 1/mb > > > > > > ox/ > > > > > > > > > > > > Test kms_flip: > > > > > > Subgroup basic-flip-vs-wf_vblank: > > > > > > fail -> PASS (bsw-nuc-2) > > > > > > Subgroup basic-plain-flip: > > > > > > dmesg-warn -> PASS (hsw-gt2) > > > > > > Test kms_force_connector_basic: > > > > > > Subgroup force-load-detect: > > > > > > pass -> SKIP (ivb-t430s) > > > > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93769 > > > > > > > > > > > Test kms_pipe_crc_basic: > > > > > > Subgroup read-crc-pipe-c: > > > > > > dmesg-warn -> PASS (hsw-gt2) > > > > > > Subgroup suspend-read-crc-pipe-a: > > > > > > dmesg-warn -> PASS (skl-i5k-2) > > > > > > skip -> PASS (hsw-brixbox) > > > > > > Subgroup suspend-read-crc-pipe-c: > > > > > > pass -> DMESG-WARN (bsw-nuc-2) > > > > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93294 > > > > > > > > > > > pass -> INCOMPLETE (hsw-gt2) > > > > > > > > > > Seems like the machine just died completely? No > > > > > stdout/stderr/command/dmesg output available from CI and time > > > > > is > > > > > given > > > > > as 0:00:00. Doesn't seem like it could be related to this > > > > > patch. > > > > > > > > > > > > > > > > Test pm_rpm: > > > > > > Subgroup basic-pci-d3-state: > > > > > > dmesg-warn -> PASS (snb-dellxps) > > > > > > Subgroup basic-rte: > > > > > > pass -> DMESG-WARN (bsw-nuc-2) > > > > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=94164 > > > > > > > > > > > > > > > Matt > > > > > > > > Pushed to -dinq, thanks for the patch and the review. > > > > > > > > I had to rebase it on top of Ville's recent > > > > s/crtc_state/old_crtc_state/ change, please double check it. > > > > > > > > Jani, this is for -fixes. > > > > > > Surely you added either > > > > > > Cc: drm-intel-fixes@lists.freedesktop.org > > > > > > or > > > > > > Cc: stable@vger.kernel.org > > > > > > in the commit when you pushed, then? > > > > No, I haven't will do that in the future. Btw, what's the rule for > > deciding that something is for -fixes or stable only after it's > > been > > pushed? Just ping you in case for -fixes and resend it in case of > > stable? > > > > --Imre > > Are you sure we need this for -fixes? The patch that introduced the > regression isn't in drm-next/4.6 as far as I know. The regressing commit ed4a6a7ca853 is in drm-intel-next, so I think we do need it for -fixes. --Imre
On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > [ text/plain ] > On ti, 2016-03-22 at 08:56 -0700, Matt Roper wrote: >> On Tue, Mar 22, 2016 at 04:17:58PM +0200, Imre Deak wrote: >> > On ti, 2016-03-22 at 15:51 +0200, Jani Nikula wrote: >> > > On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: >> > > > [ text/plain ] >> > > > On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: >> > > > > On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: >> > > > > > == Series Details == >> > > > > > >> > > > > > Series: drm/i915: Wait until after wm optimization to drop >> > > > > > runtime >> > > > > > PM reference >> > > > > > URL : https://patchwork.freedesktop.org/series/4135/ >> > > > > > State : failure >> > > > > > >> > > > > > == Summary == >> > > > > > >> > > > > > Series 4135v1 drm/i915: Wait until after wm optimization to >> > > > > > drop >> > > > > > runtime PM reference >> > > > > > http://patchwork.freedesktop.org/api/1.0/series/4135/revisi >> > > > > > ons/ >> > > > > > 1/mb >> > > > > > ox/ >> > > > > > >> > > > > > Test kms_flip: >> > > > > > Subgroup basic-flip-vs-wf_vblank: >> > > > > > fail -> PASS (bsw-nuc-2) >> > > > > > Subgroup basic-plain-flip: >> > > > > > dmesg-warn -> PASS (hsw-gt2) >> > > > > > Test kms_force_connector_basic: >> > > > > > Subgroup force-load-detect: >> > > > > > pass -> SKIP (ivb-t430s) >> > > > > >> > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93769 >> > > > > >> > > > > > Test kms_pipe_crc_basic: >> > > > > > Subgroup read-crc-pipe-c: >> > > > > > dmesg-warn -> PASS (hsw-gt2) >> > > > > > Subgroup suspend-read-crc-pipe-a: >> > > > > > dmesg-warn -> PASS (skl-i5k-2) >> > > > > > skip -> PASS (hsw-brixbox) >> > > > > > Subgroup suspend-read-crc-pipe-c: >> > > > > > pass -> DMESG-WARN (bsw-nuc-2) >> > > > > >> > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93294 >> > > > > >> > > > > > pass -> INCOMPLETE (hsw-gt2) >> > > > > >> > > > > Seems like the machine just died completely? No >> > > > > stdout/stderr/command/dmesg output available from CI and time >> > > > > is >> > > > > given >> > > > > as 0:00:00. Doesn't seem like it could be related to this >> > > > > patch. >> > > > > >> > > > > >> > > > > > Test pm_rpm: >> > > > > > Subgroup basic-pci-d3-state: >> > > > > > dmesg-warn -> PASS (snb-dellxps) >> > > > > > Subgroup basic-rte: >> > > > > > pass -> DMESG-WARN (bsw-nuc-2) >> > > > > >> > > > > https://bugs.freedesktop.org/show_bug.cgi?id=94164 >> > > > > >> > > > > >> > > > > Matt >> > > > >> > > > Pushed to -dinq, thanks for the patch and the review. >> > > > >> > > > I had to rebase it on top of Ville's recent >> > > > s/crtc_state/old_crtc_state/ change, please double check it. >> > > > >> > > > Jani, this is for -fixes. >> > > >> > > Surely you added either >> > > >> > > Cc: drm-intel-fixes@lists.freedesktop.org >> > > >> > > or >> > > >> > > Cc: stable@vger.kernel.org >> > > >> > > in the commit when you pushed, then? >> > >> > No, I haven't will do that in the future. Btw, what's the rule for >> > deciding that something is for -fixes or stable only after it's >> > been >> > pushed? Just ping you in case for -fixes and resend it in case of >> > stable? >> > >> > --Imre >> >> Are you sure we need this for -fixes? The patch that introduced the >> regression isn't in drm-next/4.6 as far as I know. > > The regressing commit ed4a6a7ca853 is in drm-intel-next, so I think we > do need it for -fixes. Ah. It's not in 4.5 and it's not on its way to 4.6. Not fixes material. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
On Tue, Mar 22, 2016 at 06:31:53PM +0200, Jani Nikula wrote: > On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > > [ text/plain ] > > On ti, 2016-03-22 at 08:56 -0700, Matt Roper wrote: > >> On Tue, Mar 22, 2016 at 04:17:58PM +0200, Imre Deak wrote: > >> > On ti, 2016-03-22 at 15:51 +0200, Jani Nikula wrote: > >> > > On Tue, 22 Mar 2016, Imre Deak <imre.deak@intel.com> wrote: > >> > > > [ text/plain ] > >> > > > On ti, 2016-03-08 at 09:26 -0800, Matt Roper wrote: > >> > > > > On Mon, Mar 07, 2016 at 11:47:51AM +0000, Patchwork wrote: > >> > > > > > == Series Details == > >> > > > > > > >> > > > > > Series: drm/i915: Wait until after wm optimization to drop > >> > > > > > runtime > >> > > > > > PM reference > >> > > > > > URL : https://patchwork.freedesktop.org/series/4135/ > >> > > > > > State : failure > >> > > > > > > >> > > > > > == Summary == > >> > > > > > > >> > > > > > Series 4135v1 drm/i915: Wait until after wm optimization to > >> > > > > > drop > >> > > > > > runtime PM reference > >> > > > > > http://patchwork.freedesktop.org/api/1.0/series/4135/revisi > >> > > > > > ons/ > >> > > > > > 1/mb > >> > > > > > ox/ > >> > > > > > > >> > > > > > Test kms_flip: > >> > > > > > Subgroup basic-flip-vs-wf_vblank: > >> > > > > > fail -> PASS (bsw-nuc-2) > >> > > > > > Subgroup basic-plain-flip: > >> > > > > > dmesg-warn -> PASS (hsw-gt2) > >> > > > > > Test kms_force_connector_basic: > >> > > > > > Subgroup force-load-detect: > >> > > > > > pass -> SKIP (ivb-t430s) > >> > > > > > >> > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93769 > >> > > > > > >> > > > > > Test kms_pipe_crc_basic: > >> > > > > > Subgroup read-crc-pipe-c: > >> > > > > > dmesg-warn -> PASS (hsw-gt2) > >> > > > > > Subgroup suspend-read-crc-pipe-a: > >> > > > > > dmesg-warn -> PASS (skl-i5k-2) > >> > > > > > skip -> PASS (hsw-brixbox) > >> > > > > > Subgroup suspend-read-crc-pipe-c: > >> > > > > > pass -> DMESG-WARN (bsw-nuc-2) > >> > > > > > >> > > > > https://bugs.freedesktop.org/show_bug.cgi?id=93294 > >> > > > > > >> > > > > > pass -> INCOMPLETE (hsw-gt2) > >> > > > > > >> > > > > Seems like the machine just died completely? No > >> > > > > stdout/stderr/command/dmesg output available from CI and time > >> > > > > is > >> > > > > given > >> > > > > as 0:00:00. Doesn't seem like it could be related to this > >> > > > > patch. > >> > > > > > >> > > > > > >> > > > > > Test pm_rpm: > >> > > > > > Subgroup basic-pci-d3-state: > >> > > > > > dmesg-warn -> PASS (snb-dellxps) > >> > > > > > Subgroup basic-rte: > >> > > > > > pass -> DMESG-WARN (bsw-nuc-2) > >> > > > > > >> > > > > https://bugs.freedesktop.org/show_bug.cgi?id=94164 > >> > > > > > >> > > > > > >> > > > > Matt > >> > > > > >> > > > Pushed to -dinq, thanks for the patch and the review. > >> > > > > >> > > > I had to rebase it on top of Ville's recent > >> > > > s/crtc_state/old_crtc_state/ change, please double check it. > >> > > > > >> > > > Jani, this is for -fixes. > >> > > > >> > > Surely you added either > >> > > > >> > > Cc: drm-intel-fixes@lists.freedesktop.org > >> > > > >> > > or > >> > > > >> > > Cc: stable@vger.kernel.org > >> > > > >> > > in the commit when you pushed, then? > >> > > >> > No, I haven't will do that in the future. Btw, what's the rule for > >> > deciding that something is for -fixes or stable only after it's > >> > been > >> > pushed? Just ping you in case for -fixes and resend it in case of > >> > stable? > >> > > >> > --Imre > >> > >> Are you sure we need this for -fixes? The patch that introduced the > >> regression isn't in drm-next/4.6 as far as I know. > > > > The regressing commit ed4a6a7ca853 is in drm-intel-next, so I think we > > do need it for -fixes. > > Ah. It's not in 4.5 and it's not on its way to 4.6. Not fixes material. For the future: $ dim tag-contains $commit_that_youre_fixing Will tell you either the kernel version of if it hasn't landed, the branches. And if it's in airlied/drm-next but dinq is already past the merge window then it needs Cc: stable. If it's only in dinq then not. I guess we could improve the heuristics of the script a bit and directly print out the Cc: stable or Cc: drm-intel-fixes line you would need, if any ... Volunteers highly welcome ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 62d36a7..0af08d7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct drm_device *dev, if (!state->legacy_cursor_update) intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); - for_each_crtc_in_state(state, crtc, crtc_state, i) { - intel_post_plane_update(to_intel_crtc(crtc)); - - if (put_domains[i]) - modeset_put_power_domains(dev_priv, put_domains[i]); - } - - if (intel_state->modeset) - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); - /* * Now that the vblank has passed, we can go ahead and program the * optimal watermarks on platforms that need two-step watermark @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct drm_device *dev, dev_priv->display.optimize_watermarks(intel_cstate); } + for_each_crtc_in_state(state, crtc, crtc_state, i) { + intel_post_plane_update(to_intel_crtc(crtc)); + + if (put_domains[i]) + modeset_put_power_domains(dev_priv, put_domains[i]); + } + + if (intel_state->modeset) + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); + mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state); mutex_unlock(&dev->struct_mutex);
At the end of an atomic commit, we currently wait for vblanks to complete, call put() on the various runtime PM references, and then try to optimize our watermarks (on platforms that need two-step watermark programming). This can lead to watermark registers being programmed while the power well is powered down. We need to wait until after watermark optimization is complete before dropping our runtime power references. Note that in the future the watermark optimization is probably going to move to an asynchronous workqueue task that happens at some arbitrary point after vblank. When we make that change, we'll no longer necessarily be operating under the power reference held here, so we'll need to wrap the watermark register programmin in a call to intel_runtime_pm_get_if_in_use() or similar. Cc: arun.siluvery@linux.intel.com Cc: ville.syrjala@linux.intel.com Cc: maarten.lankhorst@linux.intel.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)") Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)