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

[Paulo Zanoni]

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?

>
> - 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 :)


>
> Also I've merged quite a few patches already, hopefully the most recent
> versions. When rebasing pls collect all the r-b tags for speedy merging.
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni