* [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs
@ 2018-03-14 8:05 Tvrtko Ursulin
2018-03-14 8:15 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2018-03-14 8:05 UTC (permalink / raw)
To: Intel-gfx; +Cc: David Airlie, dri-devel, Rodrigo Vivi, intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Arnd Bergman reports:
"""
The conditional spinlock confuses gcc into thinking the 'flags' value
might contain uninitialized data:
drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
The code is correct, but it's easy to see how the compiler gets confused
here. This avoids the problem by pulling the lock outside of the function
into its only caller.
"""
On deeper look it seems this is caused by paravirt spinlocks
implementation when CONFIG_PARAVIRT_DEBUG is set, which by being
complicated, manages to convince gcc locked parameter can be changed
externally (impossible).
Work around it by removing the conditional locking parameters altogether.
(It was never the most elegant code anyway.)
Slight penalty we now pay is an additional irqsave spin lock/unlock cycle
on the event enable path. But since enable is not a fast path, that is
preferrable to the alternative solution which was doing MMIO under irqsave
spinlock.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_pmu.c | 32 +++++++++++++-------------------
1 file changed, 13 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 4bc7aefa9541..11fb76bd3860 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -412,7 +412,7 @@ static u64 __get_rc6(struct drm_i915_private *i915)
return val;
}
-static u64 get_rc6(struct drm_i915_private *i915, bool locked)
+static u64 get_rc6(struct drm_i915_private *i915)
{
#if IS_ENABLED(CONFIG_PM)
unsigned long flags;
@@ -428,8 +428,7 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
* previously.
*/
- if (!locked)
- spin_lock_irqsave(&i915->pmu.lock, flags);
+ spin_lock_irqsave(&i915->pmu.lock, flags);
if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
@@ -438,12 +437,10 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
}
- if (!locked)
- spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
} else {
struct pci_dev *pdev = i915->drm.pdev;
struct device *kdev = &pdev->dev;
- unsigned long flags2;
/*
* We are runtime suspended.
@@ -452,10 +449,8 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
* on top of the last known real value, as the approximated RC6
* counter value.
*/
- if (!locked)
- spin_lock_irqsave(&i915->pmu.lock, flags);
-
- spin_lock_irqsave(&kdev->power.lock, flags2);
+ spin_lock_irqsave(&i915->pmu.lock, flags);
+ spin_lock(&kdev->power.lock);
if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
i915->pmu.suspended_jiffies_last =
@@ -465,14 +460,13 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
i915->pmu.suspended_jiffies_last;
val += jiffies - kdev->power.accounting_timestamp;
- spin_unlock_irqrestore(&kdev->power.lock, flags2);
+ spin_unlock(&kdev->power.lock);
val = jiffies_to_nsecs(val);
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
- if (!locked)
- spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
return val;
@@ -481,7 +475,7 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
#endif
}
-static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
+static u64 __i915_pmu_event_read(struct perf_event *event)
{
struct drm_i915_private *i915 =
container_of(event->pmu, typeof(*i915), pmu.base);
@@ -519,7 +513,7 @@ static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
val = count_interrupts(i915);
break;
case I915_PMU_RC6_RESIDENCY:
- val = get_rc6(i915, locked);
+ val = get_rc6(i915);
break;
}
}
@@ -534,7 +528,7 @@ static void i915_pmu_event_read(struct perf_event *event)
again:
prev = local64_read(&hwc->prev_count);
- new = __i915_pmu_event_read(event, false);
+ new = __i915_pmu_event_read(event);
if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev)
goto again;
@@ -584,14 +578,14 @@ static void i915_pmu_enable(struct perf_event *event)
engine->pmu.enable_count[sample]++;
}
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
+
/*
* Store the current counter value so we can report the correct delta
* for all listeners. Even when the event was already enabled and has
* an existing non-zero value.
*/
- local64_set(&event->hw.prev_count, __i915_pmu_event_read(event, true));
-
- spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ local64_set(&event->hw.prev_count, __i915_pmu_event_read(event));
}
static void i915_pmu_disable(struct perf_event *event)
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: Work around compiler warnings on some kernel configs
2018-03-14 8:05 [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs Tvrtko Ursulin
@ 2018-03-14 8:15 ` Patchwork
2018-03-14 8:31 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-03-14 8:15 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Work around compiler warnings on some kernel configs
URL : https://patchwork.freedesktop.org/series/39939/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915/pmu: Work around compiler warnings on some kernel configs
-O:drivers/gpu/drm/i915/i915_pmu.c:442:47: warning: context imbalance in 'get_rc6' - unexpected unlock
+
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/pmu: Work around compiler warnings on some kernel configs
2018-03-14 8:05 [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs Tvrtko Ursulin
2018-03-14 8:15 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
@ 2018-03-14 8:31 ` Patchwork
2018-03-14 17:57 ` Tvrtko Ursulin
2018-03-14 10:00 ` ✓ Fi.CI.IGT: " Patchwork
2018-03-14 11:02 ` [PATCH] " Chris Wilson
3 siblings, 1 reply; 7+ messages in thread
From: Patchwork @ 2018-03-14 8:31 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Work around compiler warnings on some kernel configs
URL : https://patchwork.freedesktop.org/series/39939/
State : success
== Summary ==
Series 39939v1 drm/i915/pmu: Work around compiler warnings on some kernel configs
https://patchwork.freedesktop.org/api/1.0/series/39939/revisions/1/mbox/
---- Known issues:
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:431s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:439s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:382s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:534s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:301s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:504s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:513s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:507s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:498s
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:410s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:587s
fi-cfl-u total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:511s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:585s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:420s
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:316s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:530s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:401s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:418s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:477s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:430s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:474s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:470s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:513s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:441s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:529s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:539s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:512s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:498s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:428s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:437s
fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:398s
Blacklisted hosts:
fi-cnl-drrs total:288 pass:257 dwarn:3 dfail:0 fail:0 skip:28 time:517s
613eb885b69e808a46f11125870e47b67a326d76 drm-tip: 2018y-03m-14d-05h-56m-23s UTC integration manifest
34462fc0bee3 drm/i915/pmu: Work around compiler warnings on some kernel configs
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8338/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915/pmu: Work around compiler warnings on some kernel configs
2018-03-14 8:05 [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs Tvrtko Ursulin
2018-03-14 8:15 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
2018-03-14 8:31 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-03-14 10:00 ` Patchwork
2018-03-14 11:02 ` [PATCH] " Chris Wilson
3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-03-14 10:00 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Work around compiler warnings on some kernel configs
URL : https://patchwork.freedesktop.org/series/39939/
State : success
== Summary ==
---- Known issues:
Test gem_eio:
Subgroup in-flight-contexts:
pass -> INCOMPLETE (shard-apl) fdo#105341
Test kms_cursor_crc:
Subgroup cursor-64x64-suspend:
pass -> SKIP (shard-hsw) fdo#103540
Test kms_flip:
Subgroup 2x-modeset-vs-vblank-race-interruptible:
pass -> DMESG-WARN (shard-hsw) fdo#103060
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-offscren-pri-shrfb-draw-mmap-gtt:
pass -> FAIL (shard-apl) fdo#103167
Subgroup fbc-1p-primscrn-pri-shrfb-draw-pwrite:
pass -> DMESG-FAIL (shard-apl) fdo#101623
Subgroup fbc-1p-primscrn-shrfb-msflip-blt:
pass -> FAIL (shard-apl) fdo#104727
Test kms_setmode:
Subgroup basic:
pass -> FAIL (shard-hsw) fdo#99912
Test kms_sysfs_edid_timing:
pass -> WARN (shard-apl) fdo#100047
Test kms_vblank:
Subgroup pipe-b-ts-continuation-suspend:
pass -> SKIP (shard-snb) fdo#105411
Test prime_vgem:
Subgroup basic-fence-flip:
fail -> PASS (shard-apl) fdo#104008
fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#104727 https://bugs.freedesktop.org/show_bug.cgi?id=104727
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
shard-apl total:3360 pass:1774 dwarn:1 dfail:1 fail:8 skip:1574 time:12750s
shard-hsw total:3444 pass:1768 dwarn:2 dfail:0 fail:1 skip:1672 time:11668s
shard-snb total:3444 pass:1358 dwarn:1 dfail:0 fail:2 skip:2083 time:7064s
Blacklisted hosts:
shard-kbl total:3306 pass:1868 dwarn:1 dfail:0 fail:9 skip:1426 time:9373s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8338/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs
2018-03-14 8:05 [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs Tvrtko Ursulin
` (2 preceding siblings ...)
2018-03-14 10:00 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-03-14 11:02 ` Chris Wilson
3 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2018-03-14 11:02 UTC (permalink / raw)
To: Tvrtko Ursulin, Intel-gfx
Cc: David Airlie, dri-devel, Rodrigo Vivi, intel-gfx
Quoting Tvrtko Ursulin (2018-03-14 08:05:35)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Arnd Bergman reports:
> """
> The conditional spinlock confuses gcc into thinking the 'flags' value
> might contain uninitialized data:
>
> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The code is correct, but it's easy to see how the compiler gets confused
> here. This avoids the problem by pulling the lock outside of the function
> into its only caller.
> """
>
> On deeper look it seems this is caused by paravirt spinlocks
> implementation when CONFIG_PARAVIRT_DEBUG is set, which by being
> complicated, manages to convince gcc locked parameter can be changed
> externally (impossible).
>
> Work around it by removing the conditional locking parameters altogether.
> (It was never the most elegant code anyway.)
>
> Slight penalty we now pay is an additional irqsave spin lock/unlock cycle
> on the event enable path. But since enable is not a fast path, that is
> preferrable to the alternative solution which was doing MMIO under irqsave
> spinlock.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 4bc7aefa9541..11fb76bd3860 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -412,7 +412,7 @@ static u64 __get_rc6(struct drm_i915_private *i915)
> return val;
> }
>
> -static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> +static u64 get_rc6(struct drm_i915_private *i915)
> {
> #if IS_ENABLED(CONFIG_PM)
> unsigned long flags;
> @@ -428,8 +428,7 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> * previously.
> */
>
> - if (!locked)
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> + spin_lock_irqsave(&i915->pmu.lock, flags);
>
> if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> @@ -438,12 +437,10 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> }
>
> - if (!locked)
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> } else {
> struct pci_dev *pdev = i915->drm.pdev;
> struct device *kdev = &pdev->dev;
> - unsigned long flags2;
>
> /*
> * We are runtime suspended.
> @@ -452,10 +449,8 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> * on top of the last known real value, as the approximated RC6
> * counter value.
> */
> - if (!locked)
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> -
> - spin_lock_irqsave(&kdev->power.lock, flags2);
> + spin_lock_irqsave(&i915->pmu.lock, flags);
> + spin_lock(&kdev->power.lock);
>
> if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> i915->pmu.suspended_jiffies_last =
> @@ -465,14 +460,13 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> i915->pmu.suspended_jiffies_last;
> val += jiffies - kdev->power.accounting_timestamp;
>
> - spin_unlock_irqrestore(&kdev->power.lock, flags2);
> + spin_unlock(&kdev->power.lock);
>
> val = jiffies_to_nsecs(val);
> val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
>
> - if (!locked)
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> }
>
> return val;
> @@ -481,7 +475,7 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> #endif
> }
>
> -static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
> +static u64 __i915_pmu_event_read(struct perf_event *event)
> {
> struct drm_i915_private *i915 =
> container_of(event->pmu, typeof(*i915), pmu.base);
> @@ -519,7 +513,7 @@ static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
> val = count_interrupts(i915);
> break;
> case I915_PMU_RC6_RESIDENCY:
> - val = get_rc6(i915, locked);
> + val = get_rc6(i915);
> break;
> }
> }
> @@ -534,7 +528,7 @@ static void i915_pmu_event_read(struct perf_event *event)
>
> again:
> prev = local64_read(&hwc->prev_count);
> - new = __i915_pmu_event_read(event, false);
> + new = __i915_pmu_event_read(event);
>
> if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev)
> goto again;
> @@ -584,14 +578,14 @@ static void i915_pmu_enable(struct perf_event *event)
> engine->pmu.enable_count[sample]++;
> }
>
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> +
> /*
> * Store the current counter value so we can report the correct delta
> * for all listeners. Even when the event was already enabled and has
> * an existing non-zero value.
> */
> - local64_set(&event->hw.prev_count, __i915_pmu_event_read(event, true));
> -
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + local64_set(&event->hw.prev_count, __i915_pmu_event_read(event));
> }
Ok, the only question then is whether pmu_enable/pmu_disable is
externally serialised. Afaict, that is true through serialisation of
event->state.
Hmm, actually instead of doing local64_set() here, it would be symmetric
with i915_pmu_event_stop() if it was done in i915_pmu_event_start():
i915_pmu_event_start {
i915_pmu_enable(event);
/* ... */
i915_pmu_event_read(event);
event->hw.state = 0;
}
i915_pmu_event_stop {
if (flags & UPDATE)
i915_pmu_event_read(event);
i915_pmu_disable(event);
event->hw.state = STOPPED;
}
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Can you follow up with that adjustment for balance, if it suits you?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ✓ Fi.CI.BAT: success for drm/i915/pmu: Work around compiler warnings on some kernel configs
2018-03-14 8:31 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-03-14 17:57 ` Tvrtko Ursulin
2018-03-14 20:08 ` Arnd Bergmann
0 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2018-03-14 17:57 UTC (permalink / raw)
To: intel-gfx, Patchwork, Tvrtko Ursulin; +Cc: Arnd Bergmann
On 14/03/2018 08:31, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/pmu: Work around compiler warnings on some kernel configs
> URL : https://patchwork.freedesktop.org/series/39939/
> State : success
>
> == Summary ==
>
> Series 39939v1 drm/i915/pmu: Work around compiler warnings on some kernel configs
> https://patchwork.freedesktop.org/api/1.0/series/39939/revisions/1/mbox/
>
> ---- Known issues:
>
> Test gem_mmap_gtt:
> Subgroup basic-small-bo-tiledx:
> fail -> PASS (fi-gdg-551) fdo#102575
> Test kms_pipe_crc_basic:
> Subgroup suspend-read-crc-pipe-b:
> pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
>
> fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
> fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
>
> fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:431s
> fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:439s
> fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:382s
> fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:534s
> fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:301s
> fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:504s
> fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:513s
> fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:507s
> fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:498s
> fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:410s
> fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:587s
> fi-cfl-u total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:511s
> fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:585s
> fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:420s
> fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:316s
> fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:530s
> fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:401s
> fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:418s
> fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:477s
> fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:430s
> fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:474s
> fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:470s
> fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:513s
> fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:441s
> fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:529s
> fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:539s
> fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:512s
> fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:498s
> fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:428s
> fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:437s
> fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
> fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:398s
> Blacklisted hosts:
> fi-cnl-drrs total:288 pass:257 dwarn:3 dfail:0 fail:0 skip:28 time:517s
>
> 613eb885b69e808a46f11125870e47b67a326d76 drm-tip: 2018y-03m-14d-05h-56m-23s UTC integration manifest
> 34462fc0bee3 drm/i915/pmu: Work around compiler warnings on some kernel configs
Pushed it, thanks for the review!
(And I forgot to copy Arnd on the patch..)
So Arnd, sorry, I forgot Reported-by does not add Cc from git
send-email. https://patchwork.freedesktop.org/series/39939/ is my
version of the fix for this. (Now merged.) Hopefully it works for your
randconfigs as well and thanks for sending a patch in the first place!
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ✓ Fi.CI.BAT: success for drm/i915/pmu: Work around compiler warnings on some kernel configs
2018-03-14 17:57 ` Tvrtko Ursulin
@ 2018-03-14 20:08 ` Arnd Bergmann
0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2018-03-14 20:08 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel Graphics
On Wed, Mar 14, 2018 at 6:57 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
> On 14/03/2018 08:31, Patchwork wrote:
>
> Pushed it, thanks for the review!
>
> (And I forgot to copy Arnd on the patch..)
>
> So Arnd, sorry, I forgot Reported-by does not add Cc from git send-email.
> https://patchwork.freedesktop.org/series/39939/ is my version of the fix for
> this. (Now merged.) Hopefully it works for your randconfigs as well and
> thanks for sending a patch in the first place!
The patch looks good to me, thanks for the follow-up
Acked-by: Arnd Bergmann <arnd@arndb.de>
I didn't test it, but you'll hear from me if it breaks again.
One comment about the use of spin_lock_irqsave(), you wrote:
"Slight penalty we now pay is an additional irqsave spin lock/unlock cycle
on the event enable path. But since enable is not a fast path, that is
preferrable to the alternative solution which was doing MMIO under irqsave
spinlock."
While I don't know about the exact cost on x86, on many architectures
spin_lock_irqsave()/spin_unlock_irqrestore() is much more expensive
than a plain spin_lock()/spin_unlock() or spin_lock_irq()/spin_unlock_irq()
pair that doesn't have to store the disabled-state.
I see you already removed the inner irqsave()/irqrestore() pair that
is now useless (I failed to notice that in my original patch), but in my
experience, it's usually possible to do the same for many others
as well after proving that a function is always called with IRQs enabled
or always disabled.
Arnd
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-14 20:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-14 8:05 [PATCH] drm/i915/pmu: Work around compiler warnings on some kernel configs Tvrtko Ursulin
2018-03-14 8:15 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
2018-03-14 8:31 ` ✓ Fi.CI.BAT: success " Patchwork
2018-03-14 17:57 ` Tvrtko Ursulin
2018-03-14 20:08 ` Arnd Bergmann
2018-03-14 10:00 ` ✓ Fi.CI.IGT: " Patchwork
2018-03-14 11:02 ` [PATCH] " Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.