Hi Mike,

Thanks for the reply!
I tried the patch set on hikey960 board. It works well. And I can change strobe parameters in sysfs.
I have two feedbacks:
1. There is an unbalanced lock in cscfg_load_config_sets() in coresight-syscfg.c. If cscfg_load_feat() or cscfg_load_config() returns an error, the function returns without mutex_unlock.
2. I think it's better to show more info of preset configs in cscfg_device in sysfs. It helps userspace tools know which preset to select.

Thanks,
Yabin


On Tue, Sep 15, 2020 at 1:49 AM Mike Leach <mike.leach@linaro.org> wrote:
Hi Yabin,

Sorry for the delay - your e-mail arrived while I was on holiday & I
have been playing catch up since getting back.


On Mon, 14 Sep 2020 at 18:16, Yabin Cui <yabinc@google.com> wrote:
>
> ping?
>
> On Thu, Sep 3, 2020 at 12:25 PM Yabin Cui <yabinc@google.com> wrote:
>>
>> Hi Mike,
>>
>> I am from the Android toolchain team. We are interested in using the ETM strobe feature in Android kernels.
>> We noticed your patch "coresight: etmv4: Adds initial complex config with ETM4 strobe feature." in https://lists.linaro.org/pipermail/coresight/2020-June/004097.html.
>> We tested it on Pixel devices with Qualcomm chips. It works pretty well.
>> And I have a few questions:
>>
>> 1. The param setting of window and period seems doesn't take effect. Not sure if it is a bug or something I misunderstand.
>>
>>    The only place I found moving feat->params to feat->regs is in cs_cfg_reset_feat(). But that function will also reset feat->params to default values in feat_desc->params.
>>    So if I write a value to /sys/.../window, it goes to feat->params, but can't change value in feat->regs.
>>    I tried a work-around by changing cs_cfg_param_store():
>>      static ssize_t cs_cfg_param_store(struct device *dev, {
>>          ...
>>          spin_lock(param->feat->dev_spinlock);
>>          if (!kstrtoull(buf, 0, &val)) {
>>                  param->current_value = val;
>>                  for (i = 0; i < feat->nr_regs; i++) {
>>                          reg_desc = &feat_desc->regs[i];
>>                          flags = reg_desc->flags;
>>                          reg = &feat->regs[i];
>>                          if (flags & CS_CFG_REG_VAL_PARAM) {
>>                                  if (param == &feat->params[reg_desc->value.val32]) {
>>                                          coresight_cfg_init_reg_param(reg, flags, param);
>>                                  }
>>                          }
>>                  }
>>         }
>>        spin_unlock(param->feat->dev_spinlock);
>>     }
>>

OK - I may have fixed this in the new set - please try this out:-
https://lists.linaro.org/pipermail/coresight/2020-August/004731.html

>> 2. Is my understanding below correct?
>>   If I set the window param to 1000 and the period param to 10000, then etm will trace the last 1000 cpu cycles of every 10^7 cpu cycles?
>>   And since the strobe window/period counters are saved when disabling etm, the counting should continue from the saved value the next time I enable etm. And I should still
>>   get etm data even if the traced thread is scheduled off cpu before reaching 10^7 cpu cycles each time.
>>

That is essentially it. I was not the original author of the strobing
routine, so cannot remember if it is the first window or the last
window that is traced, but there are period x windows, with trace
captured for a single window.

>> 3. I want to +1 Mathieu's suggestion:
>>
>> Perf sessions need to be able to choose which feature they want to use rather
>> than defaulting to a system wide configuration.
>>

Again look at the new set - perf can select a configuration - which
can include strobing. The new set has an example of this in operation.

>> We use perf sessions to trace ETM data in Android. It's better if we can configure ETM strobe params in the perf interface (like from perf_event_attr or ioctl). And It's better for each perf session to count separately.
>>
>>
>> 4. The last update on the patch is Jun 24. And it seems the patch hasn't been landed in any branch yet.  So what's the future plan?
>>

The patch set is to allow a generic method to configure more complex
coresight programming settings, and make these easily selectable -
from perf and any other client or sysfs.
As such this is in RFC at the moment as we seek consensus on a way
forwards. Once that is acheived then it will be pushed upstream.


>> We need to wait until it lands to some upstream branches to cherry pick it to Android kernels. So I want to know when it will be ready.
>>
>>
>> Thanks,
>>
>> Yabin

Regards

Mike


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK