RE: [PATCH v7 06/14] ASoC: Intel: catpt: PCM operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-09-21 4:08 PM, Andy Shevchenko wrote:
> On Mon, Sep 21, 2020 at 01:54:16PM +0200, Cezary Rojewski wrote:
>> DSP designed for Lynxpoint and Wildcat Point offers no dynamic topology
>> i.e. all pipelines are already defined within firmware and host is
>> relegated to allocing stream for predefined pins. This is represented by
>> 'catpt_topology' member.
>>
...

>> +
>> +static void catpt_arrange_page_table(struct snd_pcm_substream *substream,
>> +				     struct snd_dma_buffer *pgtbl)
>> +{
>> +	struct snd_pcm_runtime *rtm = substream->runtime;
>> +	struct snd_dma_buffer *databuf = snd_pcm_get_dma_buf(substream);
>> +	int i, pages;
>> +
>> +	pages = snd_sgbuf_aligned_pages(rtm->dma_bytes);
>> +
>> +	for (i = 0; i < pages; i++) {
>> +		u32 pfn, offset;
>> +		u32 *page_table;
>> +
>> +		pfn = snd_sgbuf_get_addr(databuf, i * PAGE_SIZE) >> PAGE_SHIFT;
> 
> PFN_DOWN()
> 
Ack. Updating in both cases where explicit right shift by PAGE_SHIFT is used.

>> +		/* incrementing by 2 on even and 3 on odd */
>> +		offset = ((i << 2) + i) >> 1;
>> +		page_table = (u32 *)(pgtbl->area + offset);
>> +
>> +		if (i & 1)
>> +			*page_table |= (pfn << 4);
>> +		else
>> +			*page_table |= pfn;
>> +	}
>> +}
>> +
>> +static u32 catpt_get_channel_map(enum catpt_channel_config config)
>> +{
>> +	switch (config) {
>> +	case CATPT_CHANNEL_CONFIG_MONO:
>> +		return (0xFFFFFFF0 | CATPT_CHANNEL_CENTER);
>> +
>> +	case CATPT_CHANNEL_CONFIG_STEREO:
>> +		return (0xFFFFFF00 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_RIGHT << 4));
>> +
>> +	case CATPT_CHANNEL_CONFIG_2_POINT_1:
>> +		return (0xFFFFF000 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_RIGHT << 4)
>> +				   | (CATPT_CHANNEL_LFE << 8));
>> +
>> +	case CATPT_CHANNEL_CONFIG_3_POINT_0:
>> +		return (0xFFFFF000 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_CENTER << 4)
>> +				   | (CATPT_CHANNEL_RIGHT << 8));
>> +
>> +	case CATPT_CHANNEL_CONFIG_3_POINT_1:
>> +		return (0xFFFF0000 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_CENTER << 4)
>> +				   | (CATPT_CHANNEL_RIGHT << 8)
>> +				   | (CATPT_CHANNEL_LFE << 12));
>> +
>> +	case CATPT_CHANNEL_CONFIG_QUATRO:
>> +		return (0xFFFF0000 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_RIGHT << 4)
>> +				   | (CATPT_CHANNEL_LEFT_SURROUND << 8)
>> +				   | (CATPT_CHANNEL_RIGHT_SURROUND << 12));
>> +
>> +	case CATPT_CHANNEL_CONFIG_4_POINT_0:
>> +		return (0xFFFF0000 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_CENTER << 4)
>> +				   | (CATPT_CHANNEL_RIGHT << 8)
>> +				   | (CATPT_CHANNEL_CENTER_SURROUND << 12));
>> +
>> +	case CATPT_CHANNEL_CONFIG_5_POINT_0:
>> +		return (0xFFF00000 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_CENTER << 4)
>> +				   | (CATPT_CHANNEL_RIGHT << 8)
>> +				   | (CATPT_CHANNEL_LEFT_SURROUND << 12)
>> +				   | (CATPT_CHANNEL_RIGHT_SURROUND << 16));
>> +
>> +	case CATPT_CHANNEL_CONFIG_5_POINT_1:
>> +		return (0xFF000000 | CATPT_CHANNEL_CENTER
>> +				   | (CATPT_CHANNEL_LEFT << 4)
>> +				   | (CATPT_CHANNEL_RIGHT << 8)
>> +				   | (CATPT_CHANNEL_LEFT_SURROUND << 12)
>> +				   | (CATPT_CHANNEL_RIGHT_SURROUND << 16)
>> +				   | (CATPT_CHANNEL_LFE << 20));
>> +
>> +	case CATPT_CHANNEL_CONFIG_DUAL_MONO:
>> +		return (0xFFFFFF00 | CATPT_CHANNEL_LEFT
>> +				   | (CATPT_CHANNEL_LEFT << 4));
>> +
>> +	default:
>> +		return 0xFFFFFFFF;
>> +	}
> 
> GENMASK() for all above?
> 

Ack. GENMASK + U32_MAX for the 'default' label.

>> +}
>> +
>> +static enum catpt_channel_config catpt_get_channel_config(u32 num_channels)
>> +{
>> +	switch (num_channels) {
>> +	case 6:
>> +		return CATPT_CHANNEL_CONFIG_5_POINT_1;
>> +	case 5:
>> +		return CATPT_CHANNEL_CONFIG_5_POINT_0;
>> +	case 4:
>> +		return CATPT_CHANNEL_CONFIG_QUATRO;
>> +	case 3:
>> +		return CATPT_CHANNEL_CONFIG_2_POINT_1;
>> +	case 1:
>> +		return CATPT_CHANNEL_CONFIG_MONO;
>> +	case 2:
>> +	default:
>> +		return CATPT_CHANNEL_CONFIG_STEREO;
>> +	}
>> +}
>> +
>> +static int catpt_dai_startup(struct snd_pcm_substream *substream,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct catpt_stream_template *template;
>> +	struct catpt_stream_runtime *stream;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	template = catpt_get_stream_template(substream);
>> +
>> +	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>> +	if (!stream)
>> +		return -ENOMEM;
>> +
>> +	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, cdev->dev, PAGE_SIZE,
>> +				  &stream->pgtbl);
>> +	if (ret)
>> +		goto err_pgtbl;
>> +
>> +	res = catpt_request_region(&cdev->dram, template->persistent_size);
>> +	if (!res) {
>> +		ret = -EBUSY;
>> +		goto err_request;
>> +	}
>> +
>> +	catpt_dsp_update_srampge(cdev, &cdev->dram, cdev->spec->dram_mask);
>> +
>> +	stream->template = template;
>> +	stream->persistent = res;
>> +	stream->substream = substream;
>> +	INIT_LIST_HEAD(&stream->node);
>> +	snd_soc_dai_set_dma_data(dai, substream, stream);
>> +
>> +	spin_lock(&cdev->list_lock);
>> +	list_add_tail(&stream->node, &cdev->stream_list);
>> +	spin_unlock(&cdev->list_lock);
>> +
>> +	return 0;
>> +
>> +err_request:
>> +	snd_dma_free_pages(&stream->pgtbl);
>> +err_pgtbl:
>> +	kfree(stream);
>> +	return ret;
>> +}
>> +
>> +static void catpt_dai_shutdown(struct snd_pcm_substream *substream,
>> +			       struct snd_soc_dai *dai)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct catpt_stream_runtime *stream;
>> +
>> +	stream = snd_soc_dai_get_dma_data(dai, substream);
>> +
>> +	spin_lock(&cdev->list_lock);
>> +	list_del(&stream->node);
>> +	spin_unlock(&cdev->list_lock);
>> +
>> +	release_resource(stream->persistent);
>> +	kfree(stream->persistent);
>> +	catpt_dsp_update_srampge(cdev, &cdev->dram, cdev->spec->dram_mask);
>> +
>> +	snd_dma_free_pages(&stream->pgtbl);
>> +	kfree(stream);
>> +	snd_soc_dai_set_dma_data(dai, substream, NULL);
>> +}
>> +
>> +static int catpt_dai_hw_params(struct snd_pcm_substream *substream,
>> +			       struct snd_pcm_hw_params *params,
>> +			       struct snd_soc_dai *dai)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct catpt_stream_runtime *stream;
>> +	struct catpt_audio_format afmt;
>> +	struct catpt_ring_info rinfo;
>> +	struct snd_pcm_runtime *rtm = substream->runtime;
>> +	struct snd_dma_buffer *dmab;
>> +	int ret;
>> +
>> +	stream = snd_soc_dai_get_dma_data(dai, substream);
>> +	if (stream->allocated)
>> +		return 0;
>> +
>> +	memset(&afmt, 0, sizeof(afmt));
>> +	afmt.sample_rate = params_rate(params);
>> +	afmt.bit_depth = params_physical_width(params);
>> +	afmt.valid_bit_depth = params_width(params);
>> +	afmt.num_channels = params_channels(params);
>> +	afmt.channel_config = catpt_get_channel_config(afmt.num_channels);
>> +	afmt.channel_map = catpt_get_channel_map(afmt.channel_config);
>> +	afmt.interleaving = CATPT_INTERLEAVING_PER_CHANNEL;
>> +
>> +	dmab = snd_pcm_get_dma_buf(substream);
>> +	catpt_arrange_page_table(substream, &stream->pgtbl);
>> +
>> +	memset(&rinfo, 0, sizeof(rinfo));
>> +	rinfo.page_table_addr = stream->pgtbl.addr;
>> +	rinfo.num_pages = DIV_ROUND_UP(rtm->dma_bytes, PAGE_SIZE);
>> +	rinfo.size = rtm->dma_bytes;
>> +	rinfo.offset = 0;
>> +	rinfo.ring_first_page_pfn = snd_sgbuf_get_addr(dmab, 0) >> PAGE_SHIFT;
>> +
>> +	ret = catpt_ipc_alloc_stream(cdev, stream->template->path_id,
>> +				     stream->template->type,
>> +				     &afmt, &rinfo,
>> +				     stream->template->num_entries,
>> +				     stream->template->entries,
>> +				     stream->persistent,
>> +				     cdev->scratch,
>> +				     &stream->info);
>> +	if (ret)
>> +		return CATPT_IPC_ERROR(ret);
>> +
>> +	stream->allocated = true;
>> +	return 0;
>> +}
>> +
>> +static int catpt_dai_hw_free(struct snd_pcm_substream *substream,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct catpt_stream_runtime *stream;
>> +
>> +	stream = snd_soc_dai_get_dma_data(dai, substream);
>> +	if (!stream->allocated)
>> +		return 0;
>> +
>> +	catpt_ipc_reset_stream(cdev, stream->info.stream_hw_id);
>> +	catpt_ipc_free_stream(cdev, stream->info.stream_hw_id);
>> +
>> +	stream->allocated = false;
>> +	return 0;
>> +}
>> +
>> +static int catpt_set_dspvol(struct catpt_dev *cdev, u8 stream_id, long *ctlvol);
>> +
>> +static int catpt_dai_apply_usettings(struct snd_soc_dai *dai,
>> +				     struct catpt_stream_runtime *stream)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct snd_soc_component *component = dai->component;
>> +	struct snd_kcontrol *pos, *kctl = NULL;
>> +	const char *name;
>> +	int ret;
>> +	u32 id = stream->info.stream_hw_id;
>> +
>> +	/* only selected streams have individual controls */
>> +	switch (id) {
>> +	case CATPT_PIN_ID_OFFLOAD1:
>> +		name = "Media0 Playback Volume";
>> +		break;
>> +	case CATPT_PIN_ID_OFFLOAD2:
>> +		name = "Media1 Playback Volume";
>> +		break;
>> +	case CATPT_PIN_ID_CAPTURE1:
>> +		name = "Mic Capture Volume";
>> +		break;
>> +	case CATPT_PIN_ID_REFERENCE:
>> +		name = "Loopback Mute";
>> +		break;
>> +	default:
>> +		return 0;
>> +	};
>> +
>> +	list_for_each_entry(pos, &component->card->snd_card->controls, list) {
>> +		if (pos->private_data == component &&
>> +		    !strncmp(name, pos->id.name, sizeof(pos->id.name))) {
>> +			kctl = pos;
>> +			break;
>> +		}
>> +	}
>> +	if (!kctl)
>> +		return -ENOENT;
>> +
>> +	if (stream->template->type != CATPT_STRM_TYPE_LOOPBACK)
>> +		return catpt_set_dspvol(cdev, id, (long *)kctl->private_value);
>> +	ret = catpt_ipc_mute_loopback(cdev, id, *(bool *)kctl->private_value);
>> +	if (ret)
>> +		return CATPT_IPC_ERROR(ret);
>> +	return 0;
>> +}
>> +
>> +static int catpt_dai_prepare(struct snd_pcm_substream *substream,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct catpt_stream_runtime *stream;
>> +	int ret;
>> +
>> +	stream = snd_soc_dai_get_dma_data(dai, substream);
>> +	if (stream->prepared)
>> +		return 0;
>> +
>> +	ret = catpt_ipc_reset_stream(cdev, stream->info.stream_hw_id);
>> +	if (ret)
>> +		return CATPT_IPC_ERROR(ret);
>> +
>> +	ret = catpt_ipc_pause_stream(cdev, stream->info.stream_hw_id);
>> +	if (ret)
>> +		return CATPT_IPC_ERROR(ret);
>> +
>> +	ret = catpt_dsp_update_lpclock(cdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = catpt_dai_apply_usettings(dai, stream);
>> +	if (ret)
>> +		return ret;
>> +
>> +	stream->prepared = true;
>> +	return 0;
>> +}
>> +
>> +static int catpt_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct catpt_stream_runtime *stream;
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	snd_pcm_uframes_t pos;
>> +	int ret;
>> +
>> +	stream = snd_soc_dai_get_dma_data(dai, substream);
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +		/* only offload is set_write_pos driven */
>> +		if (stream->template->type != CATPT_STRM_TYPE_RENDER)
>> +			goto resume_stream;
>> +
>> +		pos = frames_to_bytes(runtime, runtime->start_threshold);
>> +		/*
>> +		 * Dsp operates on buffer halves, thus max 2x set_write_pos
>> +		 * (entire buffer filled) prior to stream start.
>> +		 */
>> +		ret = catpt_ipc_set_write_pos(cdev, stream->info.stream_hw_id,
>> +					      pos, false, false);
>> +		if (ret)
>> +			return CATPT_IPC_ERROR(ret);
>> +		fallthrough;
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +	resume_stream:
>> +		ret = catpt_ipc_resume_stream(cdev, stream->info.stream_hw_id);
>> +		if (ret)
>> +			return CATPT_IPC_ERROR(ret);
>> +		break;
>> +
>> +	case SNDRV_PCM_TRIGGER_STOP:
>> +		stream->prepared = false;
>> +		catpt_dsp_update_lpclock(cdev);
>> +		fallthrough;
>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> +		ret = catpt_ipc_pause_stream(cdev, stream->info.stream_hw_id);
>> +		if (ret)
>> +			return CATPT_IPC_ERROR(ret);
>> +		break;
>> +
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void catpt_stream_update_position(struct catpt_dev *cdev,
>> +				  struct catpt_stream_runtime *stream,
>> +				  struct catpt_notify_position *pos)
>> +{
>> +	struct snd_pcm_substream *substream = stream->substream;
>> +	struct snd_pcm_runtime *r = substream->runtime;
>> +	snd_pcm_uframes_t dsppos, newpos;
>> +	int ret;
>> +
>> +	dsppos = bytes_to_frames(r, pos->stream_position);
>> +
>> +	/* only offload is set_write_pos driven */
>> +	if (stream->template->type != CATPT_STRM_TYPE_RENDER)
>> +		goto exit;
>> +
>> +	if (dsppos >= r->buffer_size / 2)
>> +		newpos = r->buffer_size / 2;
>> +	else
>> +		newpos = 0;
>> +	/*
>> +	 * Dsp operates on buffer halves, thus on every notify position
>> +	 * (buffer half consumed) update wp to allow stream progression.
>> +	 */
>> +	ret = catpt_ipc_set_write_pos(cdev, stream->info.stream_hw_id,
>> +				      frames_to_bytes(r, newpos),
>> +				      false, false);
>> +	if (ret) {
>> +		dev_err(cdev->dev, "update position for stream %d failed: %d\n",
>> +			stream->info.stream_hw_id, ret);
>> +		return;
>> +	}
>> +exit:
>> +	snd_pcm_period_elapsed(substream);
>> +}
>> +
>> +#define CATPT_BUFFER_MAX_SIZE	76800 /* 200ms native format */
> 
> I don't understand 'native format'. Please, give clearer comment, like
> 
> /* 200ms for 8 8-bit channels at 48kHz (native format) */
> 

Sure, will expand the description. Just so you know, it's 2/24(32)/48kHz
for LPT/WPT solution.

>> +#define CATPT_PCM_PERIODS_MAX	4
>> +#define CATPT_PCM_PERIODS_MIN	2
>> +

...

> 
>> +#define DSP_VOLUME_MAX	0x7FFFFFFF /* 0db */
> 
> S32_MAX ?
> 
Ack.

>> +static const u32 volume_map[] = {
>> +	DSP_VOLUME_MAX >> 30,
>> +	DSP_VOLUME_MAX >> 29,
>> +	DSP_VOLUME_MAX >> 28,
>> +	DSP_VOLUME_MAX >> 27,
>> +	DSP_VOLUME_MAX >> 26,
>> +	DSP_VOLUME_MAX >> 25,
>> +	DSP_VOLUME_MAX >> 24,
>> +	DSP_VOLUME_MAX >> 23,
>> +	DSP_VOLUME_MAX >> 22,
>> +	DSP_VOLUME_MAX >> 21,
>> +	DSP_VOLUME_MAX >> 20,
>> +	DSP_VOLUME_MAX >> 19,
>> +	DSP_VOLUME_MAX >> 18,
>> +	DSP_VOLUME_MAX >> 17,
>> +	DSP_VOLUME_MAX >> 16,
>> +	DSP_VOLUME_MAX >> 15,
>> +	DSP_VOLUME_MAX >> 14,
>> +	DSP_VOLUME_MAX >> 13,
>> +	DSP_VOLUME_MAX >> 12,
>> +	DSP_VOLUME_MAX >> 11,
>> +	DSP_VOLUME_MAX >> 10,
>> +	DSP_VOLUME_MAX >> 9,
>> +	DSP_VOLUME_MAX >> 8,
>> +	DSP_VOLUME_MAX >> 7,
>> +	DSP_VOLUME_MAX >> 6,
>> +	DSP_VOLUME_MAX >> 5,
>> +	DSP_VOLUME_MAX >> 4,
>> +	DSP_VOLUME_MAX >> 3,
>> +	DSP_VOLUME_MAX >> 2,
>> +	DSP_VOLUME_MAX >> 1,
>> +	DSP_VOLUME_MAX >> 0,
>> +};
> 
> Why do you have table? Does it have any gaps? Why can't be formula used?
> 

No gaps, just a lookup table. As catpt aims to keep 100% backward
compatibility with what sound/soc/intel/haswell solution exposes, it is
also useful for filling kcontrol info: catpt_volume_info() function.

>> +static u32 ctlvol_to_dspvol(u32 value)
>> +{
>> +	if (value >= ARRAY_SIZE(volume_map))
>> +		value = 0;
>> +	return volume_map[value];
>> +}
>> +
>> +static u32 dspvol_to_ctlvol(u32 volume)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(volume_map); i++)
>> +		if (volume_map[i] >= volume)
>> +			return i;
> 
>> +	return i - 1;
> 
> It seems okay in this case, but in general the code looks dangerous (for
> example, if ARRAY_SIZE is 0).
> 

Agreed, although volume_map will never change so I've decided to drop
additional safety.

...

>> +}
>> +
>> +static int catpt_set_dspvol(struct catpt_dev *cdev, u8 stream_id, long *ctlvol)
>> +{
>> +	bool all_equal = true;
>> +	u32 dspvol;
>> +	int ret, i;
> 
>> +	for (i = 1; all_equal && i < CATPT_CHANNELS_MAX; i++)
>> +		all_equal = (ctlvol[i] == ctlvol[0]);
> 
> 	for (i = 1; i < CATPT_CHANNELS_MAX; i++)
> 		if (ctlvol[i] != ctlvol[0])
> 			break;
> 
>> +	if (all_equal) {
> 
> if (i == _MAX)
> 
> and one variable less.
> 
Keen eye, thanks for this tip! Ack.


...

>> +		dspvol = ctlvol_to_dspvol(ctlvol[0]);
>> +
>> +		ret = catpt_ipc_set_volume(cdev, stream_id,
>> +					   CATPT_ALL_CHANNELS_MASK, dspvol,
>> +					   0, CATPT_AUDIO_CURVE_NONE);
>> +	} else {
>> +		for (i = 0; i < CATPT_CHANNELS_MAX; i++) {
>> +			dspvol = ctlvol_to_dspvol(ctlvol[i]);
>> +
>> +			ret = catpt_ipc_set_volume(cdev, stream_id,
>> +						   i, dspvol,
>> +						   0, CATPT_AUDIO_CURVE_NONE);
> 
>> +			if (ret)
>> +				goto exit;
> 
> Don't see necessity of this in this patch...
> 
>> +		}
>> +	}
> 
>> +exit:
> 
> ...neither this.
> 

Believe simple 'break' suffices. 'exit' label becomes redundant too.

Thanks,
Czarek





[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux