[v5,1/7] drm/i915/pmu: Change bitmask of enabled events to u32

Commit Message

Umesh Nerlige Ramappa May 16, 2023, 11:35 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

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.

v2: Fix WARN_ON firing for INTERRUPT event (Umesh)

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
Cc: Ashutosh Dixit <[e-mail]>
---
 drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Comments

Dixit, Ashutosh May 17, 2023, 12:25 a.m. UTC | #1

On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh/Tvrtko,

Mostly repeating comments/questions made on the previous patch below.

> From: Tvrtko Ursulin <[e-mail]>
>
> 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.
>
> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
> Cc: Ashutosh Dixit <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 7ece883a7d95..96543dce2db1 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);
>  }
> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>		return other_bit(config);
>  }
>
> -static u64 config_mask(u64 config)
> +static u32 config_mask(const u64 config)
>  {
> -	return BIT_ULL(config_bit(config));
> +	unsigned int bit = config_bit(config);

Give that config_bit() can return -1 (I understand it is avoided in moving
the code to config_mask from config_bit), maybe the code below should also
have that check?

	int bit = config_bit(config);

	if (bit != -1)
	{
		...
	}

Though as mentioned below the 'if (__builtin_constant_p())' would have to
go. Maybe the code could even have stayed in config_bit with the check.

> +
> +	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.

After addressing the above, this patch is:

Reviewed-by: Ashutosh Dixit <[e-mail]>

> +
> +	return BIT(config_bit(config));
>  }
>
>  static bool is_engine_event(struct perf_event *event)
> @@ -633,11 +644,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 +661,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 +708,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 +744,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.36.1
>

Umesh Nerlige Ramappa May 17, 2023, 6:55 a.m. UTC | #2

On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh/Tvrtko,
>
>Mostly repeating comments/questions made on the previous patch below.
>
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> 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.
>>
>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>>
>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
>> Cc: Ashutosh Dixit <[e-mail]>
>> ---
>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index 7ece883a7d95..96543dce2db1 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);
>>  }
>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>>		return other_bit(config);
>>  }
>>
>> -static u64 config_mask(u64 config)
>> +static u32 config_mask(const u64 config)
>>  {
>> -	return BIT_ULL(config_bit(config));
>> +	unsigned int bit = config_bit(config);
>
>Give that config_bit() can return -1 (I understand it is avoided in moving
>the code to config_mask from config_bit), maybe the code below should also
>have that check?

config_mask is only called to check frequency related events in the 
code, so I don't see it returing -1 here.

>
>	int bit = config_bit(config);
>
>	if (bit != -1)
>	{
>		...
>	}
>
>Though as mentioned below the 'if (__builtin_constant_p())' would have to
>go. Maybe the code could even have stayed in config_bit with the check.
>
>> +
>> +	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?

Not sure about earlier code where these checks were inside config_bit(), 
but with changes I made, I don't see this being a builtin constant.  
However, nothing prevents a caller from just passing a builtin_constant 
to this in future.

Thanks,
Umesh

>
>> +	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.
>
>After addressing the above, this patch is:
>
>Reviewed-by: Ashutosh Dixit <[e-mail]>
>
>> +
>> +	return BIT(config_bit(config));
>>  }
>>
>>  static bool is_engine_event(struct perf_event *event)
>> @@ -633,11 +644,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 +661,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 +708,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 +744,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.36.1
>>

Tvrtko Ursulin May 17, 2023, 8:26 a.m. UTC | #3

On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
> On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>>>
>>
>> Hi Umesh/Tvrtko,
>>
>> Mostly repeating comments/questions made on the previous patch below.

First of all thanks for improving this, my v1 obviously wasn't good enough.

>>
>>> From: Tvrtko Ursulin <[e-mail]>
>>>
>>> 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.
>>>
>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>>>
>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
>>> Cc: Ashutosh Dixit <[e-mail]>
>>> ---
>>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c 
>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>> index 7ece883a7d95..96543dce2db1 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);
>>>  }
>>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>>>         return other_bit(config);
>>>  }
>>>
>>> -static u64 config_mask(u64 config)
>>> +static u32 config_mask(const u64 config)
>>>  {
>>> -    return BIT_ULL(config_bit(config));
>>> +    unsigned int bit = config_bit(config);
>>
>> Give that config_bit() can return -1 (I understand it is avoided in 
>> moving
>> the code to config_mask from config_bit), maybe the code below should 
>> also
>> have that check?
> 
> config_mask is only called to check frequency related events in the 
> code, so I don't see it returing -1 here.

Yeah that should be fine since -1 would make the below asserts fire 
anyway. (If it would get called from a different path in the future.)

>>
>>     int bit = config_bit(config);
>>
>>     if (bit != -1)
>>     {
>>         ...
>>     }
>>
>> Though as mentioned below the 'if (__builtin_constant_p())' would have to
>> go. Maybe the code could even have stayed in config_bit with the check.
>>
>>> +
>>> +    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?
> 
> Not sure about earlier code where these checks were inside config_bit(), 
> but with changes I made, I don't see this being a builtin constant. 
> However, nothing prevents a caller from just passing a builtin_constant 
> to this in future.

Are you sure? I would have thought it would always be a compile time 
constant now that the check is in config_mask. Aahhh.. with the 
multi-tile changes maybe it can't unroll the loops and calculate the 
masks at compile time. Maybe it is a bit too much and we should drop the 
__builtin_constant_p branch? Probably.. But I guess it is safe to use 
GEM_WARN_ON_ONCE instead of WARN_ON_ONCE since there are no external 
callers (nothing coming from event) now. That way at least production 
builds don't have to have the check.

Regards,

Tvrtko

> 
> Thanks,
> Umesh
> 
>>
>>> +    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.
>>
>> After addressing the above, this patch is:
>>
>> Reviewed-by: Ashutosh Dixit <[e-mail]>
>>
>>> +
>>> +    return BIT(config_bit(config));
>>>  }
>>>
>>>  static bool is_engine_event(struct perf_event *event)
>>> @@ -633,11 +644,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 +661,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 +708,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 +744,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.36.1
>>>

Dixit, Ashutosh May 17, 2023, 4:25 p.m. UTC | #4

On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
>
>
> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
> > On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
> >> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
> >>>
> >>
> >> Hi Umesh/Tvrtko,
> >>
> >> Mostly repeating comments/questions made on the previous patch below.
>
> First of all thanks for improving this, my v1 obviously wasn't good enough.
>
> >>
> >>> From: Tvrtko Ursulin <[e-mail]>
> >>>
> >>> 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.
> >>>
> >>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
> >>>
> >>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >>> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
> >>> Cc: Ashutosh Dixit <[e-mail]>
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
> >>>  1 file changed, 18 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>> index 7ece883a7d95..96543dce2db1 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);
> >>>  }
> >>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
> >>>         return other_bit(config);
> >>>  }
> >>>
> >>> -static u64 config_mask(u64 config)
> >>> +static u32 config_mask(const u64 config)
> >>>  {
> >>> -    return BIT_ULL(config_bit(config));
> >>> +    unsigned int bit = config_bit(config);
> >>
> >> Give that config_bit() can return -1 (I understand it is avoided in
> >> moving
> >> the code to config_mask from config_bit), maybe the code below should
> >> also
> >> have that check?
> >
> > config_mask is only called to check frequency related events in the code,
> > so I don't see it returing -1 here.
>
> Yeah that should be fine since -1 would make the below asserts fire
> anyway. (If it would get called from a different path in the future.)
>
> >>
> >>     int bit = config_bit(config);
> >>
> >>     if (bit != -1)
> >>     {
> >>         ...
> >>     }
> >>
> >> Though as mentioned below the 'if (__builtin_constant_p())' would have to
> >> go. Maybe the code could even have stayed in config_bit with the check.
> >>
> >>> +
> >>> +    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?
> >
> > Not sure about earlier code where these checks were inside config_bit(),
> > but with changes I made, I don't see this being a builtin
> > constant. However, nothing prevents a caller from just passing a
> > builtin_constant to this in future.
>
> Are you sure? I would have thought it would always be a compile time
> constant now that the check is in config_mask. Aahhh.. with the multi-tile
> changes maybe it can't unroll the loops and calculate the masks at compile
> time. Maybe it is a bit too much and we should drop the
> __builtin_constant_p branch? Probably..

Ah yes, with the code move to config_mask, they really all are compile time
constants (provided compiler can unroll the loops) so at least that is the
justfication for leaving the __builtin_constant_p in. So I'd probably just
leave it as is (though it is a bit too much).

> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
> since there are no external callers (nothing coming from event) now. That
> way at least production builds don't have to have the check.

Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
I guess.

So I'm ok with the code staying as is. Enough bike-shed on this already.

Thanks.
--
Ashutosh


>
> Regards,
>
> Tvrtko
>
> >
> > Thanks,
> > Umesh
> >
> >>
> >>> +    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.
> >>
> >> After addressing the above, this patch is:
> >>
> >> Reviewed-by: Ashutosh Dixit <[e-mail]>
> >>
> >>> +
> >>> +    return BIT(config_bit(config));
> >>>  }
> >>>
> >>>  static bool is_engine_event(struct perf_event *event)
> >>> @@ -633,11 +644,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 +661,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 +708,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 +744,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.36.1
> >>>

Umesh Nerlige Ramappa May 17, 2023, 8:15 p.m. UTC | #5

On Wed, May 17, 2023 at 09:25:03AM -0700, Dixit, Ashutosh wrote:
>On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
>>
>>
>> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
>> > On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>> >> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>> >>>
>> >>
>> >> Hi Umesh/Tvrtko,
>> >>
>> >> Mostly repeating comments/questions made on the previous patch below.
>>
>> First of all thanks for improving this, my v1 obviously wasn't good enough.
>>
>> >>
>> >>> From: Tvrtko Ursulin <[e-mail]>
>> >>>
>> >>> 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.
>> >>>
>> >>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>> >>>
>> >>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>> >>> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
>> >>> Cc: Ashutosh Dixit <[e-mail]>
>> >>> ---
>> >>>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>> >>>  1 file changed, 18 insertions(+), 8 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
>> >>> b/drivers/gpu/drm/i915/i915_pmu.c
>> >>> index 7ece883a7d95..96543dce2db1 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);
>> >>>  }
>> >>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>> >>>         return other_bit(config);
>> >>>  }
>> >>>
>> >>> -static u64 config_mask(u64 config)
>> >>> +static u32 config_mask(const u64 config)
>> >>>  {
>> >>> -    return BIT_ULL(config_bit(config));
>> >>> +    unsigned int bit = config_bit(config);
>> >>
>> >> Give that config_bit() can return -1 (I understand it is avoided in
>> >> moving
>> >> the code to config_mask from config_bit), maybe the code below should
>> >> also
>> >> have that check?
>> >
>> > config_mask is only called to check frequency related events in the code,
>> > so I don't see it returing -1 here.
>>
>> Yeah that should be fine since -1 would make the below asserts fire
>> anyway. (If it would get called from a different path in the future.)
>>
>> >>
>> >>     int bit = config_bit(config);
>> >>
>> >>     if (bit != -1)
>> >>     {
>> >>         ...
>> >>     }
>> >>
>> >> Though as mentioned below the 'if (__builtin_constant_p())' would have to
>> >> go. Maybe the code could even have stayed in config_bit with the check.
>> >>
>> >>> +
>> >>> +    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?
>> >
>> > Not sure about earlier code where these checks were inside config_bit(),
>> > but with changes I made, I don't see this being a builtin
>> > constant. However, nothing prevents a caller from just passing a
>> > builtin_constant to this in future.
>>
>> Are you sure? I would have thought it would always be a compile time
>> constant now that the check is in config_mask. Aahhh.. with the multi-tile
>> changes maybe it can't unroll the loops and calculate the masks at compile
>> time. Maybe it is a bit too much and we should drop the
>> __builtin_constant_p branch? Probably..
>
>Ah yes, with the code move to config_mask, they really all are compile time
>constants (provided compiler can unroll the loops) so at least that is the
>justfication for leaving the __builtin_constant_p in. So I'd probably just
>leave it as is (though it is a bit too much).
>
>> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
>> since there are no external callers (nothing coming from event) now. That
>> way at least production builds don't have to have the check.
>
>Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
>I guess.
>
>So I'm ok with the code staying as is. Enough bike-shed on this already.

Leaving it as is. @Ashutosh, okay to use your R-b without any changes to 
this patch?

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh
>
>
>>
>> Regards,
>>
>> Tvrtko
>>
>> >
>> > Thanks,
>> > Umesh
>> >
>> >>
>> >>> +    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.
>> >>
>> >> After addressing the above, this patch is:
>> >>
>> >> Reviewed-by: Ashutosh Dixit <[e-mail]>
>> >>
>> >>> +
>> >>> +    return BIT(config_bit(config));
>> >>>  }
>> >>>
>> >>>  static bool is_engine_event(struct perf_event *event)
>> >>> @@ -633,11 +644,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 +661,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 +708,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 +744,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.36.1
>> >>>

Dixit, Ashutosh May 17, 2023, 8:15 p.m. UTC | #6

On Wed, 17 May 2023 13:15:14 -0700, Umesh Nerlige Ramappa wrote:
>
> Leaving it as is. @Ashutosh, okay to use your R-b without any changes to
> this patch?

Yes.

Reviewed-by: Ashutosh Dixit <[e-mail]>

Tvrtko Ursulin May 18, 2023, 9:07 a.m. UTC | #7

On 17/05/2023 17:25, Dixit, Ashutosh wrote:
> On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
>>
>>
>> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
>>> On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
>>>> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>>>>>
>>>>
>>>> Hi Umesh/Tvrtko,
>>>>
>>>> Mostly repeating comments/questions made on the previous patch below.
>>
>> First of all thanks for improving this, my v1 obviously wasn't good enough.
>>
>>>>
>>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>>
>>>>> 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.
>>>>>
>>>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>>>> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
>>>>> Cc: Ashutosh Dixit <[e-mail]>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>>>>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
>>>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>>>> index 7ece883a7d95..96543dce2db1 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);
>>>>>   }
>>>>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>>>>>          return other_bit(config);
>>>>>   }
>>>>>
>>>>> -static u64 config_mask(u64 config)
>>>>> +static u32 config_mask(const u64 config)
>>>>>   {
>>>>> -    return BIT_ULL(config_bit(config));
>>>>> +    unsigned int bit = config_bit(config);
>>>>
>>>> Give that config_bit() can return -1 (I understand it is avoided in
>>>> moving
>>>> the code to config_mask from config_bit), maybe the code below should
>>>> also
>>>> have that check?
>>>
>>> config_mask is only called to check frequency related events in the code,
>>> so I don't see it returing -1 here.
>>
>> Yeah that should be fine since -1 would make the below asserts fire
>> anyway. (If it would get called from a different path in the future.)
>>
>>>>
>>>>      int bit = config_bit(config);
>>>>
>>>>      if (bit != -1)
>>>>      {
>>>>          ...
>>>>      }
>>>>
>>>> Though as mentioned below the 'if (__builtin_constant_p())' would have to
>>>> go. Maybe the code could even have stayed in config_bit with the check.
>>>>
>>>>> +
>>>>> +    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?
>>>
>>> Not sure about earlier code where these checks were inside config_bit(),
>>> but with changes I made, I don't see this being a builtin
>>> constant. However, nothing prevents a caller from just passing a
>>> builtin_constant to this in future.
>>
>> Are you sure? I would have thought it would always be a compile time
>> constant now that the check is in config_mask. Aahhh.. with the multi-tile
>> changes maybe it can't unroll the loops and calculate the masks at compile
>> time. Maybe it is a bit too much and we should drop the
>> __builtin_constant_p branch? Probably..
> 
> Ah yes, with the code move to config_mask, they really all are compile time
> constants (provided compiler can unroll the loops) so at least that is the
> justfication for leaving the __builtin_constant_p in. So I'd probably just
> leave it as is (though it is a bit too much).
> 
>> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
>> since there are no external callers (nothing coming from event) now. That
>> way at least production builds don't have to have the check.
> 
> Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
> I guess.
> 
> So I'm ok with the code staying as is. Enough bike-shed on this already.

Latest series looks fine to me and thanks for your patience. Hope you 
would agree changing that one thing to u32 made more sense than changing 
the other to u64 so bike shed wasn't for nothing.

Regards,

Tvrtko

Dixit, Ashutosh May 19, 2023, 5:02 a.m. UTC | #8

On Thu, 18 May 2023 02:07:55 -0700, Tvrtko Ursulin wrote:
>
> On 17/05/2023 17:25, Dixit, Ashutosh wrote:
> > On Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote:
> >>
> >>
> >> On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote:
> >>> On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote:
> >>>> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
> >>>>>
> >>>>
> >>>> Hi Umesh/Tvrtko,
> >>>>
> >>>> Mostly repeating comments/questions made on the previous patch below.
> >>
> >> First of all thanks for improving this, my v1 obviously wasn't good enough.
> >>
> >>>>
> >>>>> From: Tvrtko Ursulin <[e-mail]>
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
> >>>>>
> >>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >>>>> Signed-off-by: Umesh Nerlige Ramappa <[e-mail]>
> >>>>> Cc: Ashutosh Dixit <[e-mail]>
> >>>>> ---
> >>>>>   drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
> >>>>>   1 file changed, 18 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> b/drivers/gpu/drm/i915/i915_pmu.c
> >>>>> index 7ece883a7d95..96543dce2db1 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);
> >>>>>   }
> >>>>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
> >>>>>          return other_bit(config);
> >>>>>   }
> >>>>>
> >>>>> -static u64 config_mask(u64 config)
> >>>>> +static u32 config_mask(const u64 config)
> >>>>>   {
> >>>>> -    return BIT_ULL(config_bit(config));
> >>>>> +    unsigned int bit = config_bit(config);
> >>>>
> >>>> Give that config_bit() can return -1 (I understand it is avoided in
> >>>> moving
> >>>> the code to config_mask from config_bit), maybe the code below should
> >>>> also
> >>>> have that check?
> >>>
> >>> config_mask is only called to check frequency related events in the code,
> >>> so I don't see it returing -1 here.
> >>
> >> Yeah that should be fine since -1 would make the below asserts fire
> >> anyway. (If it would get called from a different path in the future.)
> >>
> >>>>
> >>>>      int bit = config_bit(config);
> >>>>
> >>>>      if (bit != -1)
> >>>>      {
> >>>>          ...
> >>>>      }
> >>>>
> >>>> Though as mentioned below the 'if (__builtin_constant_p())' would have to
> >>>> go. Maybe the code could even have stayed in config_bit with the check.
> >>>>
> >>>>> +
> >>>>> +    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?
> >>>
> >>> Not sure about earlier code where these checks were inside config_bit(),
> >>> but with changes I made, I don't see this being a builtin
> >>> constant. However, nothing prevents a caller from just passing a
> >>> builtin_constant to this in future.
> >>
> >> Are you sure? I would have thought it would always be a compile time
> >> constant now that the check is in config_mask. Aahhh.. with the multi-tile
> >> changes maybe it can't unroll the loops and calculate the masks at compile
> >> time. Maybe it is a bit too much and we should drop the
> >> __builtin_constant_p branch? Probably..
> >
> > Ah yes, with the code move to config_mask, they really all are compile time
> > constants (provided compiler can unroll the loops) so at least that is the
> > justfication for leaving the __builtin_constant_p in. So I'd probably just
> > leave it as is (though it is a bit too much).
> >
> >> But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE
> >> since there are no external callers (nothing coming from event) now. That
> >> way at least production builds don't have to have the check.
> >
> > Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too
> > I guess.
> >
> > So I'm ok with the code staying as is. Enough bike-shed on this already.
>
> Latest series looks fine to me and thanks for your patience. Hope you would
> agree changing that one thing to u32 made more sense than changing the
> other to u64 so bike shed wasn't for nothing.

Hi Tvrtko, yes definitely, no issues :)

Thanks for your patience too.
--
Ashutosh

Patch

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..96543dce2db1 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);
 }
@@ -88,9 +88,20 @@  static unsigned int config_bit(const u64 config)
 		return other_bit(config);
 }
 
-static u64 config_mask(u64 config)
+static u32 config_mask(const u64 config)
 {
-	return BIT_ULL(config_bit(config));
+	unsigned int bit = config_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(config_bit(config));
 }
 
 static bool is_engine_event(struct perf_event *event)
@@ -633,11 +644,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 +661,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 +708,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 +744,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);
 	}