On Fri, Feb 28, 2014 at 05:11:46PM -0300, Paulo Zanoni wrote:
> 2014-02-28 10:50 GMT-03:00 Imre Deak <imre.deak@xxxxxxxxx>:
> > On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >>
> >> Since the addition of dev_priv->mm.busy, there's no more need for
> >> dev_priv->pc8.gpu_idle, so kill it.
> >>
> >> Notice that when you remove gpu_idle, hsw_package_c8_gpu_idle and
> >> hsw_package_c8_gpu_busy become identical to hsw_enable_package_c8 and
> >> hsw_disable_package_c8, so just use them.
> >>
> >> Also, when we boot the machine, dev_priv->mm.busy initially considers
> >> the machine as idle. This is opposed to dev_priv->pc8.gpu_idle, which
> >> considered it busy. So dev_priv->pc8.disable_count has to be
> >> initalized to 1 now.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> >> drivers/gpu/drm/i915/i915_drv.h | 10 ++++------
> >> drivers/gpu/drm/i915/intel_display.c | 30 ++----------------------------
> >> drivers/gpu/drm/i915/intel_pm.c | 3 +--
> >> 4 files changed, 8 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index 34e347f..62d0c0915 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -2014,7 +2014,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
> >> mutex_lock(&dev_priv->pc8.lock);
> >> seq_printf(m, "Requirements met: %s\n",
> >> yesno(dev_priv->pc8.requirements_met));
> >> - seq_printf(m, "GPU idle: %s\n", yesno(dev_priv->pc8.gpu_idle));
> >> + seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
> >> seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
> >> seq_printf(m, "IRQs disabled: %s\n",
> >> yesno(dev_priv->pc8.irqs_disabled));
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index a5caa7e..2a2a3a9 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1321,11 +1321,10 @@ struct ilk_wm_values {
> >> * Ideally every piece of our code that needs PC8+ disabled would call
> >> * hsw_disable_package_c8, which would increment disable_count and prevent the
> >> * system from reaching PC8+. But we don't have a symmetric way to do this for
> >> - * everything, so we have the requirements_met and gpu_idle variables. When we
> >> - * switch requirements_met or gpu_idle to true we decrease disable_count, and
> >> - * increase it in the opposite case. The requirements_met variable is true when
> >> - * all the CRTCs, encoders and the power well are disabled. The gpu_idle
> >> - * variable is true when the GPU is idle.
> >> + * everything, so we have the requirements_met variable. When we switch
> >> + * requirements_met to true we decrease disable_count, and increase it in the
> >> + * opposite case. The requirements_met variable is true when all the CRTCs,
> >> + * encoders and the power well are disabled.
> >> *
> >> * In addition to everything, we only actually enable PC8+ if disable_count
> >> * stays at zero for at least some seconds. This is implemented with the
> >> @@ -1348,7 +1347,6 @@ struct ilk_wm_values {
> >> */
> >> struct i915_package_c8 {
> >> bool requirements_met;
> >> - bool gpu_idle;
> >> bool irqs_disabled;
> >> /* Only true after the delayed work task actually enables it. */
> >> bool enabled;
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index c64fb7f..796a116 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6812,32 +6812,6 @@ done:
> >> mutex_unlock(&dev_priv->pc8.lock);
> >> }
> >>
> >> -static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
> >> -{
> >> - if (!HAS_PC8(dev_priv->dev))
> >> - return;
> >> -
> >> - mutex_lock(&dev_priv->pc8.lock);
> >> - if (!dev_priv->pc8.gpu_idle) {
> >> - dev_priv->pc8.gpu_idle = true;
> >> - __hsw_enable_package_c8(dev_priv);
> >> - }
> >> - mutex_unlock(&dev_priv->pc8.lock);
> >> -}
> >> -
> >> -static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
> >> -{
> >> - if (!HAS_PC8(dev_priv->dev))
> >> - return;
> >> -
> >> - mutex_lock(&dev_priv->pc8.lock);
> >> - if (dev_priv->pc8.gpu_idle) {
> >> - dev_priv->pc8.gpu_idle = false;
> >> - __hsw_disable_package_c8(dev_priv);
> >> - }
> >> - mutex_unlock(&dev_priv->pc8.lock);
> >> -}
> >> -
> >> #define for_each_power_domain(domain, mask) \
> >> for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \
> >> if ((1 << (domain)) & (mask))
> >> @@ -8195,7 +8169,7 @@ void intel_mark_busy(struct drm_device *dev)
> >> if (dev_priv->mm.busy)
> >> return;
> >>
> >> - hsw_package_c8_gpu_busy(dev_priv);
> >> + hsw_disable_package_c8(dev_priv);
> >> i915_update_gfx_val(dev_priv);
> >> dev_priv->mm.busy = true;
> >> }
> >> @@ -8224,7 +8198,7 @@ void intel_mark_idle(struct drm_device *dev)
> >> gen6_rps_idle(dev->dev_private);
> >>
> >> out:
> >> - hsw_package_c8_gpu_idle(dev_priv);
> >> + hsw_enable_package_c8(dev_priv);
> >> }
> >>
> >> void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index a6b877a..50b80bb 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -5786,10 +5786,9 @@ void intel_pm_setup(struct drm_device *dev)
> >>
> >> mutex_init(&dev_priv->pc8.lock);
> >> dev_priv->pc8.requirements_met = false;
> >> - dev_priv->pc8.gpu_idle = false;
> >> dev_priv->pc8.irqs_disabled = false;
> >> dev_priv->pc8.enabled = false;
> >> - dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
> >> + dev_priv->pc8.disable_count = 1; /* requirements_met */
> >
> > This looks ok, but it's part of "Merge PC8 with runtime PM, v2" along
> > with patch 1/11, so they can be skipped from this patchset.
>
> Yes. Since you spotted some potential conflicts between this series
> and yours, I decided to send "Merge PC8 with runtime PM v2" in a way
> that it's independent form this series: it just contains the fixes
> that are necessary, avoiding some of the conflicts you detected. So we
> can just merge that series instead of this, if wanted.
I'm voting for potential conflicts be damned - this entire runtime pm
enabling will keep on being ugly for a while, so I'm just trying to pull
in as much as possible ...
-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
