Re: [PATCH 1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-07-14 09:17, Jiucheng Xu wrote:
[...]
+        list_for_each_entry((sibling), &(event)->sibling_list,
+                    sibling_list) {
+            if (sibling->pmu != event->pmu &&
+                !is_software_event(sibling))
+                return -EINVAL;

It looks like you don't have multiple sets of hardware counters; if so you'd also need to reject the group if it contains more than one event for this PMU.
I have 5 HW counters and 5 relevant events. Do you mean I need reject the group?

Ah, I think I understand a bit better now, so each channel is a separate counter - in that case you need to check that the group doesn't contain more than one event targeting the same channel. The reason is that event_add should return an error when another event is already using the given counter - when that happens, perf will try to schedule different combinations of events to work out which ones can count simultaneously and which need to be multiplexed. However if two conflicting events are in the same group, and thus required to be scheduled together atomically, then it ends up in an infinite loop trying to achieve the impossible. Therefore event_init must reject any group whose events would never be able to be scheduled together.

[...]
+static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+    struct ddr_pmu *pmu = hlist_entry_safe(node, struct ddr_pmu, node);
+    int target;
+
+    if (cpu != pmu->cpu)
+        return 0;
+
+    target = cpumask_any_but(cpu_online_mask, cpu);
+    if (target >= nr_cpu_ids)
+        return 0;
+
+    perf_pmu_migrate_context(&pmu->pmu, cpu, target);
+    pmu->cpu = target;
+
+    WARN_ON(irq_set_affinity_hint(pmu->info.irq_num, cpumask_of(pmu->cpu)));

This is wrong, it needs to be irq_set_affinity().

Okay, Could you please tell what is the major difference between the two API?

The affinity hint is just a field exposed to userspace, for the benefit of tools like irqbalance, it doesn't actually serve any purpose in the kernel itself.

Historically, it used to be the case that irq_set_affinity_hint() also happened to call irq_set_affinity() internally, and this anti-pattern developed around that due to module exports, but that has all been cleaned up now.

[...]
+        goto err2;
+    }
+
+    irq_name = of_get_property(node, "interrupt-names", NULL);
+    if (!irq_name)
+        irq_name = "ddr_pmu";

That's not how the "interrupt-names" property works. If you only have a single interrupt then there's not much need for it to be named in the DT at all. If you do want to use named interrupts then use platform_get_irq_byname(), and the name should probably have a bnit more functional meaning. Either way, please don't abuse the DT like this.
Okay, actually there will be multiple interrupts , but not in current G12 series.

That's fair enough, so we should try to anticipate it in the design of the DT binding. If for instance future SoCs are going to move from having a single combined overflow interrupt to a separate interrupt for each counter, then the driver can reasonably continue to get them by index and we'll effectively only need to update maxItems in the binding. If on the other hand there's still going to be one combined overflow interrupt, plus some other new interrupt for something completely different, then it *could* be more appropriate to have names, and thus to define and use a standard "overflow" name from the beginning even when it is the only one present, so that we can remain consistent later once more are added.

+
+    ret = request_irq(info->irq_num, dmc_irq_handler,
+              IRQF_SHARED, irq_name, (void *)info);

Who else is sharing the IRQ? If it's other instances of this PMU then that's still manageable under the normal paradigm, the driver just needs to coordinate affinity chanmges between all instances. If it's random other devices, then maybe it's time to reason about how system PMUs could use proper IRQ-safe locking and abandon the affinity stuff, since this seems to be coming up more and more.

The IRQ is private. I will change it.

OK, that's good. In that case you can simply replace IRQF_SHARED with IRQF_NOBALANCING here, since that is needed either way to prevent userspace changing affinities behind our back.

[...]
+static void append_attr_list(struct attribute *attr)
+{
+    int i;
+
+    for (i = 0; g12_pmu_format_attrs[i] && i < 255; i++)
+        ;

Eww, what? :(

+
+    g12_pmu_format_attrs[i] = attr;
+
+    g12_pmu_format_attrs[i + 1] = NULL;

(that's pointless either way)

OK, I think I see what's going on here now. Dynamically patching the attribute arrays is pretty grim - it's far cleaner and more sustainable to statically define the whole array with all the possible attributes, then use .is_visible to hide the ones which aren't relevant to the current system.

I have not got your point yet. I have no idea how to use .is_visible to hide the irrelevant attribute. I need a little time to think it.

It might be easiest to follow an example like arm_dsu_pmu - in that case, the hardware has an ID register that says which event IDs are supported; you could have some kind of static bitmap/lookup table/function to encode the equivalent data of which AXI IDs are relevant to which PMU model(s). Another approach is to encode the supported models directly in each attribute itself and thus make the lookup the other way round - this is probably more complex to implement, and I hesitate to suggest looking at arm-cmn as an example since that's rather extreme and hard to follow, but it is another possibility.

A third option might be to push all the AXI ID data out to JSON in the userspace perf tool, like fsl_imx8_ddr_perf seems to do, however that might make it less accessible to other userspace tools, so it probably depends on your expected use-cases whether that's worth considering or not.

[...]
+    struct device_node *node = pdev->dev.of_node;
+    const char *model;
+
+    if (of_property_read_string(node, "model", &model))
+        return -EINVAL;

No, use of_device_is_compatible(), and define the binding properly.

Why the "model" property couldn't be used? Do you mean use existing property rather than creating new one?

Yes, as I suggested on patch #4, the standard practice would be to have hierarchical compatible strings in the DT like so:

	compatible = "amlogic,g12b-ddr-pmu", "amlogic,g12-ddr-pmu";

then your of_device_id table can still match on the generic "amlogic,g12-ddr-pmu" string to bind the driver, and where necessary you can then do:

	if (of_device_is_compatible(node, "amlogic,g12b-ddr-pmu"))
		/* do G12B-specific stuff */

Alternatively, you can put all the SoC-specific compatibles directly in the of_device_id table and use the .data member to associate your SoC-specific identifier or static data which you can then retrieve with device_get_match_data(). Both approaches are commonly used.

+    dev_info(&pdev->dev, "%s", model);
+
+    if (strcmp(model, "g12a") == 0) {
+        dev_info(&pdev->dev, "ddr pmu for g12a\n");
+    } else if (strcmp(model, "g12b") == 0) {
+        dev_info(&pdev->dev, "ddr pmu for g12b\n");
+
+        append_attr_list(&format_attr_nna.attr);
+        append_attr_list(&format_attr_gdc.attr);
+        append_attr_list(&format_attr_arm1.attr);
+        append_attr_list(&format_attr_mipi_isp.attr);
+        append_attr_list(&format_attr_sd_emmc_a.attr);
+    } else if (strcmp(model, "sm1") == 0) {
+        dev_info(&pdev->dev, "ddr pmu for sm1\n");
+
+        append_attr_list(&format_attr_nna.attr);
+    }
+#endif
+    return aml_ddr_pmu_create(pdev, &g12_ops, g12_pmu_format_attrs);
+}
+
+static int  __exit g12_ddr_pmu_remove(struct platform_device *pdev)
+{
+    aml_ddr_pmu_remove(pdev);
+
+    return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id aml_ddr_pmu_dt_match[] = {
+    {
+        .compatible = "amlogic,g12-ddr-pmu",
+    },
+    {}
+};
+#endif
+
+static struct platform_driver g12_ddr_pmu_driver = {
+    .driver = {
+        .name = "amlogic,ddr-pmu",
+        .owner = THIS_MODULE,

The driver core sets this automatically.

+    #ifdef CONFIG_OF
+        .of_match_table = aml_ddr_pmu_dt_match,
+    #endif
+    },
+    .remove = g12_ddr_pmu_remove,
+};
+
+static int __init aml_g12_ddr_pmu_init(void)
+{
+    return platform_driver_probe(&g12_ddr_pmu_driver, g12_ddr_pmu_probe);
+}
+
+static void __exit aml_g12_ddr_pmu_exit(void)
+{
+    platform_driver_unregister(&g12_ddr_pmu_driver);
+}
+
+module_init(aml_g12_ddr_pmu_init);
+module_exit(aml_g12_ddr_pmu_exit);

Use module_platform_driver_probe() (if of course you really think the __init shenanigans are beneficial, otherwise just use regular module_platform_driver() for even less surprise).

Thanks,
Robin.


Thanks for your time, Robin! TBH I'm a little nervous since this is my first submitting to upstream. Your comments are great and helpful. I will update the driver.

No worries, this is actually pretty good for a first submission. And the perf APIs in particular do have some horribly subtle aspects which took me a long time to fully understand too :)

Thanks,
Robin.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux