[Intel-gfx] [PATCH] drm/i915: Hold runtime PM during plane commit

[Paulo Zanoni]

2014-12-16 7:42 GMT-02:00 Daniel Vetter <daniel at 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 at 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 at 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 at intel.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
>> > Signed-off-by: Matt Roper <matthew.d.roper at 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 at intel.com>
>
> Also Cc: stable at 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



-- 
Paulo Zanoni