Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx