[Intel-gfx] [PATCH 08/19] drm/i915: add some runtime PM get/put calls
Paulo Zanoni
przanoni at gmail.com
Fri Nov 29 14:56:21 CET 2013
2013/11/29 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Nov 29, 2013 at 11:05:49AM -0200, Rodrigo Vivi wrote:
>> I think this one and last one could be only one patch, but anyway:
>
> Yeah, agreed that the split doesn't make too much sense. What I'd really
> like to see in these commits (and also all the following ones that
> sprinkle runtime pm get/put calls all over the place) is a reference to
> the relevant subtest. And with that a natural way to split stuff would be
> so that each patch adds the get/put calls for a given feature and hence
> testcase.
That's what I did. I guess I just failed at properly communicating it.
Patch 7 is the very basic thing. If you don't have it, as soon as you
enter D3 you'll hit dozens of WARNs and errors. I guess we can say it
fixes the "rte" subtest of pm_pc8.
Patch 8 is just for the sysfs and debugfs stuff. It fixes the
"debugfs-read", "debugfs-forcewake-user" and "sysfs-read" subtests.
I don't think merging both patches in a single one is good because of
rebasing hell. Our code changes way too much for me to try to keep
giant patches around. If needed, I could try to split patch 8 into
smaller ones.
>
> I don't care too much about the split since it's all disabled by default,
> so as-is is imo ok. But the tests should be mentioned. That applies in
> general btw: If there's a specific igt testcase for a feature or bugfix,
> the kernel commit message should mention the testname.
I agree with this, but the problem is that while you're doing the very
early enabling work of a feature you tend to do so many changes that
you lose track of everything. Also, you change the test suite too. For
patches after the commit that really enables the feature, listing the
test case name is a must-have, and I hope I did this :)
>
> If people want, we could use a standardized patter like
>
> IGT-Testcase: igt/pm_pc8/gem_mmap_check
I'd love to have something like that! Any tag format works for me, as
long as we use it consistently.
Thanks for the reviews!
Paulo
>
> or something like that.
>
> Cheers, Daniel
>>
>> Reviewed-by Rodrigo Vivi <rodrigo.vivi at gmail.com>
>>
>> On Wed, Nov 27, 2013 at 06:21:54PM -0200, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > These are needed when we cat the debugfs and sysfs files.
>> >
>> > V2: - Rebase
>> > V3: - Rebase
>> > V4: - Rebase
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > ---
>> > drivers/gpu/drm/i915/i915_debugfs.c | 45 ++++++++++++++++++++++++++++++++++---
>> > drivers/gpu/drm/i915/i915_sysfs.c | 14 ++++++++++--
>> > drivers/gpu/drm/i915/intel_dp.c | 11 +++++++--
>> > drivers/gpu/drm/i915/intel_panel.c | 3 +++
>> > drivers/gpu/drm/i915/intel_uncore.c | 4 ++++
>> > 5 files changed, 70 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> > index 13accf7..6badc15 100644
>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> > @@ -564,10 +564,12 @@ static int i915_gem_seqno_info(struct seq_file *m, void *data)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > for_each_ring(ring, dev_priv, i)
>> > i915_ring_seqno_info(m, ring);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > return 0;
>> > @@ -585,6 +587,7 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > if (INTEL_INFO(dev)->gen >= 8) {
>> > int i;
>> > @@ -711,6 +714,7 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
>> > }
>> > i915_ring_seqno_info(m, ring);
>> > }
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > return 0;
>> > @@ -904,9 +908,11 @@ static int i915_rstdby_delays(struct seq_file *m, void *unused)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > crstanddelay = I915_READ16(CRSTANDVID);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > seq_printf(m, "w/ctx: %d, w/o ctx: %d\n", (crstanddelay >> 8) & 0x3f, (crstanddelay & 0x3f));
>> > @@ -919,7 +925,9 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
>> > struct drm_info_node *node = (struct drm_info_node *) m->private;
>> > struct drm_device *dev = node->minor->dev;
>> > drm_i915_private_t *dev_priv = dev->dev_private;
>> > - int ret;
>> > + int ret = 0;
>> > +
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>> >
>> > @@ -945,7 +953,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
>> > /* RPSTAT1 is in the GT power well */
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > - return ret;
>> > + goto out;
>> >
>> > gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>> >
>> > @@ -1033,7 +1041,9 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
>> > seq_puts(m, "no P-state info available\n");
>> > }
>> >
>> > - return 0;
>> > +out:
>> > + intel_runtime_pm_put(dev_priv);
>> > + return ret;
>> > }
>> >
>> > static int i915_delayfreq_table(struct seq_file *m, void *unused)
>> > @@ -1047,6 +1057,7 @@ static int i915_delayfreq_table(struct seq_file *m, void *unused)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > for (i = 0; i < 16; i++) {
>> > delayfreq = I915_READ(PXVFREQ_BASE + i * 4);
>> > @@ -1054,6 +1065,8 @@ static int i915_delayfreq_table(struct seq_file *m, void *unused)
>> > (delayfreq & PXVFREQ_PX_MASK) >> PXVFREQ_PX_SHIFT);
>> > }
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > +
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > return 0;
>> > @@ -1075,12 +1088,14 @@ static int i915_inttoext_table(struct seq_file *m, void *unused)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > for (i = 1; i <= 32; i++) {
>> > inttoext = I915_READ(INTTOEXT_BASE_ILK + i * 4);
>> > seq_printf(m, "INTTOEXT%02d: 0x%08x\n", i, inttoext);
>> > }
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > return 0;
>> > @@ -1098,11 +1113,13 @@ static int ironlake_drpc_info(struct seq_file *m)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > rgvmodectl = I915_READ(MEMMODECTL);
>> > rstdbyctl = I915_READ(RSTDBYCTL);
>> > crstandvid = I915_READ16(CRSTANDVID);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > seq_printf(m, "HD boost: %s\n", (rgvmodectl & MEMMODE_BOOST_EN) ?
>> > @@ -1166,6 +1183,7 @@ static int gen6_drpc_info(struct seq_file *m)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > spin_lock_irq(&dev_priv->uncore.lock);
>> > forcewake_count = dev_priv->uncore.forcewake_count;
>> > @@ -1191,6 +1209,8 @@ static int gen6_drpc_info(struct seq_file *m)
>> > sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
>> > mutex_unlock(&dev_priv->rps.hw_lock);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > +
>> > seq_printf(m, "Video Turbo Mode: %s\n",
>> > yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
>> > seq_printf(m, "HW control enabled: %s\n",
>> > @@ -1405,6 +1425,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>> > ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
>> >
>> > @@ -1421,6 +1442,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>> > ((ia_freq >> 8) & 0xff) * 100);
>> > }
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev_priv->rps.hw_lock);
>> >
>> > return 0;
>> > @@ -1436,8 +1458,10 @@ static int i915_gfxec(struct seq_file *m, void *unused)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > seq_printf(m, "GFXEC: %ld\n", (unsigned long)I915_READ(0x112f4));
>> > + intel_runtime_pm_put(dev_priv);
>> >
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > @@ -1617,6 +1641,7 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
>> > swizzle_string(dev_priv->mm.bit_6_swizzle_x));
>> > @@ -1648,6 +1673,7 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
>> > seq_printf(m, "DISP_ARB_CTL = 0x%08x\n",
>> > I915_READ(DISP_ARB_CTL));
>> > }
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > return 0;
>> > @@ -1708,16 +1734,19 @@ static int i915_ppgtt_info(struct seq_file *m, void *data)
>> > {
>> > struct drm_info_node *node = (struct drm_info_node *) m->private;
>> > struct drm_device *dev = node->minor->dev;
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> >
>> > int ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > if (INTEL_INFO(dev)->gen >= 8)
>> > gen8_ppgtt_info(m, dev);
>> > else if (INTEL_INFO(dev)->gen >= 6)
>> > gen6_ppgtt_info(m, dev);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > return 0;
>> > @@ -1791,6 +1820,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>> > u32 psrperf = 0;
>> > bool enabled = false;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > +
>> > seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
>> > seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
>> >
>> > @@ -1803,6 +1834,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>> > EDP_PSR_PERF_CNT_MASK;
>> > seq_printf(m, "Performance_Counter: %u\n", psrperf);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > return 0;
>> > }
>> >
>> > @@ -3016,8 +3048,11 @@ i915_cache_sharing_get(void *data, u64 *val)
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> >
>> > snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>> > +
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev_priv->dev->struct_mutex);
>> >
>> > *val = (snpcr & GEN6_MBC_SNPCR_MASK) >> GEN6_MBC_SNPCR_SHIFT;
>> > @@ -3038,6 +3073,7 @@ i915_cache_sharing_set(void *data, u64 val)
>> > if (val > 3)
>> > return -EINVAL;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > DRM_DEBUG_DRIVER("Manually setting uncore sharing to %llu\n", val);
>> >
>> > /* Update the cache sharing policy here as well */
>> > @@ -3046,6 +3082,7 @@ i915_cache_sharing_set(void *data, u64 val)
>> > snpcr |= (val << GEN6_MBC_SNPCR_SHIFT);
>> > I915_WRITE(GEN6_MBCUNIT_SNPCR, snpcr);
>> >
>> > + intel_runtime_pm_put(dev_priv);
>> > return 0;
>> > }
>> >
>> > @@ -3061,6 +3098,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
>> > if (INTEL_INFO(dev)->gen < 6)
>> > return 0;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>> >
>> > return 0;
>> > @@ -3075,6 +3113,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>> > return 0;
>> >
>> > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>> > + intel_runtime_pm_put(dev_priv);
>> >
>> > return 0;
>> > }
>> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
>> > index 05d8b16..33bcae3 100644
>> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> > @@ -40,10 +40,13 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>> > struct drm_i915_private *dev_priv = dev->dev_private;
>> > u64 raw_time; /* 32b value may overflow during fixed point math */
>> > u64 units = 128ULL, div = 100000ULL, bias = 100ULL;
>> > + u32 ret;
>> >
>> > if (!intel_enable_rc6(dev))
>> > return 0;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > +
>> > /* On VLV, residency time is in CZ units rather than 1.28us */
>> > if (IS_VALLEYVIEW(dev)) {
>> > u32 clkctl2;
>> > @@ -52,7 +55,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>> > CLK_CTL2_CZCOUNT_30NS_SHIFT;
>> > if (!clkctl2) {
>> > WARN(!clkctl2, "bogus CZ count value");
>> > - return 0;
>> > + ret = 0;
>> > + goto out;
>> > }
>> > units = DIV_ROUND_UP_ULL(30ULL * bias, (u64)clkctl2);
>> > if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
>> > @@ -62,7 +66,11 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>> > }
>> >
>> > raw_time = I915_READ(reg) * units;
>> > - return DIV_ROUND_UP_ULL(raw_time, div);
>> > + ret = DIV_ROUND_UP_ULL(raw_time, div);
>> > +
>> > +out:
>> > + intel_runtime_pm_put(dev_priv);
>> > + return ret;
>> > }
>> >
>> > static ssize_t
>> > @@ -448,7 +456,9 @@ static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr
>> > ret = mutex_lock_interruptible(&dev->struct_mutex);
>> > if (ret)
>> > return ret;
>> > + intel_runtime_pm_get(dev_priv);
>> > rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
>> > + intel_runtime_pm_put(dev_priv);
>> > mutex_unlock(&dev->struct_mutex);
>> >
>> > if (attr == &dev_attr_gt_RP0_freq_mhz) {
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 1e372d5..28fc070 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -3082,9 +3082,12 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> > struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> > struct drm_device *dev = connector->dev;
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > enum drm_connector_status status;
>> > struct edid *edid = NULL;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > +
>> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> > connector->base.id, drm_get_connector_name(connector));
>> >
>> > @@ -3096,7 +3099,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>> > status = g4x_dp_detect(intel_dp);
>> >
>> > if (status != connector_status_connected)
>> > - return status;
>> > + goto out;
>> >
>> > intel_dp_probe_oui(intel_dp);
>> >
>> > @@ -3112,7 +3115,11 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>> >
>> > if (intel_encoder->type != INTEL_OUTPUT_EDP)
>> > intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>> > - return connector_status_connected;
>> > + status = connector_status_connected;
>> > +
>> > +out:
>> > + intel_runtime_pm_put(dev_priv);
>> > + return status;
>> > }
>> >
>> > static int intel_dp_get_modes(struct drm_connector *connector)
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> > index e480cf4..4b7925b 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -845,11 +845,14 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd)
>> > {
>> > struct intel_connector *connector = bl_get_data(bd);
>> > struct drm_device *dev = connector->base.dev;
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > int ret;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > mutex_lock(&dev->mode_config.mutex);
>> > ret = intel_panel_get_backlight(connector);
>> > mutex_unlock(&dev->mode_config.mutex);
>> > + intel_runtime_pm_put(dev_priv);
>> >
>> > return ret;
>> > }
>> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> > index 00e5ced..2d745a6 100644
>> > --- a/drivers/gpu/drm/i915/intel_uncore.c
>> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> > @@ -366,6 +366,8 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>> > if (!dev_priv->uncore.funcs.force_wake_get)
>> > return;
>> >
>> > + intel_runtime_pm_get(dev_priv);
>> > +
>> > /* Redirect to VLV specific routine */
>> > if (IS_VALLEYVIEW(dev_priv->dev))
>> > return vlv_force_wake_get(dev_priv, fw_engine);
>> > @@ -399,6 +401,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>> > 1);
>> > }
>> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> > +
>> > + intel_runtime_pm_put(dev_priv);
>> > }
>> >
>> > /* We give fast paths for the really cool registers */
>> > --
>> > 1.8.3.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
More information about the Intel-gfx
mailing list