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

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