Re: [alsa-devel] [PATCH 4/7] ALSA: core: Expand DMA buffer information

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: lgirdwood@gmail.com, alsa-devel@alsa-project.org,
	broonie@kernel.org, tiwai@suse.com,
	pierre-louis.bossart@linux.intel.com
Subject: Re: [alsa-devel] [PATCH 4/7] ALSA: core: Expand DMA buffer information
Date: Tue, 17 Dec 2019 13:13:36 +0100	[thread overview]
Message-ID: <22099cfc-c3a0-5c97-e508-31132dbd8a72@intel.com> (raw)
In-Reply-To: <s5h1rt32quc.wl-tiwai@suse.de>

On 2019-12-17 11:23, Takashi Iwai wrote:
> On Tue, 17 Dec 2019 10:58:48 +0100,
> Cezary Rojewski wrote:
>>
>> Update DMA buffer definition for snd_compr_runtime so it is represented
>> similarly as in snd_pcm_runtime. While at it, modify
>> snd_compr_set_runtime_buffer to account for newly added members.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   include/sound/compress_driver.h | 35 ++++++++++++++++++++++++---------
>>   1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
>> index bc88d6f964da..00f633c0c3ba 100644
>> --- a/include/sound/compress_driver.h
>> +++ b/include/sound/compress_driver.h
>> @@ -23,7 +23,6 @@ struct snd_compr_ops;
>>    * struct snd_compr_runtime: runtime stream description
>>    * @state: stream state
>>    * @ops: pointer to DSP callbacks
>> - * @dma_buffer_p: runtime dma buffer pointer
>>    * @buffer: pointer to kernel buffer, valid only when not in mmap mode or
>>    *	DSP doesn't implement copy
>>    * @buffer_size: size of the above buffer
>> @@ -34,11 +33,14 @@ struct snd_compr_ops;
>>    * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>>    * @sleep: poll sleep
>>    * @private_data: driver private data pointer
>> + * @dma_area: virtual buffer address
>> + * @dma_addr: physical buffer address (not accessible from main CPU)
>> + * @dma_bytes: size of DMA area
>> + * @dma_buffer_p: runtime dma buffer pointer
>>    */
>>   struct snd_compr_runtime {
>>   	snd_pcm_state_t state;
>>   	struct snd_compr_ops *ops;
>> -	struct snd_dma_buffer *dma_buffer_p;
>>   	void *buffer;
>>   	u64 buffer_size;
>>   	u32 fragment_size;
>> @@ -47,6 +49,11 @@ struct snd_compr_runtime {
>>   	u64 total_bytes_transferred;
>>   	wait_queue_head_t sleep;
>>   	void *private_data;
>> +
>> +	unsigned char *dma_area;
>> +	dma_addr_t dma_addr;
>> +	size_t dma_bytes;
>> +	struct snd_dma_buffer *dma_buffer_p;
> 
> Why do we need to have both dma_buffer_p and its values?
> For consistency with PCM stream?
> 
> For PCM, dma_area, dma_addr and dma_bytes are the primary data, which
> aren't necessarily set by the dma_buffer but manually set up by the
> driver.
> 
> Just wondering.

Yeah, this is simply for consistency reasons. As referred to in previous 
reply, I'd like to see compr & PCM being tightly coupled in the future 
so separate page allocation functions are not required. Whether this is 
entirely possible or not I not know yet, although I'm an optimist when 
it comes to that subject.

If it indeed comes to that, having shared concepts and consistent API 
makes it much easier for one to refactor.

Czarek
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel