diff mbox series

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

Message ID 20230622011141.328029-3-ilkka@os.amperecomputing.com
State New
Headers
Series perf: ampere: Add support for Ampere SoC PMUs |

Commit Message

Ilkka Koskinen June 22, 2023, 1:11 a.m. UTC 
  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.

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

Comments

Jonathan Cameron June 22, 2023, 8:33 a.m. UTC | #1 
On Wed, 21 Jun 2023 18:11:39 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

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

Otherwise one question inline.
> 
> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>  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
> 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)
>  	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,
>  	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)
>  		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?

>  	}
>  
>  	hwc->state = 0;
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> 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);
Besar Wicaksono June 22, 2023, 10:14 a.m. UTC | #2 
> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Thursday, June 22, 2023 3:34 PM
> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; 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 <ilkka@os.amperecomputing.com> wrote:
> 
> > 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. 

> Otherwise one question inline.
> >
> > Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> > ---
> >  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
> > 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)
> >       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,
> >       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)
> >               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

> >       }
> >
> >       hwc->state = 0;
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
> b/drivers/perf/arm_cspmu/arm_cspmu.h
> > 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);
Ilkka Koskinen June 22, 2023, 11:23 p.m. UTC | #3 
On Thu, 22 Jun 2023, Besar Wicaksono wrote:

>> -----Original Message-----
>> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> Sent: Thursday, June 22, 2023 3:34 PM
>> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
>> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; 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 <ilkka@os.amperecomputing.com> wrote:
>>
>>> 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

>
>> Otherwise one question inline.
>>>
>>> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>>  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
>>> 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)
>>>       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,
>>>       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)
>>>               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
>
>>>       }
>>>
>>>       hwc->state = 0;
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
>> b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> 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);
>
>
diff mbox series

Patch

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
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)
 	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,
 	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)
 		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);
 	}
 
 	hwc->state = 0;
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
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);