Messages in this thread | | | Subject | Re: [PATCH v2 8/9] ASoC: qdsp6-dai: add gapless support | From | Srinivas Kandagatla <> | Date | Wed, 22 Jul 2020 10:00:06 +0100 |
| |
Thanks Pierre for quick review!
On 21/07/2020 20:53, Pierre-Louis Bossart wrote: > > > > >> case ASM_CLIENT_EVENT_CMD_EOS_DONE: >> - prtd->state = Q6ASM_STREAM_STOPPED; >> + spin_lock_irqsave(&prtd->lock, flags); >> + if (prtd->notify_on_drain) { >> + if (substream->partial_drain) { >> + /** > > why the kernel-doc style comment? > Nothing intentional, will fix this!
> [...]
> >> -static int q6asm_dai_compr_set_params(struct snd_soc_component >> *component, >> - struct snd_compr_stream *stream, >> - struct snd_compr_params *params) >> +static int __q6asm_dai_compr_set_codec_params(struct >> snd_soc_component *component, >> + struct snd_compr_stream *stream, >> + struct snd_codec *codec, >> + int stream_id) > > not sure I get why you added the __ prefix, does it have any semantic > meaning? Nope, just to mark them it as internal function, as the function name is very much similar to q6asm_dai_compr_set_params() callback! I will try to come up with better naming and also move the indirection changes to a separate patch!
> >> { >> struct snd_compr_runtime *runtime = stream->runtime; >> struct q6asm_dai_rtd *prtd = runtime->private_data; >> - struct snd_soc_pcm_runtime *rtd = stream->private_data; >> - int dir = stream->direction; >> - struct q6asm_dai_data *pdata; >> struct q6asm_flac_cfg flac_cfg; >> struct q6asm_wma_cfg wma_cfg; >> struct q6asm_alac_cfg alac_cfg; >> @@ -667,53 +718,18 @@ static int q6asm_dai_compr_set_params(struct >> snd_soc_component *component, >> struct snd_dec_alac *alac; >> struct snd_dec_ape *ape; >> - codec_options = &(prtd->codec_param.codec.options); >> - >> - >> - memcpy(&prtd->codec_param, params, sizeof(*params)); >> - >> - pdata = snd_soc_component_get_drvdata(component); >> - if (!pdata) >> - return -EINVAL; >> - >> - if (!prtd || !prtd->audio_client) { >> - dev_err(dev, "private data null or audio client freed\n"); >> - return -EINVAL; >> - } >> - >> - prtd->periods = runtime->fragments; >> - prtd->pcm_count = runtime->fragment_size; >> - prtd->pcm_size = runtime->fragments * runtime->fragment_size; >> - prtd->bits_per_sample = 16; >> - if (dir == SND_COMPRESS_PLAYBACK) { >> - ret = q6asm_open_write(prtd->audio_client, prtd->stream_id, >> - params->codec.id, params->codec.profile, >> - prtd->bits_per_sample, true); >> - >> - if (ret < 0) { >> - dev_err(dev, "q6asm_open_write failed\n"); >> - q6asm_audio_client_free(prtd->audio_client); >> - prtd->audio_client = NULL; >> - return ret; >> - } >> - } >> + codec_options = &(prtd->codec.options); >> - prtd->session_id = q6asm_get_session_id(prtd->audio_client); >> - ret = q6routing_stream_open(rtd->dai_link->id, LEGACY_PCM_MODE, >> - prtd->session_id, dir); >> - if (ret) { >> - dev_err(dev, "Stream reg failed ret:%d\n", ret); >> - return ret; >> - } >> + memcpy(&prtd->codec, codec, sizeof(*codec)); >> - switch (params->codec.id) { >> + switch (codec->id) { >> case SND_AUDIOCODEC_FLAC: >> memset(&flac_cfg, 0x0, sizeof(struct q6asm_flac_cfg)); >> flac = &codec_options->flac_d; >> - flac_cfg.ch_cfg = params->codec.ch_in; >> - flac_cfg.sample_rate = params->codec.sample_rate; > > all these indirection changes could have gone in a earlier patch, this > adds a lot of changes that make this patch long to review. Same comment > for all formats > >> + flac_cfg.ch_cfg = codec->ch_in; >> + flac_cfg.sample_rate = codec->sample_rate; >> flac_cfg.stream_info_present = 1; >> flac_cfg.sample_size = flac->sample_size; >> flac_cfg.min_blk_size = flac->min_blk_size; > > [...] > ...
>> @@ -917,6 +1018,14 @@ static int q6asm_dai_compr_trigger(struct >> snd_soc_component *component, >> ret = q6asm_cmd_nowait(prtd->audio_client, prtd->stream_id, >> CMD_PAUSE); >> break; >> + case SND_COMPR_TRIGGER_NEXT_TRACK: >> + prtd->next_track = true; >> + prtd->next_track_stream_id = (prtd->stream_id == 1 ? 2 : 1); > > it's rather odd, the initialization above uses next_track_stream_id = 0?
Vaild stream ids start from 1, So we are toggling this between 1 and 2. So when we set next_track_stream_id to 0, that means we have opened the new next stream id and is set to prtd->stream_id. This logic is to ensure that we are not going to open next stream id twice!
> >> + break; >> + case SND_COMPR_TRIGGER_DRAIN: >> + case SND_COMPR_TRIGGER_PARTIAL_DRAIN: >> + prtd->notify_on_drain = true; >> + break; >> default: >> ret = -EINVAL; >> break; >> diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h >> index 69513ac1fa55..a8dddfeb28da 100644 >> --- a/sound/soc/qcom/qdsp6/q6asm.h >> +++ b/sound/soc/qcom/qdsp6/q6asm.h >> @@ -34,6 +34,7 @@ enum { >> #define MAX_SESSIONS 8 >> #define NO_TIMESTAMP 0xFF00 >> #define FORMAT_LINEAR_PCM 0x0000 >> +#define ASM_LAST_BUFFER_FLAG BIT(30) >> struct q6asm_flac_cfg { >> u32 sample_rate; >>
| |