| Message ID | 20230601030144.3458136-5-ilkka@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: ampere: Add support for Ampere SoC PMUs | expand |
On 2023-06-01 04:01, Ilkka Koskinen wrote: > Some platforms may use e.g. different filtering mechanism and, thus, > may need different way to validate the events. > > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > --- > drivers/perf/arm_cspmu/arm_cspmu.c | 4 ++++ > drivers/perf/arm_cspmu/arm_cspmu.h | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c > index b4c4ef81c719..a26f484e06b1 100644 > --- a/drivers/perf/arm_cspmu/arm_cspmu.c > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c > @@ -593,6 +593,10 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events, > if (idx >= cspmu->num_logical_ctrs) > return -EAGAIN; > > + if (cspmu->impl.ops.validate_event && > + !cspmu->impl.ops.validate_event(cspmu, event)) > + return -EAGAIN; Seems like this should be -EINVAL, or maybe the callback should return int so it can make its own distinction (yes, I know the outer logic doesn't actually propagate it, but there's no reason that couldn't improve at some point as well). Another thought is that once we get into imp-def conditions for whether an event is valid in itself, we presumably also need to consider imp-def conditions for whether a given pair of events are compatible to be grouped? Thanks, Robin. > + > set_bit(idx, hw_events->used_ctrs); > > return idx; > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h > index 4a29b921f7e8..0e5c316c96f9 100644 > --- a/drivers/perf/arm_cspmu/arm_cspmu.h > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h > @@ -106,6 +106,8 @@ struct arm_cspmu_impl_ops { > void (*set_ev_filter)(struct arm_cspmu *cspmu, > struct hw_perf_event *hwc, > u32 filter); > + /* Implementation specific event validation */ > + bool (*validate_event)(struct arm_cspmu *cspmu, struct perf_event *new); > /* Hide/show unsupported events */ > umode_t (*event_attr_is_visible)(struct kobject *kobj, > struct attribute *attr, int unused);
Hi Robin, On Thu, 1 Jun 2023, Robin Murphy wrote: > On 2023-06-01 04:01, Ilkka Koskinen wrote: >> Some platforms may use e.g. different filtering mechanism and, thus, >> may need different way to validate the events. >> >> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> >> --- >> drivers/perf/arm_cspmu/arm_cspmu.c | 4 ++++ >> drivers/perf/arm_cspmu/arm_cspmu.h | 2 ++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c >> b/drivers/perf/arm_cspmu/arm_cspmu.c >> index b4c4ef81c719..a26f484e06b1 100644 >> --- a/drivers/perf/arm_cspmu/arm_cspmu.c >> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c >> @@ -593,6 +593,10 @@ static int arm_cspmu_get_event_idx(struct >> arm_cspmu_hw_events *hw_events, >> if (idx >= cspmu->num_logical_ctrs) >> return -EAGAIN; >> + if (cspmu->impl.ops.validate_event && >> + !cspmu->impl.ops.validate_event(cspmu, event)) >> + return -EAGAIN; > > Seems like this should be -EINVAL, or maybe the callback should return int so > it can make its own distinction (yes, I know the outer logic doesn't actually > propagate it, but there's no reason that couldn't improve at some point as > well). Makes sense to me. > Another thought is that once we get into imp-def conditions for whether an > event is valid in itself, we presumably also need to consider imp-def > conditions for whether a given pair of events are compatible to be grouped? That's a good point. I'll take a look at it. Cheers, Ilkka > > Thanks, > Robin. > >> + >> set_bit(idx, hw_events->used_ctrs); >> return idx; >> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h >> b/drivers/perf/arm_cspmu/arm_cspmu.h >> index 4a29b921f7e8..0e5c316c96f9 100644 >> --- a/drivers/perf/arm_cspmu/arm_cspmu.h >> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h >> @@ -106,6 +106,8 @@ struct arm_cspmu_impl_ops { >> void (*set_ev_filter)(struct arm_cspmu *cspmu, >> struct hw_perf_event *hwc, >> u32 filter); >> + /* Implementation specific event validation */ >> + bool (*validate_event)(struct arm_cspmu *cspmu, struct perf_event >> *new); >> /* Hide/show unsupported events */ >> umode_t (*event_attr_is_visible)(struct kobject *kobj, >> struct attribute *attr, int unused); >
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c index b4c4ef81c719..a26f484e06b1 100644 --- a/drivers/perf/arm_cspmu/arm_cspmu.c +++ b/drivers/perf/arm_cspmu/arm_cspmu.c @@ -593,6 +593,10 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events, if (idx >= cspmu->num_logical_ctrs) return -EAGAIN; + if (cspmu->impl.ops.validate_event && + !cspmu->impl.ops.validate_event(cspmu, event)) + return -EAGAIN; + set_bit(idx, hw_events->used_ctrs); return idx; diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h index 4a29b921f7e8..0e5c316c96f9 100644 --- a/drivers/perf/arm_cspmu/arm_cspmu.h +++ b/drivers/perf/arm_cspmu/arm_cspmu.h @@ -106,6 +106,8 @@ struct arm_cspmu_impl_ops { void (*set_ev_filter)(struct arm_cspmu *cspmu, struct hw_perf_event *hwc, u32 filter); + /* Implementation specific event validation */ + bool (*validate_event)(struct arm_cspmu *cspmu, struct perf_event *new); /* Hide/show unsupported events */ umode_t (*event_attr_is_visible)(struct kobject *kobj, struct attribute *attr, int unused);
Some platforms may use e.g. different filtering mechanism and, thus, may need different way to validate the events. Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> --- drivers/perf/arm_cspmu/arm_cspmu.c | 4 ++++ drivers/perf/arm_cspmu/arm_cspmu.h | 2 ++ 2 files changed, 6 insertions(+)