[Intel-gfx] [PATCH 00/23] Merge PC8 with runtime PM, v2

[Daniel Vetter]

On Thu, Mar 6, 2014 at 11:23 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> 2014-03-05 14:09 GMT-03:00 Daniel Vetter <daniel at ffwll.ch>:
>> On Thu, Feb 27, 2014 at 07:26:27PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>
>>> Hi
>>>
>>> This is the second time I send this series to the mailing list. Please read the
>>> first cover letter:
>>>    http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html
>>>
>>> What's new?
>>>
>>> Two main differences:
>>>   - Added a patch from Chris, which resulted in a change on how we handle
>>>     dev_priv->pc8.gpu_busy later.
>>>   - Fixed a bug on the forcewake handling.
>>>
>>> There is some discussion if we want to merge this first, or the VLV power wells
>>> patches first. Independently of the decision, I think we should try to at least
>>> discuss these patches and review what can be reviewed. I really think this
>>> series should make it easier to add runtime PM support to other platforms, and I
>>> even added BDW and SNB support on top of these patches.
>>
>> Looks good. Only too imo minor things:
>> - Drop the runtime pm get/put from the forcewake get/put functions and
>>   adjust callers accordingly to grab the runtime pm ref themselves outside
>>   of the register access. That should fully address Chris'&mine concern in
>>   that regard. I only expect a minor impact here, if that turns out wrong
>>   please complain and we need to take another look.
>
> I am confused, because you're asking me this, but you merged
> "drm/i915: put runtime PM only when we actually release force_wake"
> without commenting anything. Maybe it's not clear to me what you
> really want. Let me explain the problem so you can give your opinion.
>
> First of all, the thing is that we're getting/putting runtime PM only
> at gen6_gt_force_wake_get/put, and that is *not* called when we're
> reading/writing generic registers, it's only called explicitly at
> certain points of our code (mostly intel_pm.c, and a few other
> places). So keep in mind that we're not getting/putting any runtime PM
> references at I915_READ/WRITE, and we *do* have a check to see if we
> do any reads/writes while we're runtime suspended.
>
> I can't do what you're requesting because of the delayed forcewake
> put. Imagine the following code:
>
> 1. intel_runtime_pm_get()
> 2. gen6_gt_force_wake_get()
> 3. do_stuff()
> 4. gen6_gt_force_wake_put()
> 5. intel_runtime_pm_put()
>
> The problem is that in step 4 we won't actually be putting forcewake,
> we will just be launching the dealyed function that will, eventually,
> disable forcewake. So at step 5 we will put runtime PM, allowing the
> device to be suspended while it is still in "forcewake" mode. With
> this, we may runtime suspend the driver, then later
> gen6_gt_force_wake_work() will be called, which will trigger
> __gen6_gt_force_wake_put() - notice the underlines on the name - which
> will try to read/write registers while the device is runtime
> suspended. And this is not what we want.
>
> So, IMHO, it doesn't make sense to runtime suspend the driver while
> we're in forcewake mode. And that's why we can't make the callers of
> gen6_gt_force_wake_get() get their own runtime PM refcounts. It also
> doesn't make sense to drop the forcewake refcount and just cancel the
> forcewake task when we runtime suspend, because if we do this we'll
> just be unsolving the exact problem that the delayed forcewake queue
> solves.
>
> So, considering the problem above, what exactly do you need and why?

I agree that I didn't understand the issue at hand at all, and that
your approach is sound. That leaves us with Chris' concern, I'll reply
directly to Chris' mail to address that.

My apologies for the massive confusion I've caused here.

>> - The aux display power domain patch. Needs acks from platform owners imo,
>>   I'd just keep it for now.
>
> I can write a new patch that doesn't kill
> intel_aux_display_runtime_get. The implementation will just contain a
> call to intel_runtime_pm_get... Then you can merge the color you
> prefer :)

Yeah, I think for now I prefer to keep the aux_display_runtime_get/put
color, at least as long as the other platform owners don't wake up ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch