On Mon, 12 Oct 2020 16:10:10 +0200, Jaroslav Kysela wrote: > > 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. The PCM API has an ioctl (SNDRV_PCM_IOCTL_USER_PVERSION) to tell kernel which protocol version the user-space can talk with. It's a reverse direction from SNDRV_PCM_IOCTL_PVERSION, and with this mechanism, the kernel can determine whether a specific feature can be enabled to user-space or not. I guess the compress API can introduce the same mechanism, if any conditional behavior is really mandatory. But, I doubt whether we really need to care about that; as mentioned earlier, there is little to change from the user-space side. It just pause or resume. The only difference is the resume target, and honestly speaking, there is no interest in it from user-space side. And, the rest is about the kernel internal, and this can be really done in the way of the original patch. The flow is quite simple and understandable... > > 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. For PCM, yes, the state query is a must from user-space applications, and that's the reason I've been arguing. thanks, Takashi
