2014-03-05 14:09 GMT-03:00 Daniel Vetter <daniel@xxxxxxxx>: > On Thu, Feb 27, 2014 at 07:26:27PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
