Re: [PATCH v6 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.

[John Garry <john.garry@xxxxxxxxxx>]

> +TX2_EVENT_ATTR(req_pktsent, CCPI2_EVENT_REQ_PKT_SENT);
> +TX2_EVENT_ATTR(snoop_pktsent, CCPI2_EVENT_SNOOP_PKT_SENT);
> +TX2_EVENT_ATTR(data_pktsent, CCPI2_EVENT_DATA_PKT_SENT);
> +TX2_EVENT_ATTR(gic_pktsent, CCPI2_EVENT_GIC_PKT_SENT);
> +
> +static struct attribute *ccpi2_pmu_events_attrs[] = {
> +	&tx2_pmu_event_attr_req_pktsent.attr.attr,
> +	&tx2_pmu_event_attr_snoop_pktsent.attr.attr,
> +	&tx2_pmu_event_attr_data_pktsent.attr.attr,
> +	&tx2_pmu_event_attr_gic_pktsent.attr.attr,
> +	NULL,
> +};

Hi Ganapatrao,

Have you considered adding these as uncore pmu-events in the perf tool?

Some advantages I see:
- perf list is not swamped with all these uncore events per PMU
For the Hisi uncore events, we get 100s of events (>600 on the board I 
just tested, which is crazy)
- can add more description in the JSON files
- less stuff in the kernel

> +
>  static const struct attribute_group l3c_pmu_events_attr_group = {
>  	.name = "events",
>  	.attrs = l3c_pmu_events_attrs,
> @@ -174,6 +240,11 @@ static const struct attribute_group dmc_pmu_events_attr_group = {
>  	.attrs = dmc_pmu_events_attrs,
>  };

[...]

>  		tx2_pmu->attr_groups = l3c_pmu_attr_groups;
>  		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
>  				"uncore_l3c_%d", tx2_pmu->node);
> @@ -665,10 +846,13 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
>  		tx2_pmu->stop_event = uncore_stop_event_l3c;
>  		break;
>  	case PMU_TYPE_DMC:
> -		tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
> +		tx2_pmu->max_counters = TX2_PMU_DMC_L3C_MAX_COUNTERS;
> +		tx2_pmu->counters_mask = 0x3;
>  		tx2_pmu->prorate_factor = TX2_PMU_DMC_CHANNELS;
>  		tx2_pmu->max_events = DMC_EVENT_MAX;
> +		tx2_pmu->events_mask = 0x1f;
>  		tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
> +		tx2_pmu->hrtimer_callback = tx2_hrtimer_callback;
>  		tx2_pmu->attr_groups = dmc_pmu_attr_groups;
>  		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
>  				"uncore_dmc_%d", tx2_pmu->node);
> @@ -676,6 +860,21 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
>  		tx2_pmu->start_event = uncore_start_event_dmc;
>  		tx2_pmu->stop_event = uncore_stop_event_dmc;
>  		break;
> +	case PMU_TYPE_CCPI2:
> +		/* CCPI2 has 8 counters */
> +		tx2_pmu->max_counters = TX2_PMU_CCPI2_MAX_COUNTERS;
> +		tx2_pmu->counters_mask = 0x7;
> +		tx2_pmu->prorate_factor = 1;
> +		tx2_pmu->max_events = CCPI2_EVENT_MAX;
> +		tx2_pmu->events_mask = 0x1ff;
> +		tx2_pmu->attr_groups = ccpi2_pmu_attr_groups;
> +		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
> +				"uncore_ccpi2_%d", tx2_pmu->node);

Do you need to check this for name == NULL?

> +		tx2_pmu->init_cntr_base = init_cntr_base_ccpi2;
> +		tx2_pmu->start_event = uncore_start_event_ccpi2;
> +		tx2_pmu->stop_event = uncore_stop_event_ccpi2;
> +		tx2_pmu->hrtimer_callback = NULL;
> +		break;
>  	case PMU_TYPE_INVALID:
>  		devm_kfree(dev, tx2_pmu);
>  		return NULL;
> @@ -744,7 +943,9 @@ static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
>  	if (cpu != tx2_pmu->cpu)
>  		return 0;
>
> -	hrtimer_cancel(&tx2_pmu->hrtimer);
> +	if (tx2_pmu->hrtimer_callback)
> +		hrtimer_cancel(&tx2_pmu->hrtimer);
> +
>  	cpumask_copy(&cpu_online_mask_temp, cpu_online_mask);
>  	cpumask_clear_cpu(cpu, &cpu_online_mask_temp);
>  	new_cpu = cpumask_any_and(

Thanks,
John