Re: [PATCH] ALSA: compress: allow pause and resume during draining

From: Jaroslav Kysela <perex@perex.cz>
To: Vinod Koul <vkoul@kernel.org>, Takashi Iwai <tiwai@suse.de>
Cc: 'Pierre-Louis Bossart' <pierre-louis.bossart@linux.intel.com>,
	alsa-devel@alsa-project.org, khw0178.kim@samsung.com,
	kimty@samsung.com, s47.kang@samsung.com, lgirdwood@gmail.com,
	tiwai@suse.com, hmseo@samsung.com,
	Gyeongtaek Lee <gt82.lee@samsung.com>,
	pilsun.jang@samsung.com, tkjung@samsung.com
Subject: Re: [PATCH] ALSA: compress: allow pause and resume during draining
Date: Mon, 12 Oct 2020 15:29:00 +0200	[thread overview]
Message-ID: <5b26cdd5-8a15-fa26-86af-13bfbfad5341@perex.cz> (raw)
In-Reply-To: <20201012122423.GJ2968@vkoul-mobl>

Dne 12. 10. 20 v 14:24 Vinod Koul napsal(a):
> On 12-10-20, 09:01, Takashi Iwai wrote:
>> On Mon, 12 Oct 2020 07:25:25 +0200,
> 
>>> So what if we add another state but keep it in kernel (hidden from
>>> userspace)...?
>>
>> That's fine, then it's just a kernel's business, and it should be
>> determined which one makes the code better.
>>
>> But, there are things to be considered, though:
>>
>> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
>>   This indicates that the type has to be defined in that way
>>   explicitly.
>>
>> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish.
>>
>>> Right now tinycompress does not make use of PCM streams, kernel handles
>>> these. I am not aware of any other implementation.
>>>
>>> So if the scope if within compress then it might work...
>>
>> Yes.  But currently the API uses SND_PCM_* even for the compress
>> stuff.  Changing this value means to have influence on PCM, even if
>> PCM stuff doesn't use it yet.  (At least you'd need to increase
>> SND_PCM_STATE_LAST, for example.)
>>
>> That said, if we want to change only for compress API by assuming that
>> the impact must be negligible, the first step would be to move from
>> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
>> should be compatible, but this has to be changed at first.  Then you
>> can introduce a new value there.
> 
> I think that sounds reasonable to me, we should not have used
> SNDRV_PCM_STATE_* in the first place and long term fix for this should
> be SNDRV_COMPRESS_STATE_
> 
> I will cook a patch for this

Although the impact is not high, I do think that we should enable the new
behaviour conditionally (when the user space asks for it) even if the state
values are split. I think that the whole thread is about 'how to extend the
current APIs'. The hidden way is really not so nice.

Unfortunately, there are no reserved fields in the snd_compr_params structure
for this, but I see the similarity with the 'no_wake_mode' field which
controls the driver behaviour.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.