[alsa-devel] [Resend] Add the stream report feature. Could you review my idea?

[Takashi Sakamoto]

Hi JongHo,

I can understand your needs but I have a question for your idea.

Most of snd_printd() output in your patch are fixed value in each 
substream and it's retrieved from user-land via nodes in procfs or via 
library APIs. I think it OK to retrieve them immediately when the error 
message is generated.

So I think it enough to retrieve these items for every time:
+ snd_printd("avail: %ld\n", snd_pcm_playback_avail(runtime));
+ else
+ snd_printd("avail: %ld\n", snd_pcm_capture_avail(runtime));
+ snd_printd("avail_max: %ld\n", runtime->avail_max);
+ snd_printd("hw_ptr: %ld\n", runtime->status->hw_ptr);
+ snd_printd("appl_ptr: %ld\n", runtime->control->appl_ptr);

The similar information can be got via hw_ptr_error() in 
sound/core/pcm_lib.c. You can enable the output when enabling 
CONFIG_SND_PCM_XRUN_DEBUG and writing appropriate value to 
/proc/asound/card%d/pcm%d{c,p}/xrun_debug. See:
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/pcm_lib.c#n145

I think there is already an alternative so I disagree with your idea.


Regards

Takashi (not a maintainer) Sakamoto
o-takashi at sakamocchi.jp

(2014年05月15日 01:59), JongHo Kim wrote:
> (Add the alsa-devel at alsa-project.org)
>
> Hi
>
> I am work on ASoC part and developing the smartphone(Android).
> we have been doing the stress and long runtime test.
> audio output is no problem but I sometimes face
> the "playback write error(DMA or IRQ trouble?)" error message.
> I know the meaning of the message and I can solve the error.
> the problem may be caused by AP(clk) or I2S or DMA or Codec.
>
> I would like to trace the hardware error at the printed message.
>
> Could you review my idea?
>
> Thanks
> ---
>   include/sound/pcm.h     |  2 ++
>   include/sound/soc-dai.h |  2 ++
>   include/sound/soc.h     |  1 +
>   sound/core/pcm_lib.c    |  3 +++
>   sound/core/pcm_native.c | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++
>   sound/soc/soc-pcm.c     | 26 +++++++++++++++++++++++++
>   6 files changed, 85 insertions(+)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index b4d6697..cd3093a 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -82,6 +82,7 @@ struct snd_pcm_ops {
>        unsigned long offset);
>    int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct
> *vma);
>    int (*ack)(struct snd_pcm_substream *substream);
> + void (*report)(struct snd_pcm_substream *substream);
>   };
>
>   /*
> @@ -502,6 +503,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
>   int snd_pcm_start(struct snd_pcm_substream *substream);
>   int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t
> status);
>   int snd_pcm_drain_done(struct snd_pcm_substream *substream);
> +void snd_pcm_report(struct snd_pcm_substream *substream);
>   #ifdef CONFIG_PM
>   int snd_pcm_suspend(struct snd_pcm_substream *substream);
>   int snd_pcm_suspend_all(struct snd_pcm *pcm);
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index fad7676..c032393 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -184,6 +184,8 @@ struct snd_soc_dai_ops {
>    struct snd_soc_dai *);
>    int (*bespoke_trigger)(struct snd_pcm_substream *, int,
>    struct snd_soc_dai *);
> + void (*report)(struct snd_pcm_substream *,
> + struct snd_soc_dai *);
>    /*
>    * For hardware based FIFO caused delay reporting.
>    * Optional.
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 0b83168..7685839 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -641,6 +641,7 @@ struct snd_soc_ops {
>    int (*hw_free)(struct snd_pcm_substream *);
>    int (*prepare)(struct snd_pcm_substream *);
>    int (*trigger)(struct snd_pcm_substream *, int);
> + void (*report)(struct snd_pcm_substream *);
>   };
>
>   struct snd_soc_compr_ops {
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index ce83def..0d6c9a3 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1944,6 +1944,9 @@ static int wait_for_avail(struct snd_pcm_substream
> *substream,
>    "%s write error (DMA or IRQ trouble?)\n",
>    is_playback ? "playback" : "capture");
>    err = -EIO;
> + snd_pcm_stream_unlock_irq(substream);
> + snd_pcm_report(substream);
> + snd_pcm_stream_lock_irq(substream);
>    break;
>    }
>    }
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index b653ab0..0ccdc10 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -978,6 +978,57 @@ int snd_pcm_drain_done(struct snd_pcm_substream
> *substream)
>        SNDRV_PCM_STATE_SETUP);
>   }
>
> +void snd_pcm_report(struct snd_pcm_substream *substream)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> +
> + if (!runtime) {
> + snd_printd("runtime disappear\n");
> + return;
> + }
> +
> + mutex_lock(&substream->pcm->open_mutex);
> + snd_printd("access: %i\n", runtime->access);
> + snd_printd("format: %i\n", runtime->format);
> + snd_printd("subformat: %d\n", runtime->subformat);
> + snd_printd("channels: %u\n", runtime->channels);
> + snd_printd("rate: %u (%u/%u)\n", runtime->rate, runtime->rate_num,
> +   runtime->rate_den);
> + snd_printd("period_size: %lu\n", runtime->period_size);
> + snd_printd("buffer_size: %lu\n", runtime->buffer_size);
> + snd_printd("tstamp_mode: %i\n", runtime->tstamp_mode);
> + snd_printd("period_step: %u\n", runtime->period_step);
> + snd_printd("avail_min: %lu\n", runtime->control->avail_min);
> + snd_printd("start_threshold: %lu\n", runtime->start_threshold);
> + snd_printd("stop_threshold: %lu\n", runtime->stop_threshold);
> + snd_printd("silence_threshold: %lu\n", runtime->silence_threshold);
> + snd_printd("silence_size: %lu\n", runtime->silence_size);
> + snd_printd("boundary: %lu\n", runtime->boundary);
> + snd_printd("state: %i\n", runtime->status->state);
> + snd_printd("owner_pid: %d\n", pid_vnr(substream->pid));
> + snd_printd("trigger_time: %ld.%09ld\n",
> +   runtime->trigger_tstamp.tv_sec,
> +   runtime->trigger_tstamp.tv_nsec);
> + snd_printd("tstamp: %ld.%09ld\n",
> +   runtime->status->tstamp.tv_sec,
> +   runtime->status->tstamp.tv_nsec);
> + snd_printd("delay: %ld\n", runtime->delay);
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + snd_printd("avail: %ld\n", snd_pcm_playback_avail(runtime));
> + else
> + snd_printd("avail: %ld\n", snd_pcm_capture_avail(runtime));
> + snd_printd("avail_max: %ld\n", runtime->avail_max);
> + snd_printd("hw_ptr: %ld\n", runtime->status->hw_ptr);
> + snd_printd("appl_ptr: %ld\n", runtime->control->appl_ptr);
> +
> + mutex_unlock(&substream->pcm->open_mutex);
> +
> + if (substream->ops->report)
> + substream->ops->report(substream);
> +
> + return;
> +}
> +
>   /*
>    * pause callbacks
>    */
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 2cedf09..98a47b3 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -883,6 +883,30 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct
> snd_pcm_substream *substream)
>    return offset;
>   }
>
> +static void soc_pcm_report(struct snd_pcm_substream *substream)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_platform *platform = rtd->platform;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + struct snd_soc_dai *codec_dai = rtd->codec_dai;
> +
> + mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + /* report the audio subsystem */
> + if (rtd->dai_link->ops && rtd->dai_link->ops->report)
> + rtd->dai_link->ops->report(substream);
> +
> + if (cpu_dai->driver->ops->report)
> + cpu_dai->driver->ops->report(substream, cpu_dai);
> +
> + if (platform->driver->ops && platform->driver->ops->report)
> + platform->driver->ops->report(substream);
> +
> + if (codec_dai->driver->ops->report)
> + codec_dai->driver->ops->report(substream, codec_dai);
> +
> + mutex_unlock(&rtd->pcm_mutex);
> +}
> +
>   /* connect a FE and BE */
>   static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
>    struct snd_soc_pcm_runtime *be, int stream)
> @@ -2267,6 +2291,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int
> num)
>    rtd->ops.close = dpcm_fe_dai_close;
>    rtd->ops.pointer = soc_pcm_pointer;
>    rtd->ops.ioctl = soc_pcm_ioctl;
> + rtd->ops.report = soc_pcm_report;
>    } else {
>    rtd->ops.open = soc_pcm_open;
>    rtd->ops.hw_params = soc_pcm_hw_params;
> @@ -2276,6 +2301,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int
> num)
>    rtd->ops.close = soc_pcm_close;
>    rtd->ops.pointer = soc_pcm_pointer;
>    rtd->ops.ioctl = soc_pcm_ioctl;
> + rtd->ops.report = soc_pcm_report;
>    }
>
>    if (platform->driver->ops) {