[alsa-devel] [PATCH 0/3] ALSA: pcm: add tracepoints for PCM params refinement

[Takashi Sakamoto]

Hi,

On Jun 7 2017 14:42, Takashi Iwai wrote:
> On Wed, 07 Jun 2017 01:46:42 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> This patchset is a part of my previous RFC and go for upstream.
>> [alsa-devel] [PATCH RFC 00/21] ALSA: pcm: add tracepoints for PCM params operation
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120548.html
>>
>> In ALSA PCM interface, applications can get hardware capability by ioctl(2)
>> with SNDRV_PCM_IOCTL_HW_REFINE/SNDRV_PCM_IOCT_HW_PARAMS in a shape of
>> 'struct snd_pcm_hw_params'. In kernel side, relevant processing is somewhat
>> complicated and developers sometimes have hard time to debug drivers for PCM
>> constraints and rules. This patchset adds tracepoints to assist their work.
>>
>> Changes:
>>   - change print format
>>   - rename events
>>   - refactoring in the RFC will be done in my later work.
>>
>>
>> This patchset adds two events; 'hw_interval_param' and 'hw_mask_param' in
>> 'snd_pcm' subsystem. When these events are probed, tracer outputs below
>> lines:
>>
>> hw_interval_param: 0,0,0,0 000/023 BUFFER_SIZE 0 0 [0 4294967295] 0 1 [0 4294967295]
>> hw_interval_param: 0,0,0,0 000/023 BUFFER_BYTES 0 0 [0 4294967295] 0 1 [128 65536]
>> hw_interval_param: 0,0,0,0 000/023 TICK_TIME 0 0 [0 4294967295] 0 0 [0 4294967295]
>> hw_mask_param:     0,0,0,0 001/023 FORMAT 00000000000000000000001000000044 00000000000000000000001000000044
>> hw_interval_param: 0,0,0,0 002/023 SAMPLE_BITS 0 1 [0 4294967295] 0 1 [16 32]
>> hw_interval_param: 0,0,0,0 003/023 SAMPLE_BITS 0 1 [16 32] 0 1 [16 32]
>>
>> The first field represents PCM character device and subdevice for probed
>> runtime of PCM substream. In the above case, '/dev/snd/pcmC0D0p' and
>> first subdevice are used for runtime of PCM substream.
> 
> How about using snd_pcm_debug_name()?  It'll be like "pcmC0D1c:2" and
> slightly more understandable than "0,1,1,2".

Yep. This looks a good idea, however for a reason I'm against it.

When mounting tracefs, you can see formats of the tracepoint. For example:

$ cat /sys/kernel/debug/tracing/events/snd_pcm/hw_mask_param/format
...
format: field:int card: ...
...
print fmt: "%d,%d,%d,%d %03d/%03d %s %08x%08x%08x%08x %08x%08x%08x%08x",
...

This means that structured data is passed to userspace in a shape of 
the 'format', then userspace application handle the data with the 'print 
fmt' for friendly printing.

If using strings generated by 'snd_pcm_debug_name()' for the printing, 
we cannot write it in 'TP_printk()' field, because format in this field 
just copied to userspace as is, as we just saw. It should be written in 
'TP_fast_assign()' field in this case. This means that the structured 
data need to include u8 array for the string.

Of cource, we can program this tracepoint in your way. But to me this is 
not so convenient, because it loses flexibility for userspace 
applications to handle the structured data. The application should parse 
the u8 array as pre-formatted text to reuse its elements; e.g. 
subdevice. This is not so friendly in a point to write tracer program.

>> The second field represents id of each rule applied to the runtime, and
>> the total number of rules added to the runtime. Basically, when the rule
>> is applied to the runtime, these events are probed, but lines with id 0
>> are exceptions. They're application of constraints to the runtime.
>>
>> The third field is name of parameter in the runtime. The rest fields
>> depends on type of the parameter (mask/interval).
>>
>>
>> In ALSA PCM core, runtimes get 22 (or 21) rules as a default. In a below
>> sample, the 21st rule is added by driver (snd-soc-imx-ssi.ko). The rest is
>> added by snd-pcm.ko. In detail, please see 'snd_pcm_hw_constraints_init()'
>> and 'snd_pcm_hw_constraints_complete()' in 'sound/core/pcm_native.c'.
>>
>> hw_interval_param: 0,0,0,0 019/023 PERIOD_TIME 0 0 [0 4294967295] 0 0 (166 4095875]
>> hw_interval_param: 0,0,0,0 020/023 BUFFER_TIME 0 0 [0 4294967295] 0 0 (333 4096000]
>> hw_interval_param: 0,0,0,0 021/023 PERIOD_SIZE 0 1 [16 32767] 0 1 [16 32766]
>> hw_interval_param: 0,0,0,0 022/023 BUFFER_BYTES 0 1 [128 65536] 0 1 [128 65536]
>> hw_interval_param: 0,0,0,0 023/023 RATE 0 0 [8000 96000] 0 0 [8000 96000]
>> hw_mask_param:     0,0,0,0 001/023 FORMAT 00000000000000000000001000000044 00000000000000000000001000000044
> 
> Could you add these explanations in Documentation?  The cover letter
> is gone at merging.

Hm. Could I postpone the task for my later work in this development 
period? Text writing takes me a bit time, but I have patches for the 
rest of my RFC and firewire stack.


Regards

Takashi Sakamoto