Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <d7e63f52-a98e-0c99-906b-6c03b25da572@intel.com>
Date:   Fri, 14 Jan 2022 16:49:51 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.3.0
Subject: Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation
Content-Language: en-US
To:     Daniel Baluta <daniel.baluta@oss.nxp.com>, <broonie@kernel.org>,
        <alsa-devel@alsa-project.org>
CC:     <pierre-louis.bossart@linux.intel.com>, <lgirdwood@gmail.com>,
        <ranjani.sridharan@linux.intel.com>,
        <kai.vehmanen@linux.intel.com>, <daniel.baluta@nxp.com>,
        <perex@perex.cz>, <tiwai@suse.com>, <linux-kernel@vger.kernel.org>
References: <20220113161341.371345-1-daniel.baluta@oss.nxp.com>
From:   Cezary Rojewski <cezary.rojewski@intel.com>
In-Reply-To: <20220113161341.371345-1-daniel.baluta@oss.nxp.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UjdaOXZiY2swR01kR3hQKzRHek5zb0ZlSHh1K08zVjkvRHRVdURvODYzWmRB?=
 =?utf-8?B?Y3R1RmdnYVZ3aDlESE1hbHpIR0hxeFRqSC9hRnBETFVxQjVYVFA1QVR2dWRv?=
 =?utf-8?B?MVY0dVJQdzBvN1Bkam41eXMwMHFvMDlGSm80dTRYUXl5amU4d3lsZVpZVFlZ?=
 =?utf-8?B?enJubloxNmJMOW1NU3Y0VllPbSswamVxYVBxZ1FERWFhYnNNM0dsaCtNY0lN?=
 =?utf-8?B?UzEvUzFTby9aZ2xwTWFSSDYzTEJJR2dQR0pwSzE3ai9wcG9zNzhNT2l6OXkx?=
 =?utf-8?B?YjJEalFnT21CL3J1NjNoZy8rRHp2TlBOZ2hlYWhSM1ZGNForMlhwRHQ0ZE0w?=
 =?utf-8?B?ZzloQ09vS0FxR1JKQjAwUnk3UHVhc0lzWHFtWm1rR09UMFpDcXN3NU1tQ2JO?=
 =?utf-8?B?alhSRk81SW5XS2JHYXg1V0VDSWt2NzhNZSt2R1lCTngxQXdaL0lScUhVTXJV?=
 =?utf-8?B?OW1DRE5UWU13SElkbDJ2VDM3NUtWbjdsbGhzdE94dDZ2M0pwMTl0VWRsUzJx?=
 =?utf-8?B?Uk5NNWNMN1BrUmZGUG01dFBHVlRIYkZCcUR2Y3RleXVBL0NKNFBYd01LYWZy?=
 =?utf-8?B?VUNFbkNQRXZ4RW9lSzVmeCtQZ2lldkFwTCtMaDA2WDBuZEFrQTc0YS9mK3Bh?=
 =?utf-8?B?RnlyREtGaG53alRSWFFRNHB1akw3cklpSmxQKzg2V2NlZHhFRXRORFNhZTdK?=
 =?utf-8?B?MjRtY2dwSnlod0k5WXdXZzdRSmtGL3JnV3pWa0hEcHFVQ2pQY0t2a2IveDNE?=
 =?utf-8?B?ZnpGdFZ3R0xMa1ViWUltTklvYlY4VHVITUU1ZExRb2F5K25XbnlWSi8xbUp2?=
 =?utf-8?B?NFhyc1haVjczQndzSFUvRThZa0ZOeWxjTFFwTmdJYWVvQjY2Sko2bFR0eGta?=
 =?utf-8?B?WDB5VnlLQkhTQ3ZOT0dOclBPdkZrZjBmZkd0R0x0NjVRbDdkTUdGT1ZWSldv?=
 =?utf-8?B?c1RXUDYxeUhmM25HakV1Z0h1dHFFQis2Nyt0bEJqTW1pS09pRGo1MmhURi9D?=
 =?utf-8?B?Q2N1NWNSb010RUttNnZ4YVovK2EyNEVSNjZVelBZT2NVWXRxbDFHdGljYW9N?=
 =?utf-8?B?WTNHTmkwYzNnejNPWE9GUDRpOVRNVDNZditJLzZwWWgwbmNyUmx2YnJhMVpi?=
 =?utf-8?B?RkZJMnZjWkpaUVR2cURzbGwxREI1bGVLZ1AxaDZUbkVEbitXcDNKYnh0ZEYx?=
 =?utf-8?B?K29mV0ZxQ2NxSE1XSFd1Wnptd01aL29tQUVIOG9QQlk0MU1qMVBFT212L3lK?=
 =?utf-8?B?SXl2bnQwclY1M1hyWXVONlZxTis4NVNuVVBIdk44OW9ocHBsMGFDK3JSdEZt?=
 =?utf-8?B?ek1KajhFcUlTM0NUb3lNUHoyZDhQL0orSjdpQ3k2VFJZaXkwWVdYMHJGQ0Z5?=
 =?utf-8?B?T3ViVjY2d1cwSElxZ29aVjMvNXpPaEJRZGxIem9OT0Y4MWZrZ1E2U1U0VE9t?=
 =?utf-8?B?Ky90ZVVlaVUrRzhtQXE2Z2pLbHlUNDdvYWg4K2lxSVUxZk5jWUdOSk14SUVy?=
 =?utf-8?B?cHcwbUtPMXR5OTR0cHp6RnhMUVlxQkVGbzYvU0daZHpMTlI5aWk5bTN0OVhJ?=
 =?utf-8?B?alE5RVp6OW1RRFV3YlpkNnNVUmpOTFZoa1JXaFFWVm82VjFpWVNaaGVvMzNm?=
 =?utf-8?B?RFVHNm9wWC9HS2RCOVpQa2NQZy81cmV2SE9lRkVLa3hFMUJucHc5TkI4RDlJ?=
 =?utf-8?B?NzNzTHk3SUNRaE45Mkk2L210K0t1b2hlU2ZxR1dIOGRLN0p3bFBGd2w3a3U0?=
 =?utf-8?B?ZEdQZHpGNXFNc2ZKeTZLVmsvaDdMR3NCa3FiczhBeEhOVFRkVHlsc0tKNC9v?=
 =?utf-8?B?WGhZejY1eWNDMFlwVlh4eVBrOUlWS21SYU9FTGtYM2V6NWI0elF2NE5kNXE4?=
 =?utf-8?B?NEJFTVNnUm5PMjFZaFRlS1ZrVkl0OWRpbUJ5b2JiQmVQWXhqVnZTNzU5QmNJ?=
 =?utf-8?B?UzlvdEJHcXZnTkxQU3BiWnJ6RUp1QnM5YXVrQ211dlRjUHpPRFpBd2dHdlE4?=
 =?utf-8?B?RXhSNTZxcGh3a0dZZERRTFJBSlFxNlVtNFd6aU83TnpUMHlLWWMvUVovTmtY?=
 =?utf-8?B?YXRwaFVUaWRWUiswMmh2V01oM1hzSVBHellqdWpOckovZkdmNTZCaDBMUmxs?=
 =?utf-8?B?MXBjZm9YdDZqQ3NjTENHREdWNWlyZEh6TVl3ZlhuRDNWZWFscWlvVHRKd0c2?=
 =?utf-8?B?WnUvcFRwZnp4amUxQjRXcUx6RWF2eXJmZGJPUmxERTZWb3NCTWVYN0JhMHAw?=
 =?utf-8?B?QkJaaVQrWVBNY3B2YXFTSFBsbDZBPT0=?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f2753591-bc99-4aa2-685e-08d9d77584fb
X-MS-Exchange-CrossTenant-AuthSource: BN6PR11MB4049.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jan 2022 15:49:59.4608
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /xbY0DN1VmeUyWtOkUyGK9LSKQdsVd733jLf46fc6lxTr0Iepf4FsBCbARmlIt3COu7r7Fa4/H3iPkgdWD0VXNUGEeeqvaByrFatG+sQLGE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB3134
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2022-01-13 5:13 PM, Daniel Baluta wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Implement snd_compress_ops. There are a lot of similarities with
> PCM implementation.
> 
> For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
> firmware.
> 
> This will be changed in the future once we either add new compress
> parameters to SOF or enhance existing sof_ipc_pcm_params structure
> to support all native compress params.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

...

> +static int create_page_table(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     unsigned char *dma_area, size_t size)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> +	int dir = cstream->direction;
> +	struct snd_sof_pcm *spcm;

The layout of this declaration block is weird - it's neither a 
reversed-christmas-tree nor higher->lower complexity (e.g. structs -> 
primitives). Could you explain why it is shaped as is?

> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	return snd_sof_create_page_table(component->dev, dmab,
> +					 spcm->stream[dir].page_table.area, size);
> +}
> +
> +int sof_compr_open(struct snd_soc_component *component,
> +		   struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_compr_runtime *runtime = cstream->runtime;

Making use of 'rtd' and 'runtime' simultaneously within a function make 
it less readable.

> +	struct sof_compr_stream *sstream;
> +	struct snd_sof_pcm *spcm;
> +	int dir;
> +
> +	sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
> +	if (!sstream)
> +		return -ENOMEM;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm) {
> +		kfree(sstream);
> +		return -EINVAL;
> +	}
> +
> +	dir = cstream->direction;
> +
> +	if (spcm->stream[dir].cstream) {
> +		kfree(sstream);
> +		return -EBUSY;
> +	}

Could you explain why this check is needed? i.e. Is is possible for 
compress stream to be opened "twice"?

> +
> +	spcm->stream[dir].cstream = cstream;
> +	spcm->stream[dir].posn.host_posn = 0;
> +	spcm->stream[dir].posn.dai_posn = 0;
> +	spcm->prepared[dir] = false;
> +
> +	runtime->private_data = sstream;
> +
> +	return 0;
> +}
> +
> +int sof_compr_free(struct snd_soc_component *component,
> +		   struct snd_compr_stream *cstream)
> +{
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_compr_runtime *runtime = cstream->runtime;

Ditto.

> +	struct sof_compr_stream *sstream = runtime->private_data;
> +	struct sof_ipc_stream stream;
> +	struct sof_ipc_reply reply;
> +	struct snd_sof_pcm *spcm;
> +	int ret = 0;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	stream.hdr.size = sizeof(stream);
> +	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
> +	stream.comp_id = spcm->stream[cstream->direction].comp_id;
> +
> +	if (spcm->prepared[cstream->direction]) {
> +		ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
> +					 &stream, sizeof(stream),
> +					 &reply, sizeof(reply));
> +		if (!ret)
> +			spcm->prepared[cstream->direction] = false;

Why is the assignment conditional? If IPC fails, is the ->prepared flag 
respected and addressed later on? It does not seem to happen here.

> +	}
> +
> +	cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
> +	spcm->stream[cstream->direction].cstream = NULL;
> +	kfree(sstream);
> +
> +	return ret;
> +}
> +
> +int sof_compr_set_params(struct snd_soc_component *component,
> +			 struct snd_compr_stream *cstream, struct snd_compr_params *params)
> +{
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
> +	struct snd_compr_runtime *rtd = cstream->runtime;
> +	struct sof_compr_stream *sstream = rtd->private_data;
> +	struct sof_ipc_pcm_params_reply ipc_params_reply;
> +	struct sof_ipc_pcm_params pcm;
> +	struct snd_sof_pcm *spcm;
> +	int ret;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
> +	cstream->dma_buffer.dev.dev = sdev->dev;
> +	ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
> +	if (ret < 0)
> +		return ret;
> +
> +	create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);

Shouldn't the result of create_page_table() be tested before moving on?


...

> +int sof_compr_trigger(struct snd_soc_component *component,
> +		      struct snd_compr_stream *cstream, int cmd)
> +{
> +	struct sof_ipc_stream stream;
> +	struct sof_ipc_reply reply;
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_sof_pcm *spcm;

Similarly to create_page_table() case, layout of this declaration block 
is weird. Perhaps I'm just unaware of the convention used within this 
directory.


...

> +static int sof_compr_pointer(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     struct snd_compr_tstamp *tstamp)
> +{
> +	struct snd_compr_runtime *runtime = cstream->runtime;
> +	struct sof_compr_stream *sstream = runtime->private_data;

I'd suggest declaring single local variable instead. The 'runtime' one 
is unused except for the initial 'sstream' assignemnt.

> +
> +	tstamp->sampling_rate = sstream->sample_rate;
> +	tstamp->copied_total = sstream->copied_total;
> +
> +	return 0;
> +}

...

> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index 087935192ce8..d001a762a866 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
>   	SOF_DEBUGFS_ACCESS_D0_ONLY,
>   };
>   
> +struct sof_compr_stream {
> +	unsigned int copied_total;
> +	unsigned int sample_rate;
> +	size_t posn_offset;
> +};

Some streaming-related PCM structs follow snd_sof_xxx naming pattern 
instead, e.g.: snd_sof_pcm. Is the naming convention for compress 
streams seen here intentional?

> +
>   struct snd_sof_dev;
>   struct snd_sof_ipc_msg;
>   struct snd_sof_ipc;
>