RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

On Thu, 22 Jun 2023, Besar Wicaksono wrote:

quoted

-----Original Message-----
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Sent: Thursday, June 22, 2023 3:34 PM
To: Ilkka Koskinen <redacted>
Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
Besar Wicaksono [off-list ref]; Suzuki K Poulose
[off-list ref]; Mark Rutland [off-list ref]; linux-
arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
filters

External email: Use caution opening links or attachments


On Wed, 21 Jun 2023 18:11:39 -0700
Ilkka Koskinen [off-list ref] wrote:

quoted

Generic filters aren't used in all the platforms. Instead, the platforms
may use different means to filter events. Add support for implementation
specific filters.

If the specification allows explicitly for non standard ways of controlling filters
it would be good to add a specification reference to this.

Want to point out that the spec considers PMEVTYPER and PMEVFILTR* registers as optional,
please refer to section 2.1 in the spec. The spec also defines PMIMPDEF registers (starting
from offset 0xD80), which is intended for implementation defined extension. My interpretation
to this is implementer can have other methods to configure event selection and filtering, although
maybe not clear of how much freedom is given to the implementer.

That's a good idea. I do that.

Cheers, Ilkka

quoted

Otherwise one question inline.

quoted

Reviewed-by: Besar Wicaksono <redacted>
Signed-off-by: Ilkka Koskinen <redacted>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
 drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c

b/drivers/perf/arm_cspmu/arm_cspmu.c

quoted

index 0f517152cb4e..fafd734c3218 100644

--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c

@@ -117,6 +117,9 @@

 static unsigned long arm_cspmu_cpuhp_state;

+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+                                 struct hw_perf_event *hwc, u32 filter);
+
 static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
 {
      return *(struct acpi_apmt_node **)dev_get_platdata(dev);

@@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct

arm_cspmu *cspmu)

quoted

      CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
      CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
      CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
+     CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);

      return 0;
 }

@@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct

arm_cspmu *cspmu,

quoted

      writel(hwc->config, cspmu->base0 + offset);
 }

-static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
                                         struct hw_perf_event *hwc,
                                         u32 filter)
 {

@@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event

*event, int pmu_flags)

quoted

              arm_cspmu_set_cc_filter(cspmu, filter);
      } else {
              arm_cspmu_set_event(cspmu, hwc);
-             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
+             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);

Optional callback so don't you need either provide a default, or check
it isn't null?

Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallback
to default if ops.set_ev_filter is null.

Regards,
Besar

quoted

quoted

      }

      hwc->state = 0;

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h

b/drivers/perf/arm_cspmu/arm_cspmu.h

quoted

index 83df53d1c132..d6d88c246a23 100644

--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h

@@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
      u32 (*event_type)(const struct perf_event *event);
      /* Decode filter value from configs */
      u32 (*event_filter)(const struct perf_event *event);
+     /* Set event filter */
+     void (*set_ev_filter)(struct arm_cspmu *cspmu,
+                           struct hw_perf_event *hwc, u32 filter);
      /* Hide/show unsupported events */
      umode_t (*event_attr_is_visible)(struct kobject *kobj,
                                       struct attribute *attr, int unused);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel