| 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
| ||
|
Message-ID: <4a4aff7e-84a9-95a7-c82c-d2eb0aa5d220@linaro.org>
Date: Wed, 3 Jan 2018 16:26:57 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Andy Gross <andy.gross@...aro.org>,
Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
alsa-devel@...a-project.org, David Brown <david.brown@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Liam Girdwood <lgirdwood@...il.com>,
Patrick Lai <plai@...eaurora.org>,
Banajit Goswami <bgoswami@...eaurora.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, linux-soc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, sboyd@...eaurora.org
Subject: Re: [RESEND PATCH v2 08/15] ASoC: qcom: q6asm: add support to audio
stream apis
Thanks for your comments.
On 02/01/18 20:08, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@...aro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>>
>> This patch adds support to open, write and media format commands
>> in the q6asm module.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> ---
>> 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 <linux/soc/qcom/apr.h>
>> #include <linux/device.h>
>> #include <linux/platform_device.h>
>> +#include <uapi/sound/asound.h>
>> #include <linux/delay.h>
>> #include <linux/slab.h>
>> #include <linux/mm.h>
>> #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
Sure I will try that.
>
> [..]
>>
>> +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?
May be q6asm_stream_callback/q6asm_session_callback() should be better.
>
>> + 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.
Yep, will cleanup these instances.
>
>> + token = data->token;
>> + switch (payload[0]) {
>
> This is again that common response struct.
>
yep!
[...]
>> +
>> + 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.
>
yes
>>
>> 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.
>
Will clean this up.
>> + 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.
will do that with all the apr drivers.
>
>> +
>> + 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?
No, will clean it up.
>
>> + 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.
sure.
>
>> + 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_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.
yep.
>
>> +
>> + 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.
I will give it a go and see.
>
>> + 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...
>
yep.
>> +{
>> + 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.
>
yep.
>> + 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?
>
This is already synchronized at the top layer in q6asm_dai driver.
>> +
>> + 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...
sure, I will make add more information here on the flag and also cleanup
as suggested.
>
>> +
>> + 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 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.
>
Sure.
>> + 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.
yes, this will be issue, we could move setting cmd_state to here.
Also I will revisit _sync() function to make sure that these are
sequenced correctly and async are not touching the cmd_state.
>
>> +
>> + 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...
I can prefix them with Q6ASM to make it much more specific to Q6ASM service.
>
> [..]
>> +
>> +#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
>
Powered by blists - more mailing lists