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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Sep 2018 19:25:26 +0530
From:   Vinod <vkoul@...nel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     broonie@...nel.org, alsa-devel@...a-project.org,
        robh+dt@...nel.org, linux-kernel@...r.kernel.org,
        bgoswami@...eaurora.org, rohitkr@...eaurora.org,
        lgirdwood@...il.com, tiwai@...e.com, perex@...ex.cz,
        devicetree@...r.kernel.org, mark.rutland@....com
Subject: Re: [PATCH 3/3] ASoC: qdsp6: q6asm-dai: Add support to compress
 offload

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