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

[Jaroslav Kysela <perex@perex.cz>]

Dne 12. 10. 20 v 15:55 Vinod Koul napsal(a):
> On 12-10-20, 15:29, Jaroslav Kysela wrote:
>> 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.
> 
> I was not really thinking of exporting the states to userspace.
> Tinycompress does not use it, I do not see any uses of it to enable
> userspace with it.. Do you think it should be exposed? If so why..?

I don't think that it's required to expose the state for the compressed API to
add this new feature. I just talk about to activate the new feature
conditionally. The question is how to extend the API now.

> Worst case we add an ioctl to query the state.. the state transitions
> are anyway result of control ops on the stream
> 
> Btw what was the motivation for pcm to expose the stream states..?

The driver may change the state when underrun / overrun or an I/O error occurs
and there's also mmap write/read mode, so the traditional read/write with an
error code handling does not work here. Also, the user space should know the
state anyway, so it's better to have all parts synced.

						Jaroslav

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