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
