On Tue, Jul 29, 2014 at 1:25 AM, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
> On Mon, Jul 28, 2014 at 03:37:11PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> Hi
>>
>> This series fixes some bugs that happen when we're runtime suspended and try to
>> use the planes APIs. I also wrote IGT test cases for the bugs, so we will be
>> able to detect future regressions.
>>
>> The controversial part of these patches is that we had previously defined that
>> we wanted to get/put runtime PM in the highest level of the stack, wrapping as
>> much code as possible, but Daniel asked me to only get/put runtime PM around the
>> functions that pin the objects (still on the highest level, but only around the
>> pin functions). This series implements Daniel's suggestions.
>
> These look pretty straightforward to me, so for all three:
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> However as a side note on the runtime PM stuff I'll admit that it's not
> an area I'd previously paid too much attention to and my first reaction
> looking at the patches was "I wonder if intel_runtime_pm_{get,put} could
> be pushed down into intel_pin_and_fence_fb_obj()." Until I read your
> cover letter I wasn't aware of the goal of "get/put runtime PM in the
> highest level of the stack, wrapping as much code as possible" and I
> don't think that's really explained anywhere in the code or in the
> commit log today. Maybe we could add a comment in intel_pm.c explaining
> that design goal for future contributors/reviewers?
Originally we had runtime pm calls way down, which in a few cases
meant that we'd dropped the runtime pm reference while we still
depended upon the register values surviving. Of course the usual timer
helps there. Hence why I've put out the "push it to the highest level
possible" guideline, to make sure that e.g. for a modeset sequence we
grab the runtime pm ref once, before we start touching any hardware.
Pinning stuff to the gtt (which is the only thing tricky here, after
the early bail-out for !crtc->active) is a bit different. Those
registers are magic aliases for memory regions, so even when we go
into runtime pm the register value will survive (since the ptes are
actually in main memory). We only need to wake up the gpu to make sure
they actually get there. And the "pin plane while display is off" case
is really special, all the pte writing we do in the gem code is done
as part of a userspace operation where we really need to have the gpu
on (and so where it makes sense to grab the runtime pm ref at the
top-level to make sure our setup doesn't disappear unexpectedly).
I hope this explains why these 3 cases are special.
-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
