Re: [PATCH 3/3] ASoC: qdsp6: q6asm-dai: Add support to compress offload

[Vinod <vkoul@kernel.org>]

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

> +			} 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

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

> +
> +		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?

> +static int q6asm_dai_compr_set_params(struct snd_compr_stream *stream,
> +				      struct snd_compr_params *params)
> +{
> +

redundant empty line

> +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()

> +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...
> +
> +	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;
> +		}
> +	}

move this to .ack

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

-- 
~Vinod