Message-ID: <4d1d17df-71a4-2896-29c1-9d033a2f3711@linaro.org>
Date:   Wed, 3 Jan 2018 16:26:52 +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, Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org,
        Banajit Goswami <bgoswami@...eaurora.org>,
        linux-kernel@...r.kernel.org, Patrick Lai <plai@...eaurora.org>,
        Takashi Iwai <tiwai@...e.com>, sboyd@...eaurora.org,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>, linux-soc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [RESEND PATCH v2 07/15] ASoC: qcom: q6asm: Add support to memory
 map and unmap

Thanks for your review comments.

On 02/01/18 05:48, 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 memory map and unmap regions commands in
>> q6asm module.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.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

[...]

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

I can rename it to buf_cnt if it makes it more readable.


> 
>> +	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.
>
yep, will try that.

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

This should actually go into the next patch.

> 
>> +				 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.
> 
yep!

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

>> +		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?
> 
No, caller should check the return value of this to verify that its 
mapped or not.

>> +
>> +	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?
> 
It would enter into unmap path, as cnt  would be 0 for 1 period.

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

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

will give that a go.

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

> 
>> +				      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.
yes, I agree.
> 
> 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.
> 
yep.
> 
>> +
>> +	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.
> 
yep, will fix that in next version.

>> +		++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.
> 
I will add a note in doc about this.
> 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.
> 
yep.

>> +{
>> +	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.
> 
yep.

>> +
>> +	buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL);
> 
> Loose a few of those parenthesis and use *buf in the sizeof.
> 
yes

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

We can get rid of this, it looks like leftover from old code.

> 
>> +	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.
yep!

> 
>> +		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".
> 
yes.

>> +	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.
>
I will revisit such instances where the buf is not protected.


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

>> +		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.
I agree, will fix such instances.
> 
>> +			}
>> +
>> +			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
> 

Powered by blists - more mailing lists