Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751267AbeABUIV (ORCPT + 1 other); Tue, 2 Jan 2018 15:08:21 -0500 Received: from mail-pl0-f66.google.com ([209.85.160.66]:36773 "EHLO mail-pl0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751069AbeABUIS (ORCPT ); Tue, 2 Jan 2018 15:08:18 -0500 X-Google-Smtp-Source: ACJfBou8iX6pPNXn5AJ+L+lYuoQNMF6bcttbvAVOR4pJIWtl6qvIxlhDbtK7KvjPOcKlL2gLY5oq6A== Date: Tue, 2 Jan 2018 12:08:13 -0800 From: Bjorn Andersson To: srinivas.kandagatla@linaro.org Cc: Andy Gross , Mark Brown , linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org, David Brown , Rob Herring , Mark Rutland , Liam Girdwood , Patrick Lai , Banajit Goswami , Jaroslav Kysela , Takashi Iwai , linux-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sboyd@codeaurora.org Subject: Re: [RESEND PATCH v2 08/15] ASoC: qcom: q6asm: add support to audio stream apis Message-ID: <20180102200813.GA8625@builder> References: <20171214173402.19074-1-srinivas.kandagatla@linaro.org> <20171214173402.19074-9-srinivas.kandagatla@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171214173402.19074-9-srinivas.kandagatla@linaro.org> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla > > This patch adds support to open, write and media format commands > in the q6asm module. > > Signed-off-by: Srinivas Kandagatla > --- > sound/soc/qcom/qdsp6/q6asm.c | 530 ++++++++++++++++++++++++++++++++++++++++++- > sound/soc/qcom/qdsp6/q6asm.h | 42 ++++ > 2 files changed, 571 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c > index 4be92441f524..dabd6509ef99 100644 > --- a/sound/soc/qcom/qdsp6/q6asm.c > +++ b/sound/soc/qcom/qdsp6/q6asm.c > @@ -8,16 +8,34 @@ > #include > #include > #include > +#include > #include > #include > #include > #include "q6asm.h" > #include "common.h" > > +#define ASM_STREAM_CMD_CLOSE 0x00010BCD > +#define ASM_STREAM_CMD_FLUSH 0x00010BCE > +#define ASM_SESSION_CMD_PAUSE 0x00010BD3 > +#define ASM_DATA_CMD_EOS 0x00010BDB > +#define DEFAULT_POPP_TOPOLOGY 0x00010BE4 > +#define ASM_STREAM_CMD_FLUSH_READBUFS 0x00010C09 > #define ASM_CMD_SHARED_MEM_MAP_REGIONS 0x00010D92 > #define ASM_CMDRSP_SHARED_MEM_MAP_REGIONS 0x00010D93 > #define ASM_CMD_SHARED_MEM_UNMAP_REGIONS 0x00010D94 > - > +#define ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2 0x00010D98 > +#define ASM_DATA_EVENT_WRITE_DONE_V2 0x00010D99 > +#define ASM_SESSION_CMD_RUN_V2 0x00010DAA > +#define ASM_MEDIA_FMT_MULTI_CHANNEL_PCM_V2 0x00010DA5 > +#define ASM_DATA_CMD_WRITE_V2 0x00010DAB > +#define ASM_SESSION_CMD_SUSPEND 0x00010DEC > +#define ASM_STREAM_CMD_OPEN_WRITE_V3 0x00010DB3 > + > +#define ASM_LEGACY_STREAM_SESSION 0 > +#define ASM_END_POINT_DEVICE_MATRIX 0 > +#define DEFAULT_APP_TYPE 0 > +#define TUN_WRITE_IO_MODE 0x0008 /* tunnel read write mode */ > #define TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */ > #define SYNC_IO_MODE 0x0001 > #define ASYNC_IO_MODE 0x0002 Probably prettier to reorder these and make them Q6ASM_IO_MODE_xyz [..] > > +static int32_t q6asm_callback(struct apr_device *adev, This callback is an extracted part of q6asm_srvc_callback(), can it be given a more descriptive name? > + struct apr_client_data *data, int session_id) > +{ > + struct audio_client *ac;// = (struct audio_client *)priv; > + uint32_t token; > + uint32_t *payload; > + uint32_t wakeup_flag = 1; > + uint32_t client_event = 0; > + struct q6asm *q6asm = dev_get_drvdata(&adev->dev); > + > + if (data == NULL) > + return -EINVAL; > + > + ac = q6asm_get_audio_client(q6asm, session_id); > + if (!q6asm_is_valid_audio_client(ac)) > + return -EINVAL; > + > + payload = data->payload; > + > + if (data->opcode == APR_BASIC_RSP_RESULT) { Move this into the switch. > + token = data->token; > + switch (payload[0]) { This is again that common response struct. > + case ASM_SESSION_CMD_PAUSE: > + client_event = ASM_CLIENT_EVENT_CMD_PAUSE_DONE; > + break; > + case ASM_SESSION_CMD_SUSPEND: > + client_event = ASM_CLIENT_EVENT_CMD_SUSPEND_DONE; > + break; > + case ASM_DATA_CMD_EOS: > + client_event = ASM_CLIENT_EVENT_CMD_EOS_DONE; > + break; > + break; > + case ASM_STREAM_CMD_FLUSH: > + client_event = ASM_CLIENT_EVENT_CMD_FLUSH_DONE; > + break; > + case ASM_SESSION_CMD_RUN_V2: > + client_event = ASM_CLIENT_EVENT_CMD_RUN_DONE; > + break; > + > + case ASM_STREAM_CMD_FLUSH_READBUFS: > + if (token != ac->session) { > + dev_err(ac->dev, "session invalid\n"); > + return -EINVAL; > + } > + case ASM_STREAM_CMD_CLOSE: > + client_event = ASM_CLIENT_EVENT_CMD_CLOSE_DONE; > + break; > + case ASM_STREAM_CMD_OPEN_WRITE_V3: > + case ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2: > + if (payload[1] != 0) { > + dev_err(ac->dev, > + "cmd = 0x%x returned error = 0x%x\n", > + payload[0], payload[1]); > + if (wakeup_flag) { > + ac->cmd_state = payload[1]; > + wake_up(&ac->cmd_wait); > + } > + return 0; > + } > + break; > + default: > + dev_err(ac->dev, "command[0x%x] not expecting rsp\n", > + payload[0]); > + break; > + } > + > + if (ac->cmd_state && wakeup_flag) { > + ac->cmd_state = 0; > + wake_up(&ac->cmd_wait); > + } > + if (ac->cb) > + ac->cb(client_event, data->token, > + data->payload, ac->priv); > + > + return 0; > + } > + > + switch (data->opcode) { > + case ASM_DATA_EVENT_WRITE_DONE_V2:{ > + struct audio_port_data *port = > + &ac->port[SNDRV_PCM_STREAM_PLAYBACK]; > + > + client_event = ASM_CLIENT_EVENT_DATA_WRITE_DONE; > + > + if (ac->io_mode & SYNC_IO_MODE) { > + dma_addr_t phys = port->buf[data->token].phys; > + > + if (lower_32_bits(phys) != payload[0] || > + upper_32_bits(phys) != payload[1]) { > + dev_err(ac->dev, "Expected addr %pa\n", > + &port->buf[data->token].phys); > + return -EINVAL; > + } > + token = data->token; > + port->buf[token].used = 1; > + } > + break; > + } > + } > + if (ac->cb) > + ac->cb(client_event, data->token, data->payload, ac->priv); > + > + return 0; > +} > + > static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data) > { > struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev); > @@ -415,12 +581,16 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data * > struct audio_port_data *port; > uint32_t dir = 0; > uint32_t sid = 0; > + int dest_port; > uint32_t *payload; > > if (!data) { > dev_err(&adev->dev, "%s: Invalid CB\n", __func__); > return 0; > } > + dest_port = (data->dest_port >> 8) & 0xFF; > + if (dest_port) > + return q6asm_callback(adev, data, dest_port); You call dest_port "session_id" above, this seems to be a better name for this variable. > > payload = data->payload; > sid = (data->token >> 8) & 0x0F; > @@ -540,6 +710,364 @@ struct audio_client *q6asm_audio_client_alloc(struct device *dev, > } > EXPORT_SYMBOL_GPL(q6asm_audio_client_alloc); > > +static int __q6asm_open_write(struct audio_client *ac, uint32_t format, > + uint16_t bits_per_sample, uint32_t stream_id, > + bool is_gapless_mode) > +{ > + struct asm_stream_cmd_open_write_v3 open; > + int rc; > + > + q6asm_add_hdr(ac, &open.hdr, sizeof(open), true, stream_id); > + ac->cmd_state = -1; > + > + open.hdr.opcode = ASM_STREAM_CMD_OPEN_WRITE_V3; > + open.mode_flags = 0x00; > + open.mode_flags |= ASM_LEGACY_STREAM_SESSION; > + if (is_gapless_mode) This is hard coded as false. > + open.mode_flags |= 1 << ASM_SHIFT_GAPLESS_MODE_FLAG; > + > + /* source endpoint : matrix */ > + open.sink_endpointype = ASM_END_POINT_DEVICE_MATRIX; > + open.bits_per_sample = bits_per_sample; > + open.postprocopo_id = DEFAULT_POPP_TOPOLOGY; > + > + switch (format) { > + case FORMAT_LINEAR_PCM: > + open.dec_fmt_id = ASM_MEDIA_FMT_MULTI_CHANNEL_PCM_V2; > + break; > + default: > + dev_err(ac->dev, "Invalid format 0x%x\n", format); > + return -EINVAL; > + } > + rc = apr_send_pkt(ac->adev, (uint32_t *) &open); > + if (rc < 0) > + return rc; > + > + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ); > + if (!rc) { > + dev_err(ac->dev, "timeout on open write\n"); > + return -ETIMEDOUT; > + } Almost every time you apr_send_pkt() you have this wait with timeout, can this send/wait/return be wrapped in a helper function to reduce the duplication? Creating a q6asm_send_sync() and q6asm_send_async() pair with this logic should help quite a bit. > + > + if (ac->cmd_state > 0) > + return adsp_err_get_lnx_err_code(ac->cmd_state); > + > + ac->io_mode |= TUN_WRITE_IO_MODE; > + > + return 0; > +} > + > +/** > + * q6asm_open_write() - Open audio client for writing > + * > + * @ac: audio client pointer > + * @format: audio sample format > + * @bits_per_sample: bits per sample > + * > + * Return: Will be an negative value on error or zero on success > + */ > +int q6asm_open_write(struct audio_client *ac, uint32_t format, > + uint16_t bits_per_sample) > +{ > + return __q6asm_open_write(ac, format, bits_per_sample, I don't see a particular reason for not inlining this, is there one coming later in the series? > + ac->stream_id, false); > +} > +EXPORT_SYMBOL_GPL(q6asm_open_write); > + > +static int __q6asm_run(struct audio_client *ac, uint32_t flags, > + uint32_t msw_ts, uint32_t lsw_ts, bool wait) > +{ > + struct asm_session_cmd_run_v2 run; > + int rc; > + > + q6asm_add_hdr(ac, &run.hdr, sizeof(run), true, ac->stream_id); > + ac->cmd_state = -1; > + > + run.hdr.opcode = ASM_SESSION_CMD_RUN_V2; > + run.flags = flags; > + run.time_lsw = lsw_ts; > + run.time_msw = msw_ts; > + > + rc = apr_send_pkt(ac->adev, (uint32_t *) &run); > + if (rc < 0) > + return rc; > + > + if (wait) { Rather than having half of the function conditional I would recommend inlining this function in the two callers. In particular if you can come up with a helper function for the send/wait/handle-error case. > + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), > + 5 * HZ); > + if (!rc) { > + dev_err(ac->dev, "timeout on run cmd\n"); > + return -ETIMEDOUT; > + } > + if (ac->cmd_state > 0) > + return adsp_err_get_lnx_err_code(ac->cmd_state); > + } > + > + return 0; > +} > + > +/** > + * q6asm_run() - start the audio client > + * > + * @ac: audio client pointer > + * @flags: flags associated with write > + * @msw_ts: timestamp msw > + * @lsw_ts: timestamp lsw > + * > + * Return: Will be an negative value on error or zero on success > + */ > +int q6asm_run(struct audio_client *ac, uint32_t flags, > + uint32_t msw_ts, uint32_t lsw_ts) > +{ > + return __q6asm_run(ac, flags, msw_ts, lsw_ts, true); > +} > +EXPORT_SYMBOL_GPL(q6asm_run); > + > +/** > + * q6asm_run_nowait() - start the audio client withou blocking > + * > + * @ac: audio client pointer > + * @flags: flags associated with write > + * @msw_ts: timestamp msw > + * @lsw_ts: timestamp lsw > + * > + * Return: Will be an negative value on error or zero on success > + */ > +int q6asm_run_nowait(struct audio_client *ac, uint32_t flags, > + uint32_t msw_ts, uint32_t lsw_ts) > +{ > + return __q6asm_run(ac, flags, msw_ts, lsw_ts, false); > +} > +EXPORT_SYMBOL_GPL(q6asm_run_nowait); > + > +/** > + * q6asm_media_format_block_multi_ch_pcm() - setup pcm configuration > + * > + * @ac: audio client pointer > + * @rate: audio sample rate > + * @channels: number of audio channels. > + * @use_default_chmap: flag to use default ch map. > + * @channel_map: channel map pointer > + * @bits_per_sample: bits per sample > + * > + * Return: Will be an negative value on error or zero on success > + */ > +int q6asm_media_format_block_multi_ch_pcm(struct audio_client *ac, > + uint32_t rate, uint32_t channels, > + bool use_default_chmap, > + char *channel_map, This should be u8 channel_map[PCM_FORMAT_MAX_NUM_CHANNEL], possibly char. Unless you, as I suggest below, want to be able to represent use_default_chmap = false, by setting this to NULL. > + uint16_t bits_per_sample) > +{ > + struct asm_multi_channel_pcm_fmt_blk_v2 fmt; > + u8 *channel_mapping; > + int rc = 0; Unnecessary initialization. > + > + q6asm_add_hdr(ac, &fmt.hdr, sizeof(fmt), true, ac->stream_id); > + ac->cmd_state = -1; > + > + fmt.hdr.opcode = ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2; > + fmt.fmt_blk.fmt_blk_size = sizeof(fmt) - sizeof(fmt.hdr) - > + sizeof(fmt.fmt_blk); > + fmt.num_channels = channels; > + fmt.bits_per_sample = bits_per_sample; > + fmt.sample_rate = rate; > + fmt.is_signed = 1; > + > + channel_mapping = fmt.channel_mapping; > + > + if (use_default_chmap) { Passing NULL as channel_map would probably be a nicer way to say this, instead of having a separate bool. > + if (q6dsp_map_channels(channel_mapping, channels)) { > + dev_err(ac->dev, " map channels failed %d\n", channels); > + return -EINVAL; > + } > + } else { > + memcpy(channel_mapping, channel_map, > + PCM_FORMAT_MAX_NUM_CHANNEL); > + } > + > + rc = apr_send_pkt(ac->adev, (uint32_t *) &fmt); > + if (rc < 0) > + goto fail_cmd; > + > + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ); > + if (!rc) { > + dev_err(ac->dev, "timeout on format update\n"); > + return -ETIMEDOUT; > + } > + if (ac->cmd_state > 0) > + return adsp_err_get_lnx_err_code(ac->cmd_state); > + > + return 0; > +fail_cmd: > + return rc; > +} > +EXPORT_SYMBOL_GPL(q6asm_media_format_block_multi_ch_pcm); > + > +/** > + * q6asm_write_nolock() - non blocking write > + * > + * @ac: audio client pointer > + * @len: lenght in bytes > + * @msw_ts: timestamp msw > + * @lsw_ts: timestamp lsw > + * @flags: flags associated with write > + * > + * Return: Will be an negative value on error or zero on success > + */ > +int q6asm_write_nolock(struct audio_client *ac, uint32_t len, uint32_t msw_ts, > + uint32_t lsw_ts, uint32_t flags) q6asm_write_async() is probably a better name, nolock indicates some relationship to mutual exclusions... > +{ > + struct asm_data_cmd_write_v2 write; > + struct audio_port_data *port; > + struct audio_buffer *ab; > + int dsp_buf = 0; > + int rc = 0; > + > + if (ac->io_mode & SYNC_IO_MODE) { Bail early if this isn't true, to save you the indentation level. > + port = &ac->port[SNDRV_PCM_STREAM_PLAYBACK]; > + q6asm_add_hdr(ac, &write.hdr, sizeof(write), false, > + ac->stream_id); > + > + dsp_buf = port->dsp_buf; > + ab = &port->buf[dsp_buf]; So we're just unconditionally telling the remote side about the next buf in our ring buffer. Do we need to ensure that this is available/ready? > + > + write.hdr.token = port->dsp_buf; > + write.hdr.opcode = ASM_DATA_CMD_WRITE_V2; > + write.buf_addr_lsw = lower_32_bits(ab->phys); > + write.buf_addr_msw = upper_32_bits(ab->phys); > + write.buf_size = len; > + write.seq_id = port->dsp_buf; > + write.timestamp_lsw = lsw_ts; > + write.timestamp_msw = msw_ts; > + write.mem_map_handle = > + ac->port[SNDRV_PCM_STREAM_PLAYBACK].mem_map_handle; > + > + if (flags == NO_TIMESTAMP) > + write.flags = (flags & 0x800000FF); Fill in the constant and this becomes if flags == 0xff00: write.flags = 0xff00 & 0x800000ff; Or in other words: if flags == 0xff00: write.flags = 0; > + else > + write.flags = (0x80000000 | flags); Drop the parenthesis and flip the |. It would be nice to have a define or a comment indicating what BIT(31) is... > + > + port->dsp_buf++; > + > + if (port->dsp_buf >= port->max_buf_cnt) > + port->dsp_buf = 0; > + > + rc = apr_send_pkt(ac->adev, (uint32_t *) &write); > + if (rc < 0) > + return rc; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(q6asm_write_nolock); > + > +static void q6asm_reset_buf_state(struct audio_client *ac) > +{ > + int cnt = 0; > + int loopcnt = 0; > + int used; > + struct audio_port_data *port = NULL; > + > + if (ac->io_mode & SYNC_IO_MODE) { > + used = (ac->io_mode & TUN_WRITE_IO_MODE ? 1 : 0); > + mutex_lock(&ac->cmd_lock); > + for (loopcnt = 0; loopcnt <= SNDRV_PCM_STREAM_CAPTURE; > + loopcnt++) { > + port = &ac->port[loopcnt]; > + cnt = port->max_buf_cnt - 1; > + port->dsp_buf = 0; > + while (cnt >= 0) { > + if (!port->buf) > + continue; > + port->buf[cnt].used = used; > + cnt--; > + } > + } > + mutex_unlock(&ac->cmd_lock); > + } > +} > + > +static int __q6asm_cmd(struct audio_client *ac, int cmd, bool wait) > +{ > + int stream_id = ac->stream_id; > + struct apr_hdr hdr; > + int rc; > + > + q6asm_add_hdr(ac, &hdr, sizeof(hdr), true, stream_id); > + ac->cmd_state = -1; Resetting cmd_state relates to the send, don't mix it with building the packet. > + switch (cmd) { > + case CMD_PAUSE: > + hdr.opcode = ASM_SESSION_CMD_PAUSE; > + break; > + case CMD_SUSPEND: > + hdr.opcode = ASM_SESSION_CMD_SUSPEND; > + break; > + case CMD_FLUSH: > + hdr.opcode = ASM_STREAM_CMD_FLUSH; > + break; > + case CMD_OUT_FLUSH: > + hdr.opcode = ASM_STREAM_CMD_FLUSH_READBUFS; > + break; > + case CMD_EOS: > + hdr.opcode = ASM_DATA_CMD_EOS; > + ac->cmd_state = 0; > + break; > + case CMD_CLOSE: > + hdr.opcode = ASM_STREAM_CMD_CLOSE; > + break; > + default: > + return -EINVAL; > + } > + > + rc = apr_send_pkt(ac->adev, (uint32_t *) &hdr); > + if (rc < 0) > + return rc; > + > + if (!wait) > + return 0; I've asked you to split the others into _sync() vs _async() operations. One particular concern I have is that I don't see any mutual exclusion protecting the cmd_state and a call with !wait will overwrite the existing value, which might be unexpected. > + > + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ); > + if (!rc) { > + dev_err(ac->dev, "timeout response for opcode[0x%x]\n", > + hdr.opcode); > + return -ETIMEDOUT; > + } > + if (ac->cmd_state > 0) > + return adsp_err_get_lnx_err_code(ac->cmd_state); > + > + if (cmd == CMD_FLUSH) > + q6asm_reset_buf_state(ac); > + > + return 0; > +} [..] > diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h > index e1409c368600..b4896059da79 100644 > --- a/sound/soc/qcom/qdsp6/q6asm.h > +++ b/sound/soc/qcom/qdsp6/q6asm.h > @@ -2,7 +2,34 @@ > #ifndef __Q6_ASM_H__ > #define __Q6_ASM_H__ > > +/* ASM client callback events */ > +#define CMD_PAUSE 0x0001 These defines has rather generic names... [..] > + > +#define MSM_FRONTEND_DAI_MULTIMEDIA1 0 > +#define MSM_FRONTEND_DAI_MULTIMEDIA2 1 > +#define MSM_FRONTEND_DAI_MULTIMEDIA3 2 > +#define MSM_FRONTEND_DAI_MULTIMEDIA4 3 > +#define MSM_FRONTEND_DAI_MULTIMEDIA5 4 > +#define MSM_FRONTEND_DAI_MULTIMEDIA6 5 > +#define MSM_FRONTEND_DAI_MULTIMEDIA7 6 > +#define MSM_FRONTEND_DAI_MULTIMEDIA8 7 > + > #define MAX_SESSIONS 16 > +#define NO_TIMESTAMP 0xFF00 > +#define FORMAT_LINEAR_PCM 0x0000 Ditto. Regards, Bjorn