lkml.org 
[lkml]   [2018]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RESEND PATCH v2 07/15] ASoC: qcom: q6asm: Add support to memory map and unmap
    On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote:

    > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
    >
    > This patch adds support to memory map and unmap regions commands in
    > q6asm module.
    >
    > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
    > ---
    > sound/soc/qcom/qdsp6/q6asm.c | 343 ++++++++++++++++++++++++++++++++++++++++++-
    > sound/soc/qcom/qdsp6/q6asm.h | 5 +
    > 2 files changed, 347 insertions(+), 1 deletion(-)
    >
    > diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
    > index 9cc583afef4d..4be92441f524 100644
    > --- a/sound/soc/qcom/qdsp6/q6asm.c
    > +++ b/sound/soc/qcom/qdsp6/q6asm.c
    > @@ -14,9 +14,46 @@
    > #include "q6asm.h"
    > #include "common.h"
    >
    > +#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 TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */
    > #define SYNC_IO_MODE 0x0001
    > #define ASYNC_IO_MODE 0x0002
    > +#define ASM_SHIFT_GAPLESS_MODE_FLAG 31
    > +#define ADSP_MEMORY_MAP_SHMEM8_4K_POOL 3
    > +
    > +struct avs_cmd_shared_mem_map_regions {
    > + struct apr_hdr hdr;
    > + u16 mem_pool_id;
    > + u16 num_regions;
    > + u32 property_flag;
    > +} __packed;
    > +
    > +struct avs_shared_map_region_payload {
    > + u32 shm_addr_lsw;
    > + u32 shm_addr_msw;
    > + u32 mem_size_bytes;
    > +} __packed;
    > +
    > +struct avs_cmd_shared_mem_unmap_regions {
    > + struct apr_hdr hdr;
    > + u32 mem_map_handle;
    > +} __packed;
    > +
    > +struct audio_buffer {
    > + dma_addr_t phys;
    > + uint32_t used;
    > + uint32_t size; /* size of buffer */
    > +};
    > +
    > +struct audio_port_data {
    > + struct audio_buffer *buf;
    > + uint32_t max_buf_cnt;

    This seems to denote the number of audio_buffers in the buf array, so
    I'm not sure about the meaning of "max_".

    > + uint32_t dsp_buf;
    > + uint32_t mem_map_handle;
    > +};
    >
    > struct audio_client {
    > int session;
    > @@ -27,6 +64,8 @@ struct audio_client {
    > uint64_t time_stamp;
    > struct apr_device *adev;
    > struct mutex cmd_lock;
    > + /* idx:1 out port, 0: in port */
    > + struct audio_port_data port[2];
    > wait_queue_head_t cmd_wait;
    > int perf_mode;
    > int stream_id;
    > @@ -86,6 +125,260 @@ static void q6asm_session_free(struct audio_client *ac)
    > mutex_unlock(&a->session_lock);
    > }
    >
    > +static inline void q6asm_add_mmaphdr(struct audio_client *ac,
    > + struct apr_hdr *hdr, u32 pkt_size,
    > + bool cmd_flg, u32 token)

    cmd_flg is true in both callers, so this function ends up simply
    assigning hdr_field, pkt_size and token. Inlining these assignments
    would make for cleaner call sites as well.

    > +{
    > + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD;
    > + hdr->src_port = 0;
    > + hdr->dest_port = 0;
    > + hdr->pkt_size = pkt_size;
    > + if (cmd_flg)
    > + hdr->token = token;
    > +}
    > +
    > +static inline void q6asm_add_hdr(struct audio_client *ac, struct apr_hdr *hdr,

    This is unused.

    > + uint32_t pkt_size, bool cmd_flg,
    > + uint32_t stream_id)
    > +{
    > + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD;
    > + hdr->src_svc = ac->adev->svc_id;
    > + hdr->src_domain = APR_DOMAIN_APPS;
    > + hdr->dest_svc = APR_SVC_ASM;
    > + hdr->dest_domain = APR_DOMAIN_ADSP;
    > + hdr->src_port = ((ac->session << 8) & 0xFF00) | (stream_id);
    > + hdr->dest_port = ((ac->session << 8) & 0xFF00) | (stream_id);
    > + hdr->pkt_size = pkt_size;
    > + if (cmd_flg)
    > + hdr->token = ac->session;
    > +}
    > +
    > +static int __q6asm_memory_unmap(struct audio_client *ac,
    > + phys_addr_t buf_add, int dir)
    > +{
    > + struct avs_cmd_shared_mem_unmap_regions mem_unmap;

    If you name this "cmd" you will declutter below code a bit.

    > + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
    > + int rc;
    > +
    > + if (!a)
    > + return -ENODEV;

    Does this NULL check add any real value?

    > +
    > + q6asm_add_mmaphdr(ac, &mem_unmap.hdr, sizeof(mem_unmap), true,
    > + ((ac->session << 8) | dir));
    > + a->mem_state = -1;
    > +
    > + mem_unmap.hdr.opcode = ASM_CMD_SHARED_MEM_UNMAP_REGIONS;
    > + mem_unmap.mem_map_handle = ac->port[dir].mem_map_handle;
    > +
    > + if (mem_unmap.mem_map_handle == 0) {

    Start the function by checking for !ac->port[dir].mem_map_handle

    > + dev_err(ac->dev, "invalid mem handle\n");
    > + return -EINVAL;
    > + }
    > +
    > + rc = apr_send_pkt(a->adev, (uint32_t *) &mem_unmap);
    > + if (rc < 0)
    > + return rc;
    > +
    > + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0),
    > + 5 * HZ);
    > + if (!rc) {
    > + dev_err(ac->dev, "CMD timeout for memory_unmap 0x%x\n",
    > + mem_unmap.mem_map_handle);
    > + return -ETIMEDOUT;
    > + } else if (a->mem_state > 0) {
    > + return adsp_err_get_lnx_err_code(a->mem_state);
    > + }
    > + ac->port[dir].mem_map_handle = 0;

    Does all errors indicate that the region is left mapped?

    > +
    > + return 0;
    > +}
    > +
    > +/**
    > + * q6asm_unmap_memory_regions() - unmap memory regions in the dsp.
    > + *
    > + * @dir: direction of audio stream
    > + * @ac: audio client instanace
    > + *
    > + * Return: Will be an negative value on failure or zero on success
    > + */
    > +int q6asm_unmap_memory_regions(unsigned int dir, struct audio_client *ac)
    > +{
    > + struct audio_port_data *port;
    > + int cnt = 0;
    > + int rc = 0;
    > +
    > + mutex_lock(&ac->cmd_lock);
    > + port = &ac->port[dir];
    > + if (!port->buf) {
    > + mutex_unlock(&ac->cmd_lock);
    > + return 0;

    Put a label right before the mutex_unlock below and return rc instead of
    0, then you can replace these two lines with "goto unlock"

    > + }
    > + cnt = port->max_buf_cnt - 1;

    What if we mapped 1 period? Why shouldn't we enter the unmap path?

    > + if (cnt >= 0) {
    > + rc = __q6asm_memory_unmap(ac, port->buf[dir].phys, dir);
    > + if (rc < 0) {
    > + dev_err(ac->dev, "%s: Memory_unmap_regions failed %d\n",
    > + __func__, rc);

    Most of the code paths through __q6asm_memory_unmap() will print an
    error, make this consistent and print the warning once.

    > + mutex_unlock(&ac->cmd_lock);
    > + return rc;

    Same here.

    > + }
    > + }
    > +
    > + port->max_buf_cnt = 0;
    > + kfree(port->buf);
    > + port->buf = NULL;
    > + mutex_unlock(&ac->cmd_lock);

    I think however that if you rearrange this function somewhat you can
    start off by doing:

    mutex_lock(&ac->cmd_lock);
    port = &ac->port[dir];

    bufs = port->buf;
    cnt = port->max_buf_cnt;

    port->max_buf_cnt = 0;
    port->buf = NULL;
    mutex_unlock(&ac->cmd_lock);

    And then you can do the rest without locks.

    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(q6asm_unmap_memory_regions);
    > +
    > +static int __q6asm_memory_map_regions(struct audio_client *ac, int dir,
    > + uint32_t period_sz, uint32_t periods,

    period_sz is typical size_t material.

    > + bool is_contiguous)
    > +{
    > + struct avs_cmd_shared_mem_map_regions *mmap_regions = NULL;

    Calling this "cmd" would declutter the function.

    > + struct avs_shared_map_region_payload *mregions = NULL;
    > + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
    > + struct audio_port_data *port = NULL;
    > + struct audio_buffer *ab = NULL;
    > + void *mmap_region_cmd = NULL;

    No need to initialize this.

    Also, this really is a avs_cmd_shared_mem_map_regions with some extra
    data at the end, not a void *.

    > + void *payload = NULL;
    > + int rc = 0;
    > + int i = 0;
    > + int cmd_size = 0;

    Most of these can be left uninitialized.

    > + uint32_t num_regions;
    > + uint32_t buf_sz;
    > +
    > + if (!a)
    > + return -ENODEV;
    > + num_regions = is_contiguous ? 1 : periods;
    > + buf_sz = is_contiguous ? (period_sz * periods) : period_sz;
    > + buf_sz = PAGE_ALIGN(buf_sz);
    > +
    > + cmd_size = sizeof(*mmap_regions) + (sizeof(*mregions) * num_regions);
    > +
    > + mmap_region_cmd = kzalloc(cmd_size, GFP_KERNEL);
    > + if (!mmap_region_cmd)
    > + return -ENOMEM;
    > +
    > + mmap_regions = (struct avs_cmd_shared_mem_map_regions *)mmap_region_cmd;
    > + q6asm_add_mmaphdr(ac, &mmap_regions->hdr, cmd_size, true,
    > + ((ac->session << 8) | dir));
    > + a->mem_state = -1;
    > +
    > + mmap_regions->hdr.opcode = ASM_CMD_SHARED_MEM_MAP_REGIONS;
    > + mmap_regions->mem_pool_id = ADSP_MEMORY_MAP_SHMEM8_4K_POOL;
    > + mmap_regions->num_regions = num_regions;
    > + mmap_regions->property_flag = 0x00;
    > +
    > + payload = ((u8 *) mmap_region_cmd +
    > + sizeof(struct avs_cmd_shared_mem_map_regions));

    mmap_region_cmd is void *, so no need to type cast.


    > +
    > + mregions = (struct avs_shared_map_region_payload *)payload;

    Payload is void *, so no need to type cast. But there's also no benefit
    of having "payload", as this line can be written as:

    mregions = mmap_region_cmd + sizeof(*mmap_regions);


    But adding a flexible array member to the avs_cmd_shared_mem_map_regions
    struct would make things even clearer, in particular you would be able
    to read the struct definition and see that there's a payload following
    the command.

    > +
    > + ac->port[dir].mem_map_handle = 0;

    Isn't this already 0?

    > + port = &ac->port[dir];
    > +
    > + for (i = 0; i < num_regions; i++) {
    > + ab = &port->buf[i];
    > + mregions->shm_addr_lsw = lower_32_bits(ab->phys);
    > + mregions->shm_addr_msw = upper_32_bits(ab->phys);
    > + mregions->mem_size_bytes = buf_sz;

    Here you're dereferencing port->buf without holding cmd_lock.

    > + ++mregions;
    > + }
    > +
    > + rc = apr_send_pkt(a->adev, (uint32_t *) mmap_region_cmd);
    > + if (rc < 0)
    > + goto fail_cmd;
    > +
    > + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0),
    > + 5 * HZ);
    > + if (!rc) {
    > + dev_err(ac->dev, "timeout. waited for memory_map\n");
    > + rc = -ETIMEDOUT;
    > + goto fail_cmd;
    > + }
    > +
    > + if (a->mem_state > 0) {
    > + rc = adsp_err_get_lnx_err_code(a->mem_state);
    > + goto fail_cmd;
    > + }
    > + rc = 0;
    > +fail_cmd:
    > + kfree(mmap_region_cmd);
    > + return rc;
    > +}
    > +
    > +/**
    > + * q6asm_map_memory_regions() - map memory regions in the dsp.
    > + *
    > + * @dir: direction of audio stream

    This sounds boolean, perhaps worth mentioning here if true means rx or
    tx.

    And it's idiomatic to have such a parameter later in the list, it would
    probably be more natural to read the call sight if the order was:

    q6asm_map_memory_regions(ac, phys, periods, size, true);

    > + * @ac: audio client instanace
    > + * @phys: physcial address that needs mapping.
    > + * @period_sz: audio period size
    > + * @periods: number of periods
    > + *
    > + * Return: Will be an negative value on failure or zero on success
    > + */
    > +int q6asm_map_memory_regions(unsigned int dir, struct audio_client *ac,
    > + dma_addr_t phys,
    > + unsigned int period_sz, unsigned int periods)

    period_sz could with benefit be of type size_t.

    > +{
    > + struct audio_buffer *buf;
    > + int cnt;
    > + int rc;
    > +
    > + if (ac->port[dir].buf) {
    > + dev_err(ac->dev, "Buffer already allocated\n");
    > + return 0;
    > + }
    > +
    > + mutex_lock(&ac->cmd_lock);

    I believe this lock should cover above check.

    > +
    > + buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL);

    Loose a few of those parenthesis and use *buf in the sizeof.

    > + if (!buf) {
    > + mutex_unlock(&ac->cmd_lock);
    > + return -ENOMEM;
    > + }
    > +
    > +
    > + ac->port[dir].buf = buf;
    > +
    > + buf[0].phys = phys;
    > + buf[0].used = dir ^ 1;

    Why would this be called "used", and it's probably cleaner to just use
    !!dir.

    > + buf[0].size = period_sz;
    > + cnt = 1;
    > + while (cnt < periods) {

    cnt goes from 1 to periods and is incremented 1 each step, this would be
    more succinct as a for loop.

    > + if (period_sz > 0) {
    > + buf[cnt].phys = buf[0].phys + (cnt * period_sz);
    > + buf[cnt].used = dir ^ 1;
    > + buf[cnt].size = period_sz;
    > + }
    > + cnt++;
    > + }
    > +
    > + ac->port[dir].max_buf_cnt = periods;
    > + mutex_unlock(&ac->cmd_lock);

    The only possible purpose of taking cmd_lock here is to protect
    ac->port[dir].buf, but

    > +
    > + rc = __q6asm_memory_map_regions(ac, dir, period_sz, periods, 1);

    The last parameter should be "true".

    > + if (rc < 0) {
    > + dev_err(ac->dev,
    > + "CMD Memory_map_regions failed %d for size %d\n", rc,
    > + period_sz);
    > +
    > +
    > + ac->port[dir].max_buf_cnt = 0;
    > + kfree(buf);
    > + ac->port[dir].buf = NULL;

    These operations are done without holding cmd_lock.

    > +
    > + return rc;
    > + }
    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(q6asm_map_memory_regions);
    > +
    > /**
    > * q6asm_audio_client_free() - Freee allocated audio client
    > *
    > @@ -117,8 +410,10 @@ static struct audio_client *q6asm_get_audio_client(struct q6asm *a,
    >
    > static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
    > {
    > - struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
    > + struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev);
    > struct audio_client *ac = NULL;
    > + struct audio_port_data *port;
    > + uint32_t dir = 0;
    > uint32_t sid = 0;
    > uint32_t *payload;
    >
    > @@ -135,6 +430,52 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *
    > return 0;
    > }
    >
    > + a = dev_get_drvdata(ac->dev->parent);
    > + if (data->opcode == APR_BASIC_RSP_RESULT) {

    This is a case in below switch statement.

    > + switch (payload[0]) {
    > + case ASM_CMD_SHARED_MEM_MAP_REGIONS:
    > + case ASM_CMD_SHARED_MEM_UNMAP_REGIONS:
    > + if (payload[1] != 0) {
    > + dev_err(ac->dev,
    > + "cmd = 0x%x returned error = 0x%x sid:%d\n",
    > + payload[0], payload[1], sid);
    > + a->mem_state = payload[1];
    > + } else {
    > + a->mem_state = 0;

    Just assign a->mem_state = payload[1] outside the conditional, as it
    will be the same value.

    > + }
    > +
    > + wake_up(&a->mem_wait);
    > + break;
    > + default:
    > + dev_err(&adev->dev, "command[0x%x] not expecting rsp\n",
    > + payload[0]);
    > + break;
    > + }
    > + return 0;
    > + }

    Regards,
    Bjorn

    \
     
     \ /
      Last update: 2018-01-02 06:50    [W:4.627 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site