* [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32
@ 2023-05-16 9:24 Tvrtko Ursulin
2023-05-16 11:56 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2023-05-16 9:24 UTC (permalink / raw)
To: Intel-gfx, dri-devel
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Having it as u64 was a confusing (but harmless) mistake.
Also add some asserts to make sure the internal field does not overflow
in the future.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
I am not entirely sure the __builtin_constant_p->BUILD_BUG_ON branch will
work with all compilers. Lets see...
Compile tested only.
---
drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..8736b3418f88 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event)
return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
}
-static bool is_engine_config(u64 config)
+static bool is_engine_config(const u64 config)
{
return config < __I915_PMU_OTHER(0);
}
@@ -82,15 +82,28 @@ static unsigned int other_bit(const u64 config)
static unsigned int config_bit(const u64 config)
{
+ unsigned int bit;
+
if (is_engine_config(config))
- return engine_config_sample(config);
+ bit = engine_config_sample(config);
else
- return other_bit(config);
+ bit = other_bit(config);
+
+ if (__builtin_constant_p(config))
+ BUILD_BUG_ON(bit >
+ BITS_PER_TYPE(typeof_member(struct i915_pmu,
+ enable)) - 1);
+ else
+ WARN_ON_ONCE(bit >
+ BITS_PER_TYPE(typeof_member(struct i915_pmu,
+ enable)) - 1);
+
+ return bit;
}
-static u64 config_mask(u64 config)
+static u32 config_mask(const u64 config)
{
- return BIT_ULL(config_bit(config));
+ return BIT(config_bit(config));
}
static bool is_engine_event(struct perf_event *event)
@@ -633,11 +646,10 @@ static void i915_pmu_enable(struct perf_event *event)
{
struct drm_i915_private *i915 =
container_of(event->pmu, typeof(*i915), pmu.base);
+ const unsigned int bit = event_bit(event);
struct i915_pmu *pmu = &i915->pmu;
unsigned long flags;
- unsigned int bit;
- bit = event_bit(event);
if (bit == -1)
goto update;
@@ -651,7 +663,7 @@ static void i915_pmu_enable(struct perf_event *event)
GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
GEM_BUG_ON(pmu->enable_count[bit] == ~0);
- pmu->enable |= BIT_ULL(bit);
+ pmu->enable |= BIT(bit);
pmu->enable_count[bit]++;
/*
@@ -698,7 +710,7 @@ static void i915_pmu_disable(struct perf_event *event)
{
struct drm_i915_private *i915 =
container_of(event->pmu, typeof(*i915), pmu.base);
- unsigned int bit = event_bit(event);
+ const unsigned int bit = event_bit(event);
struct i915_pmu *pmu = &i915->pmu;
unsigned long flags;
@@ -734,7 +746,7 @@ static void i915_pmu_disable(struct perf_event *event)
* bitmask when the last listener on an event goes away.
*/
if (--pmu->enable_count[bit] == 0) {
- pmu->enable &= ~BIT_ULL(bit);
+ pmu->enable &= ~BIT(bit);
pmu->timer_enabled &= pmu_needs_timer(pmu, true);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/pmu: Change bitmask of enabled events to u32 2023-05-16 9:24 [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 Tvrtko Ursulin @ 2023-05-16 11:56 ` Patchwork 2023-05-16 16:54 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh 2023-05-16 22:13 ` Umesh Nerlige Ramappa 2 siblings, 0 replies; 5+ messages in thread From: Patchwork @ 2023-05-16 11:56 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 8513 bytes --] == Series Details == Series: drm/i915/pmu: Change bitmask of enabled events to u32 URL : https://patchwork.freedesktop.org/series/117805/ State : failure == Summary == CI Bug Log - changes from CI_DRM_13151 -> Patchwork_117805v1 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_117805v1 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_117805v1, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/index.html Participating hosts (38 -> 37) ------------------------------ Missing (1): fi-snb-2520m Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_117805v1: ### IGT changes ### #### Possible regressions #### * igt@gem_exec_parallel@engines@fds: - bat-atsm-1: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-atsm-1/igt@gem_exec_parallel@engines@fds.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-atsm-1/igt@gem_exec_parallel@engines@fds.html Known issues ------------ Here are the changes found in Patchwork_117805v1 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@i915_module_load@load: - bat-adls-5: [PASS][3] -> [ABORT][4] ([i915#4391]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-adls-5/igt@i915_module_load@load.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-adls-5/igt@i915_module_load@load.html * igt@i915_selftest@live@gt_heartbeat: - fi-kbl-soraka: [PASS][5] -> [DMESG-FAIL][6] ([i915#5334] / [i915#7872]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html - bat-jsl-1: [PASS][7] -> [DMESG-FAIL][8] ([i915#5334]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-jsl-1/igt@i915_selftest@live@gt_heartbeat.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-jsl-1/igt@i915_selftest@live@gt_heartbeat.html * igt@i915_selftest@live@hangcheck: - fi-skl-guc: [PASS][9] -> [DMESG-WARN][10] ([i915#8073]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/fi-skl-guc/igt@i915_selftest@live@hangcheck.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/fi-skl-guc/igt@i915_selftest@live@hangcheck.html * igt@i915_suspend@basic-s2idle-without-i915: - bat-rpls-2: NOTRUN -> [ABORT][11] ([i915#6687]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-rpls-2/igt@i915_suspend@basic-s2idle-without-i915.html * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence: - bat-dg2-11: NOTRUN -> [SKIP][12] ([i915#1845] / [i915#5354]) +2 similar issues [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html #### Possible fixes #### * igt@i915_pm_rpm@basic-rte: - {bat-mtlp-8}: [TIMEOUT][13] -> [PASS][14] [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-mtlp-8/igt@i915_pm_rpm@basic-rte.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-mtlp-8/igt@i915_pm_rpm@basic-rte.html * igt@i915_selftest@live@requests: - {bat-mtlp-8}: [ABORT][15] ([i915#4983] / [i915#7920] / [i915#7953]) -> [PASS][16] [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-mtlp-8/igt@i915_selftest@live@requests.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-mtlp-8/igt@i915_selftest@live@requests.html - {bat-mtlp-6}: [ABORT][17] ([i915#4983] / [i915#7920] / [i915#7953]) -> [PASS][18] [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-mtlp-6/igt@i915_selftest@live@requests.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-mtlp-6/igt@i915_selftest@live@requests.html * igt@i915_selftest@live@reset: - bat-rpls-2: [ABORT][19] ([i915#4983] / [i915#7461] / [i915#7913] / [i915#8347]) -> [PASS][20] [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-rpls-2/igt@i915_selftest@live@reset.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-rpls-2/igt@i915_selftest@live@reset.html * igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1: - bat-dg2-8: [FAIL][21] ([i915#7932]) -> [PASS][22] +2 similar issues [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html #### Warnings #### * igt@i915_selftest@live@slpc: - bat-rpls-1: [DMESG-WARN][23] ([i915#6367] / [i915#7953]) -> [DMESG-WARN][24] ([i915#6367]) [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-rpls-1/igt@i915_selftest@live@slpc.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-rpls-1/igt@i915_selftest@live@slpc.html * igt@i915_suspend@basic-s3-without-i915: - fi-tgl-1115g4: [INCOMPLETE][25] ([i915#7443] / [i915#7953] / [i915#8102]) -> [INCOMPLETE][26] ([i915#7443] / [i915#8102]) [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/fi-tgl-1115g4/igt@i915_suspend@basic-s3-without-i915.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/fi-tgl-1115g4/igt@i915_suspend@basic-s3-without-i915.html - bat-rpls-1: [ABORT][27] ([i915#6687] / [i915#7953] / [i915#7978]) -> [ABORT][28] ([i915#6687] / [i915#7978]) [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13151/bat-rpls-1/igt@i915_suspend@basic-s3-without-i915.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/bat-rpls-1/igt@i915_suspend@basic-s3-without-i915.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845 [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078 [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391 [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334 [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354 [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367 [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645 [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687 [i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443 [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#7872]: https://gitlab.freedesktop.org/drm/intel/issues/7872 [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913 [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920 [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932 [i915#7953]: https://gitlab.freedesktop.org/drm/intel/issues/7953 [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978 [i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073 [i915#8102]: https://gitlab.freedesktop.org/drm/intel/issues/8102 [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347 Build changes ------------- * Linux: CI_DRM_13151 -> Patchwork_117805v1 CI-20190529: 20190529 CI_DRM_13151: 4a28d03908b9d284f0b64a1fd9f5304761460db8 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7292: 9d9475ffd3b5ae18fd8ec120595385f6c562f249 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_117805v1: 4a28d03908b9d284f0b64a1fd9f5304761460db8 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits c5dd01180977 drm/i915/pmu: Change bitmask of enabled events to u32 == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117805v1/index.html [-- Attachment #2: Type: text/html, Size: 10193 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 2023-05-16 9:24 [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 Tvrtko Ursulin 2023-05-16 11:56 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork @ 2023-05-16 16:54 ` Dixit, Ashutosh 2023-05-16 22:13 ` Umesh Nerlige Ramappa 2 siblings, 0 replies; 5+ messages in thread From: Dixit, Ashutosh @ 2023-05-16 16:54 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, dri-devel On Tue, 16 May 2023 02:24:45 -0700, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Having it as u64 was a confusing (but harmless) mistake. > > Also add some asserts to make sure the internal field does not overflow > in the future. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > I am not entirely sure the __builtin_constant_p->BUILD_BUG_ON branch will > work with all compilers. Lets see... > > Compile tested only. > --- > drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index 7ece883a7d95..8736b3418f88 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event) > return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff; > } > > -static bool is_engine_config(u64 config) > +static bool is_engine_config(const u64 config) > { > return config < __I915_PMU_OTHER(0); > } > @@ -82,15 +82,28 @@ static unsigned int other_bit(const u64 config) > > static unsigned int config_bit(const u64 config) > { > + unsigned int bit; > + > if (is_engine_config(config)) > - return engine_config_sample(config); > + bit = engine_config_sample(config); > else > - return other_bit(config); > + bit = other_bit(config); > + > + if (__builtin_constant_p(config)) > + BUILD_BUG_ON(bit > > + BITS_PER_TYPE(typeof_member(struct i915_pmu, > + enable)) - 1); Given that config comes from the event (it is event->attr.config), can this ever be a builtin constant? > + else > + WARN_ON_ONCE(bit > > + BITS_PER_TYPE(typeof_member(struct i915_pmu, > + enable)) - 1); There is really an even stricter limit on what the bit can be, which is the total number of possible events but anyway this is good enough. So this patch is: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > + > + return bit; > } > > -static u64 config_mask(u64 config) > +static u32 config_mask(const u64 config) > { > - return BIT_ULL(config_bit(config)); > + return BIT(config_bit(config)); > } > > static bool is_engine_event(struct perf_event *event) > @@ -633,11 +646,10 @@ static void i915_pmu_enable(struct perf_event *event) > { > struct drm_i915_private *i915 = > container_of(event->pmu, typeof(*i915), pmu.base); > + const unsigned int bit = event_bit(event); > struct i915_pmu *pmu = &i915->pmu; > unsigned long flags; > - unsigned int bit; > > - bit = event_bit(event); > if (bit == -1) > goto update; > > @@ -651,7 +663,7 @@ static void i915_pmu_enable(struct perf_event *event) > GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count)); > GEM_BUG_ON(pmu->enable_count[bit] == ~0); > > - pmu->enable |= BIT_ULL(bit); > + pmu->enable |= BIT(bit); > pmu->enable_count[bit]++; > > /* > @@ -698,7 +710,7 @@ static void i915_pmu_disable(struct perf_event *event) > { > struct drm_i915_private *i915 = > container_of(event->pmu, typeof(*i915), pmu.base); > - unsigned int bit = event_bit(event); > + const unsigned int bit = event_bit(event); > struct i915_pmu *pmu = &i915->pmu; > unsigned long flags; > > @@ -734,7 +746,7 @@ static void i915_pmu_disable(struct perf_event *event) > * bitmask when the last listener on an event goes away. > */ > if (--pmu->enable_count[bit] == 0) { > - pmu->enable &= ~BIT_ULL(bit); > + pmu->enable &= ~BIT(bit); > pmu->timer_enabled &= pmu_needs_timer(pmu, true); > } > > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 2023-05-16 9:24 [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 Tvrtko Ursulin 2023-05-16 11:56 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork 2023-05-16 16:54 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh @ 2023-05-16 22:13 ` Umesh Nerlige Ramappa 2023-05-16 23:52 ` Umesh Nerlige Ramappa 2 siblings, 1 reply; 5+ messages in thread From: Umesh Nerlige Ramappa @ 2023-05-16 22:13 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, dri-devel On Tue, May 16, 2023 at 10:24:45AM +0100, Tvrtko Ursulin wrote: >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Having it as u64 was a confusing (but harmless) mistake. > >Also add some asserts to make sure the internal field does not overflow >in the future. > >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> >Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >--- >I am not entirely sure the __builtin_constant_p->BUILD_BUG_ON branch will >work with all compilers. Lets see... > >Compile tested only. >--- > drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >index 7ece883a7d95..8736b3418f88 100644 >--- a/drivers/gpu/drm/i915/i915_pmu.c >+++ b/drivers/gpu/drm/i915/i915_pmu.c >@@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event) > return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff; > } > >-static bool is_engine_config(u64 config) >+static bool is_engine_config(const u64 config) > { > return config < __I915_PMU_OTHER(0); > } >@@ -82,15 +82,28 @@ static unsigned int other_bit(const u64 config) > > static unsigned int config_bit(const u64 config) > { >+ unsigned int bit; >+ > if (is_engine_config(config)) >- return engine_config_sample(config); >+ bit = engine_config_sample(config); > else >- return other_bit(config); >+ bit = other_bit(config); >+ >+ if (__builtin_constant_p(config)) >+ BUILD_BUG_ON(bit > >+ BITS_PER_TYPE(typeof_member(struct i915_pmu, >+ enable)) - 1); >+ else >+ WARN_ON_ONCE(bit > >+ BITS_PER_TYPE(typeof_member(struct i915_pmu, >+ enable)) - 1); The else is firing for the INTERRUPT event because event_bit() also calls config_bit(). It would be best to move this check to config_mask() and leave this function as is. Thanks, Umesh >+ >+ return bit; > } > >-static u64 config_mask(u64 config) >+static u32 config_mask(const u64 config) > { >- return BIT_ULL(config_bit(config)); >+ return BIT(config_bit(config)); > } > > static bool is_engine_event(struct perf_event *event) >@@ -633,11 +646,10 @@ static void i915_pmu_enable(struct perf_event *event) > { > struct drm_i915_private *i915 = > container_of(event->pmu, typeof(*i915), pmu.base); >+ const unsigned int bit = event_bit(event); > struct i915_pmu *pmu = &i915->pmu; > unsigned long flags; >- unsigned int bit; > >- bit = event_bit(event); > if (bit == -1) > goto update; > >@@ -651,7 +663,7 @@ static void i915_pmu_enable(struct perf_event *event) > GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count)); > GEM_BUG_ON(pmu->enable_count[bit] == ~0); > >- pmu->enable |= BIT_ULL(bit); >+ pmu->enable |= BIT(bit); > pmu->enable_count[bit]++; > > /* >@@ -698,7 +710,7 @@ static void i915_pmu_disable(struct perf_event *event) > { > struct drm_i915_private *i915 = > container_of(event->pmu, typeof(*i915), pmu.base); >- unsigned int bit = event_bit(event); >+ const unsigned int bit = event_bit(event); > struct i915_pmu *pmu = &i915->pmu; > unsigned long flags; > >@@ -734,7 +746,7 @@ static void i915_pmu_disable(struct perf_event *event) > * bitmask when the last listener on an event goes away. > */ > if (--pmu->enable_count[bit] == 0) { >- pmu->enable &= ~BIT_ULL(bit); >+ pmu->enable &= ~BIT(bit); > pmu->timer_enabled &= pmu_needs_timer(pmu, true); > } > >-- >2.39.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 2023-05-16 22:13 ` Umesh Nerlige Ramappa @ 2023-05-16 23:52 ` Umesh Nerlige Ramappa 0 siblings, 0 replies; 5+ messages in thread From: Umesh Nerlige Ramappa @ 2023-05-16 23:52 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, dri-devel On Tue, May 16, 2023 at 03:13:01PM -0700, Umesh Nerlige Ramappa wrote: >On Tue, May 16, 2023 at 10:24:45AM +0100, Tvrtko Ursulin wrote: >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >>Having it as u64 was a confusing (but harmless) mistake. >> >>Also add some asserts to make sure the internal field does not overflow >>in the future. >> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> >>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >>--- >>I am not entirely sure the __builtin_constant_p->BUILD_BUG_ON branch will >>work with all compilers. Lets see... >> >>Compile tested only. >>--- >>drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++++++++++++++++---------- >>1 file changed, 22 insertions(+), 10 deletions(-) >> >>diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >>index 7ece883a7d95..8736b3418f88 100644 >>--- a/drivers/gpu/drm/i915/i915_pmu.c >>+++ b/drivers/gpu/drm/i915/i915_pmu.c >>@@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event) >> return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff; >>} >> >>-static bool is_engine_config(u64 config) >>+static bool is_engine_config(const u64 config) >>{ >> return config < __I915_PMU_OTHER(0); >>} >>@@ -82,15 +82,28 @@ static unsigned int other_bit(const u64 config) >> >>static unsigned int config_bit(const u64 config) >>{ >>+ unsigned int bit; >>+ >> if (is_engine_config(config)) >>- return engine_config_sample(config); >>+ bit = engine_config_sample(config); >> else >>- return other_bit(config); >>+ bit = other_bit(config); >>+ >>+ if (__builtin_constant_p(config)) >>+ BUILD_BUG_ON(bit > >>+ BITS_PER_TYPE(typeof_member(struct i915_pmu, >>+ enable)) - 1); >>+ else >>+ WARN_ON_ONCE(bit > >>+ BITS_PER_TYPE(typeof_member(struct i915_pmu, >>+ enable)) - 1); > >The else is firing for the INTERRUPT event because event_bit() also >calls config_bit(). It would be best to move this check to >config_mask() and leave this function as is. I posted the modified version here - https://patchwork.freedesktop.org/patch/537361/?series=117843&rev=1 as part of the MTL PMU series so that it Tests out with IGT patches. Thanks, Umesh > >Thanks, >Umesh > >>+ >>+ return bit; >>} >> >>-static u64 config_mask(u64 config) >>+static u32 config_mask(const u64 config) >>{ >>- return BIT_ULL(config_bit(config)); >>+ return BIT(config_bit(config)); >>} >> >>static bool is_engine_event(struct perf_event *event) >>@@ -633,11 +646,10 @@ static void i915_pmu_enable(struct perf_event *event) >>{ >> struct drm_i915_private *i915 = >> container_of(event->pmu, typeof(*i915), pmu.base); >>+ const unsigned int bit = event_bit(event); >> struct i915_pmu *pmu = &i915->pmu; >> unsigned long flags; >>- unsigned int bit; >> >>- bit = event_bit(event); >> if (bit == -1) >> goto update; >> >>@@ -651,7 +663,7 @@ static void i915_pmu_enable(struct perf_event *event) >> GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count)); >> GEM_BUG_ON(pmu->enable_count[bit] == ~0); >> >>- pmu->enable |= BIT_ULL(bit); >>+ pmu->enable |= BIT(bit); >> pmu->enable_count[bit]++; >> >> /* >>@@ -698,7 +710,7 @@ static void i915_pmu_disable(struct perf_event *event) >>{ >> struct drm_i915_private *i915 = >> container_of(event->pmu, typeof(*i915), pmu.base); >>- unsigned int bit = event_bit(event); >>+ const unsigned int bit = event_bit(event); >> struct i915_pmu *pmu = &i915->pmu; >> unsigned long flags; >> >>@@ -734,7 +746,7 @@ static void i915_pmu_disable(struct perf_event *event) >> * bitmask when the last listener on an event goes away. >> */ >> if (--pmu->enable_count[bit] == 0) { >>- pmu->enable &= ~BIT_ULL(bit); >>+ pmu->enable &= ~BIT(bit); >> pmu->timer_enabled &= pmu_needs_timer(pmu, true); >> } >> >>-- >>2.39.2 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-16 23:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-16 9:24 [Intel-gfx] [PATCH] drm/i915/pmu: Change bitmask of enabled events to u32 Tvrtko Ursulin 2023-05-16 11:56 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork 2023-05-16 16:54 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh 2023-05-16 22:13 ` Umesh Nerlige Ramappa 2023-05-16 23:52 ` Umesh Nerlige Ramappa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox