| Message ID | 1418667113-11706-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>: > During plane operations, we read/write some registers that only operate > properly if we're not runtime suspended. At the moment we're not > holding the runtime PM reference across the whole plane operation, so > there's a potential for problems. > > This issue was already partially addressed by commit > > commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > Date: Fri Aug 15 15:59:32 2014 -0300 > > drm/i915: fix plane/cursor handling when runtime suspended > > which took care of holding the runtime PM reference during the pin and > fence operations for plane updates. However there are still a few > actual plane registers that we also need to hold the runtime PM > reference for. Recent refactoring patches in preparation for atomic > have rearranged the code and made it increasingly likely that the > hardware will have time to suspend between the pin/fence operation and > the actual register writes. > > The solution here grabs the runtime PM reference around the 'commit' > operation for planes, which should cover all the relevant register > reads/writes. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> I see we're in the middle of a very big rework on how all these prepare/commit functions are called, so I don't think it makes sense to spend too much time trying to find the very-best-perfect spot for the get/put calls, since they're likely to be changed later. So I guess that for now it's important to fix the current "regression" reported by QA: Testcase: igt/pm-rpm/legacy-planes Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3044af5..a0ddce5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11863,6 +11863,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > uint32_t src_w, uint32_t src_h) > { > struct drm_device *dev = plane->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_framebuffer *old_fb = plane->fb; > struct intel_plane_state state; > struct intel_plane *intel_plane = to_intel_plane(plane); > @@ -11902,7 +11903,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > return ret; > } > > + intel_runtime_pm_get(dev_priv); > intel_plane->commit_plane(plane, &state); > + intel_runtime_pm_put(dev_priv); > > if (fb != old_fb && old_fb) { > if (intel_crtc->active) > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK +5 360/366 365/366
SNB 448/450 448/450
IVB 497/498 497/498
BYT 289/289 289/289
HSW 563/564 563/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
ILK igt_drv_suspend_fence-restore-untiled DMESG_WARN(1, M26)PASS(5, M37M26) PASS(1, M37)
ILK igt_kms_flip_bcs-flip-vs-modeset-interruptible DMESG_WARN(1, M26)PASS(5, M37M26) PASS(1, M37)
ILK igt_kms_flip_busy-flip-interruptible DMESG_WARN(1, M26)PASS(5, M37M26) PASS(1, M37)
ILK igt_kms_flip_flip-vs-rmfb-interruptible DMESG_WARN(1, M26)PASS(5, M37M26) PASS(1, M37)
ILK igt_kms_flip_rcs-flip-vs-dpms DMESG_WARN(1, M26)PASS(4, M37M26) PASS(1, M37)
Note: You need to pay more attention to line start with '*'
On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote: > 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>: > > During plane operations, we read/write some registers that only operate > > properly if we're not runtime suspended. At the moment we're not > > holding the runtime PM reference across the whole plane operation, so > > there's a potential for problems. > > > > This issue was already partially addressed by commit > > > > commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Date: Fri Aug 15 15:59:32 2014 -0300 > > > > drm/i915: fix plane/cursor handling when runtime suspended > > > > which took care of holding the runtime PM reference during the pin and > > fence operations for plane updates. However there are still a few > > actual plane registers that we also need to hold the runtime PM > > reference for. Recent refactoring patches in preparation for atomic > > have rearranged the code and made it increasingly likely that the > > hardware will have time to suspend between the pin/fence operation and > > the actual register writes. Which kind of registers? If this is just in the system agent then a rpm ref is enough, but if this is also about plane registers then we'd need a reference of the plane power domain. Which would indicate some failure to check for crtc->active somewhere I think. > > The solution here grabs the runtime PM reference around the 'commit' > > operation for planes, which should cover all the relevant register > > reads/writes. > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > I see we're in the middle of a very big rework on how all these > prepare/commit functions are called, so I don't think it makes sense > to spend too much time trying to find the very-best-perfect spot for > the get/put calls, since they're likely to be changed later. So I > guess that for now it's important to fix the current "regression" > reported by QA: > > Testcase: igt/pm-rpm/legacy-planes > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq? -Daniel
On Tue, 16 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote: >> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>: >> > During plane operations, we read/write some registers that only operate >> > properly if we're not runtime suspended. At the moment we're not >> > holding the runtime PM reference across the whole plane operation, so >> > there's a potential for problems. >> > >> > This issue was already partially addressed by commit >> > >> > commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 >> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > Date: Fri Aug 15 15:59:32 2014 -0300 >> > >> > drm/i915: fix plane/cursor handling when runtime suspended >> > >> > which took care of holding the runtime PM reference during the pin and >> > fence operations for plane updates. However there are still a few >> > actual plane registers that we also need to hold the runtime PM >> > reference for. Recent refactoring patches in preparation for atomic >> > have rearranged the code and made it increasingly likely that the >> > hardware will have time to suspend between the pin/fence operation and >> > the actual register writes. > > Which kind of registers? If this is just in the system agent then a rpm > ref is enough, but if this is also about plane registers then we'd need a > reference of the plane power domain. Which would indicate some failure to > check for crtc->active somewhere I think. > >> > The solution here grabs the runtime PM reference around the 'commit' >> > operation for planes, which should cover all the relevant register >> > reads/writes. >> > >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> >> I see we're in the middle of a very big rework on how all these >> prepare/commit functions are called, so I don't think it makes sense >> to spend too much time trying to find the very-best-perfect spot for >> the get/put calls, since they're likely to be changed later. So I >> guess that for now it's important to fix the current "regression" >> reported by QA: >> >> Testcase: igt/pm-rpm/legacy-planes >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq? At least the partial fix referenced is in 3.17. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 16 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 16 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote: >>> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>: >>> > During plane operations, we read/write some registers that only operate >>> > properly if we're not runtime suspended. At the moment we're not >>> > holding the runtime PM reference across the whole plane operation, so >>> > there's a potential for problems. >>> > >>> > This issue was already partially addressed by commit >>> > >>> > commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 >>> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> > Date: Fri Aug 15 15:59:32 2014 -0300 >>> > >>> > drm/i915: fix plane/cursor handling when runtime suspended >>> > >>> > which took care of holding the runtime PM reference during the pin and >>> > fence operations for plane updates. However there are still a few >>> > actual plane registers that we also need to hold the runtime PM >>> > reference for. Recent refactoring patches in preparation for atomic >>> > have rearranged the code and made it increasingly likely that the >>> > hardware will have time to suspend between the pin/fence operation and >>> > the actual register writes. >> >> Which kind of registers? If this is just in the system agent then a rpm >> ref is enough, but if this is also about plane registers then we'd need a >> reference of the plane power domain. Which would indicate some failure to >> check for crtc->active somewhere I think. >> >>> > The solution here grabs the runtime PM reference around the 'commit' >>> > operation for planes, which should cover all the relevant register >>> > reads/writes. >>> > >>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 >>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >>> >>> I see we're in the middle of a very big rework on how all these >>> prepare/commit functions are called, so I don't think it makes sense >>> to spend too much time trying to find the very-best-perfect spot for >>> the get/put calls, since they're likely to be changed later. So I >>> guess that for now it's important to fix the current "regression" >>> reported by QA: >>> >>> Testcase: igt/pm-rpm/legacy-planes >>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq? > > At least the partial fix referenced is in 3.17. Ugh, but it conflicts badly, there's no ->commit_plane in drm-intel-next-fixes. Daniel, pick this up for dinq, and if someone wants this to stable, it needs a backported version. BR, Jani. > >> -Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
2014-12-16 7:42 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote: >> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>: >> > During plane operations, we read/write some registers that only operate >> > properly if we're not runtime suspended. At the moment we're not >> > holding the runtime PM reference across the whole plane operation, so >> > there's a potential for problems. >> > >> > This issue was already partially addressed by commit >> > >> > commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 >> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > Date: Fri Aug 15 15:59:32 2014 -0300 >> > >> > drm/i915: fix plane/cursor handling when runtime suspended >> > >> > which took care of holding the runtime PM reference during the pin and >> > fence operations for plane updates. However there are still a few >> > actual plane registers that we also need to hold the runtime PM >> > reference for. Recent refactoring patches in preparation for atomic >> > have rearranged the code and made it increasingly likely that the >> > hardware will have time to suspend between the pin/fence operation and >> > the actual register writes. > > Which kind of registers? Some of the regs touched by ivb_get_colorkey(). > If this is just in the system agent then a rpm > ref is enough, but if this is also about plane registers then we'd need a > reference of the plane power domain. Which would indicate some failure to > check for crtc->active somewhere I think. > >> > The solution here grabs the runtime PM reference around the 'commit' >> > operation for planes, which should cover all the relevant register >> > reads/writes. >> > >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> >> I see we're in the middle of a very big rework on how all these >> prepare/commit functions are called, so I don't think it makes sense >> to spend too much time trying to find the very-best-perfect spot for >> the get/put calls, since they're likely to be changed later. So I >> guess that for now it's important to fix the current "regression" >> reported by QA: >> >> Testcase: igt/pm-rpm/legacy-planes >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq? I would suggest only dinq, since it's the only thing that can reproduce the WARN, which got visible after: commit 6beb8c23ebcc3d3287d8a247d11b73d7d0eaa475 drm/i915: Consolidate plane 'prepare' functions (v2) > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Dec 16, 2014 at 03:29:56PM +0200, Jani Nikula wrote: > On Tue, 16 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Tue, 16 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote: > >>> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>: > >>> > During plane operations, we read/write some registers that only operate > >>> > properly if we're not runtime suspended. At the moment we're not > >>> > holding the runtime PM reference across the whole plane operation, so > >>> > there's a potential for problems. > >>> > > >>> > This issue was already partially addressed by commit > >>> > > >>> > commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 > >>> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>> > Date: Fri Aug 15 15:59:32 2014 -0300 > >>> > > >>> > drm/i915: fix plane/cursor handling when runtime suspended > >>> > > >>> > which took care of holding the runtime PM reference during the pin and > >>> > fence operations for plane updates. However there are still a few > >>> > actual plane registers that we also need to hold the runtime PM > >>> > reference for. Recent refactoring patches in preparation for atomic > >>> > have rearranged the code and made it increasingly likely that the > >>> > hardware will have time to suspend between the pin/fence operation and > >>> > the actual register writes. > >> > >> Which kind of registers? If this is just in the system agent then a rpm > >> ref is enough, but if this is also about plane registers then we'd need a > >> reference of the plane power domain. Which would indicate some failure to > >> check for crtc->active somewhere I think. > >> > >>> > The solution here grabs the runtime PM reference around the 'commit' > >>> > operation for planes, which should cover all the relevant register > >>> > reads/writes. > >>> > > >>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 > >>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >>> > >>> I see we're in the middle of a very big rework on how all these > >>> prepare/commit functions are called, so I don't think it makes sense > >>> to spend too much time trying to find the very-best-perfect spot for > >>> the get/put calls, since they're likely to be changed later. So I > >>> guess that for now it's important to fix the current "regression" > >>> reported by QA: > >>> > >>> Testcase: igt/pm-rpm/legacy-planes > >>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq? > > > > At least the partial fix referenced is in 3.17. > > Ugh, but it conflicts badly, there's no ->commit_plane in > drm-intel-next-fixes. > > Daniel, pick this up for dinq, and if someone wants this to stable, it > needs a backported version. Done, and thanks to Paulo for the additional clarification. I've augmented the commit message with that. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3044af5..a0ddce5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11863,6 +11863,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, uint32_t src_w, uint32_t src_h) { struct drm_device *dev = plane->dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct drm_framebuffer *old_fb = plane->fb; struct intel_plane_state state; struct intel_plane *intel_plane = to_intel_plane(plane); @@ -11902,7 +11903,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, return ret; } + intel_runtime_pm_get(dev_priv); intel_plane->commit_plane(plane, &state); + intel_runtime_pm_put(dev_priv); if (fb != old_fb && old_fb) { if (intel_crtc->active)
During plane operations, we read/write some registers that only operate properly if we're not runtime suspended. At the moment we're not holding the runtime PM reference across the whole plane operation, so there's a potential for problems. This issue was already partially addressed by commit commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956 Author: Paulo Zanoni <paulo.r.zanoni@intel.com> Date: Fri Aug 15 15:59:32 2014 -0300 drm/i915: fix plane/cursor handling when runtime suspended which took care of holding the runtime PM reference during the pin and fence operations for plane updates. However there are still a few actual plane registers that we also need to hold the runtime PM reference for. Recent refactoring patches in preparation for atomic have rearranged the code and made it increasingly likely that the hardware will have time to suspend between the pin/fence operation and the actual register writes. The solution here grabs the runtime PM reference around the 'commit' operation for planes, which should cover all the relevant register reads/writes. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 +++ 1 file changed, 3 insertions(+)