2014-12-16 7:42 GMT-02:00 Daniel Vetter <daniel@xxxxxxxx>: > 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@xxxxxxxxx>: >> > 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@xxxxxxxxx> >> > 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@xxxxxxxxx> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180 >> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> >> >> 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@xxxxxxxxx> > > Also Cc: stable@xxxxxxxxxxxxxxx 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 -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
