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

[Takashi Iwai]

On Wed, 07 Jun 2017 09:50:42 +0200,
Takashi Sakamoto wrote:
> 
> 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.

Fair enough, we can keep as is then.

> >> 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.

It's fine if you can promise to submit the document patch in this
development cycle :)


thanks,

Takashi