Re: [PATCH v2 5/5] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Robin Murphy]

On 2023-06-02 08:13, Ilkka Koskinen wrote:
> Hi Robin,
>
> On Thu, 1 Jun 2023, Robin Murphy wrote:
> > On 2023-06-01 04:01, Ilkka Koskinen wrote:
> > [...]
> > > +static bool ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
> > > +                    struct perf_event *new)
> > > +{
> > > +    struct perf_event *curr;
> > > +    unsigned int idx;
> > > +    u32 threshold = 0, rank = 0, bank = 0;
> > > +
> > > +    /* We compare the global filter settings to existing events */
> > > +    idx = find_first_bit(cspmu->hw_events.used_ctrs,
> > > +                 cspmu->cycle_counter_logical_idx);
> > > +
> > > +    /* This is the first event */
> > > +    if (idx == cspmu->cycle_counter_logical_idx)
> > > +        return true;
> > > +
> > > +    curr = cspmu->hw_events.events[idx];
> > > +
> > > +    if (get_filter_enable(new)) {
> > > +        threshold    = get_threshold(new);
> > > +        rank        = get_rank(new);
> > > +        bank        = get_bank(new);
> > > +    }
> > > +
> > > +    if (get_filter_enable(new) != get_filter_enable(curr) ||
> >
> > Is there any useful purpose in allowing the user to specify nonzero 
> > rank, bank or threshold values with filter_enable=0? Assuming not, 
> > then between this and ampere_cspmu_set_ev_filter() it appears that you 
> > don't need filter_enable at all.
>
> Not really. I dropped filter_enable at one point but restored it later 
> to match the smmuv3 pmu driver. I totally agree, it's unnecessary and 
> it's better to remove it completely.

Ah, I see - the SMMU PMCG driver needs that to differentiate between 
"filter values defaulted to 0 because user didn't ask for filtering" and 
"user asked to filter an exact match on StreamID 0", since it's 
impractical to expect userspace tools to understand and manually set the 
all-ones mask value to indicate that filtering wasn't requested. In your 
case, though, since values of 0 appear to mean "no filter", it should 
just work as expected without needing any additional complexity. Ideally 
your interface should reflect the functionality and expected usage model 
of your PMU hardware in the way that's most intuitive and helpful for 
the user - it doesn't need to be influenced by other PMUs that work 
differently.

Thanks,
Robin.