On Tue, Dec 16, 2014 at 03:29:56PM +0200, Jani Nikula wrote: > On Tue, 16 Dec 2014, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > > On Tue, 16 Dec 2014, Daniel Vetter <daniel@xxxxxxxx> 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@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? 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? > > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
