Hi John,
On Wed, Oct 16, 2019 at 7:01 PM John Garry <john.garry@xxxxxxxxxx> wrote:
>
>
> > +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?
>
At the moment no, since the number of events exposed/listed are very few.
> 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
>
>
Thanks,
Ganapat
