Re: [Resend] Add the stream report feature. Could you review my idea?

["KimJongHo_f" <furmuwon@gmail.com>]

Thanks for your replied

I will give you a supplementary explanation.
We work with test engineer. They are only test for the developing smartphone.
They reported on the error to me with kernel log files when discover the audio error.
If I found error message "playback write error(DMA or IRQ trouble?)" in the kernel log, 
To fix the problem is very difficult because do not had any information.
The object of the under patch is to get more infomation when the error message is generated.
ex> how longtime play? which hardware is problem? and hardware status? more.. more..

The xrun_debug and "/proc/asound/card[n]/pcm[n]p,c/sub[n]/* " are very excellent functions!
I know and use the that but that is not help for hardware and then that is operated by console actions.

So I using the added snd_pcm_report and soc_pcm_report,
I want to see the hardware register status when the error message is generated.
In addition, I want to know substream's runtime status infomation.

-----Original Message-----
From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp] 
Sent: Thursday, May 15, 2014 12:37 PM
To: JongHo Kim; Jaroslav Kysela; Abramo Bagnara; Takashi Iwai; Mark Brown; Liam Girdwood
Cc: alsa-devel@alsa-project.org
Subject: Re: [Resend] Add the stream report feature. Could you review my idea?

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@sakamocchi.jp

(2014年05月15日 01:59), JongHo Kim wrote:
> (Add the alsa-devel@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) {

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel