lkml.org 
[lkml]   [2018]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] ASoC: qdsp6: q6asm-dai: Add support to compress offload
From
Date
Thanks for the review,

On 04/09/18 14:55, Vinod wrote:
> On 03-09-18, 13:34, Srinivas Kandagatla wrote:
>
>> +static void compress_event_handler(uint32_t opcode, uint32_t token,
>> + uint32_t *payload, void *priv)
>> +{
>> + struct q6asm_dai_rtd *prtd = priv;
>> + struct snd_compr_stream *substream = prtd->cstream;
>> + unsigned long flags;
>> + uint64_t avail;
>> +
>> + switch (opcode) {
>> + case ASM_CLIENT_EVENT_CMD_RUN_DONE:
>> + spin_lock_irqsave(&prtd->lock, flags);
>> + avail = prtd->bytes_received - prtd->bytes_sent;
>> + if (!prtd->bytes_sent) {
>> + if (avail < substream->runtime->fragment_size) {
>> + prtd->xrun = 1;
>
> so you are trying to detect xrun :) So in compress core we added support
> for .ack callback which tells driver how much data is valid in ring
> buffer and we can send this to DSP, so DSP "knows" valid data and should
> not overrun, ofcourse DSP needs support for it
>
Thanks, I will take a closer look at ack callback.

>> + } else {
>> + q6asm_write_async(prtd->audio_client,
>> + prtd->pcm_count,
>> + 0, 0, NO_TIMESTAMP);
>> + prtd->bytes_sent += prtd->pcm_count;
>> + }
>> + }
>> +
>> + spin_unlock_irqrestore(&prtd->lock, flags);
>> + break;
>
> empty line after break helps readability

Yes, I will do.
>
>> + case ASM_CLIENT_EVENT_CMD_EOS_DONE:
>> + prtd->state = Q6ASM_STREAM_STOPPED;
>> + break;
>> + case ASM_CLIENT_EVENT_DATA_WRITE_DONE:
>> + spin_lock_irqsave(&prtd->lock, flags);
>> + prtd->byte_offset += prtd->pcm_count;
>> + prtd->copied_total += prtd->pcm_count;
>
> so should you need two counters, copied_total should give you byte_offset
> as well, we know the ring buffer size

Yep, looks redundant to me too.
>
>> +
>> + if (prtd->byte_offset >= prtd->pcm_size)
>> + prtd->byte_offset -= prtd->pcm_size;
>
> :)
>
>> +
>> + snd_compr_fragment_elapsed(substream);
>
> so will ASM_CLIENT_EVENT_DATA_WRITE_DONE be invoked on fragment bytes
> consumed?
Yes.
>
>> +static int q6asm_dai_compr_set_params(struct snd_compr_stream *stream,
>> + struct snd_compr_params *params)
>> +{
>> +
>
> redundant empty line
ya.
>
>> +static int q6asm_dai_compr_trigger(struct snd_compr_stream *stream, int cmd)
>> +{
>> + struct snd_compr_runtime *runtime = stream->runtime;
>> + struct q6asm_dai_rtd *prtd = runtime->private_data;
>> + int ret = 0;
>> +
>> + switch (cmd) {
>> + case SNDRV_PCM_TRIGGER_START:
>> + case SNDRV_PCM_TRIGGER_RESUME:
>> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> + ret = q6asm_run_nowait(prtd->audio_client, 0, 0, 0);
>
> the triggers are not in atomic context, do we have q6asm_run()
>
Yes, we do have q6asm_run() which is a blocking call.

>> +static int q6asm_dai_compr_copy(struct snd_compr_stream *stream,
>> + char __user *buf, size_t count)
>> +{
>> + struct snd_compr_runtime *runtime = stream->runtime;
>> + struct q6asm_dai_rtd *prtd = runtime->private_data;
>> + uint64_t avail = 0;
>> + unsigned long flags;
>> + size_t copy;
>> + void *dstn;
>> +
>> + dstn = prtd->buffer + prtd->copy_pointer;
>> + if (count < prtd->pcm_size - prtd->copy_pointer) {
>> + if (copy_from_user(dstn, buf, count))
>> + return -EFAULT;
>> +
>> + prtd->copy_pointer += count;
>> + } else {
>> + copy = prtd->pcm_size - prtd->copy_pointer;
>> + if (copy_from_user(dstn, buf, copy))
>> + return -EFAULT;
>> +
>> + if (copy_from_user(prtd->buffer, buf + copy, count - copy))
>> + return -EFAULT;
>> + prtd->copy_pointer = count - copy;
>> + }
>> +
>> + spin_lock_irqsave(&prtd->lock, flags);
>> + prtd->bytes_received += count;
>
> why not use core copy method and..

Which core method are you referring to?
.
>> +
>> + if (prtd->state == Q6ASM_STREAM_RUNNING && prtd->xrun) {
>> + avail = prtd->bytes_received - prtd->copied_total;
>> + if (avail >= runtime->fragment_size) {
>> + prtd->xrun = 0;
>> + q6asm_write_async(prtd->audio_client,
>> + prtd->pcm_count, 0, 0, NO_TIMESTAMP);
>> + prtd->bytes_sent += prtd->pcm_count;
>> + }
>> + }
...
>
>> +static int q6asm_dai_compr_get_codec_caps(struct snd_compr_stream *stream,
>> + struct snd_compr_codec_caps *codec)
>> +{
>> + switch (codec->codec) {
>> + case SND_AUDIOCODEC_MP3:
>> + codec->num_descriptors = 2;
>> + codec->descriptor[0].max_ch = 2;
>> + memcpy(codec->descriptor[0].sample_rates,
>> + supported_sample_rates,
>> + sizeof(supported_sample_rates));
>> + codec->descriptor[0].num_sample_rates =
>> + sizeof(supported_sample_rates)/sizeof(unsigned int);
>> + codec->descriptor[0].bit_rate[0] = 320; /* 320kbps */
>> + codec->descriptor[0].bit_rate[1] = 128;
>> + codec->descriptor[0].num_bitrates = 2;
>> + codec->descriptor[0].profiles = 0;
>> + codec->descriptor[0].modes = SND_AUDIOCHANMODE_MP3_STEREO;
>> + codec->descriptor[0].formats = 0;
>
> since we are static here, how about using a table based approach and
> use that here
Sure, will do that in next version.

thanks,
srini
>

\
 
 \ /
  Last update: 2018-09-12 12:31    [W:0.082 / U:3.648 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site