Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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 <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.
> >>>>>
> >>>>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
> >>>>>
> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >>>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>>>> ---
> >>>>>   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