On 29-09-20, 10:54, Takashi Iwai wrote:

On Tue, 29 Sep 2020 10:40:51 +0200, Gyeongtaek Lee wrote:

...

On 9/29/20 04:13 PM, Takashi Iwai wrote:

...

On Tue, 29 Sep 2020 03:51:35 +0200, Gyeongtaek Lee wrote:

...

On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:

...

On 9/28/20 6:13 AM, Jaroslav Kysela wrote:

...

Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a): > With a stream with low bitrate, user can't pause or resume the stream > near the end of the stream because current ALSA doesn't allow it. > If the stream has very low bitrate enough to store whole stream into > the buffer, user can't do anything except stop the stream and then > restart it from the first. > If pause and resume is allowed during draining, user experience can be > enhanced.

It seems that we need a new state to handle the pause + drain condition for this case.

With this proposed change, the pause state in drain is invisible.

Indeed it's be much nicer to have a new state, e..g SNDRV_PCM_STATE_DRAINING_PAUSED.

Ok. I will add the new state.

...

One concern is that states are defined in uapi/sound/asoc.h, so wouldn't this have impacts on userspace as well? We'd change the value of SNDRV_PCM_STATE_LAST.

I also agree that adding new state and increase LAST value in the header of uapi could be dangerous. So, I added it to comress_offload.h for now. It could be merged into snd_pcm_state_t in someday with big changes. Could you review the fixed patch below?

Hrm, this resulted in rather more complex changes than the original patch. It shows that introducing yet another state is no good idea for this particular case.

Since the possible application's behavior after this pause is as same as the normal pause (i.e. either resuming pause or dropping), I find it OK to take the original approach.

Thank you for the review. I think that I should resend the original patch.

Let's wait a bit for other opinions. We may add rather a new flag instead of introducing a new state, too, for example.

I was out for a week, back now ;-)

So bigger question is if kernel should handle this change or we ask userspace to handle this. Userland knows that bit rate is less so small buffer can be for longer duration so instead of sending dumb X bytes, should it not scale the buffer based on bit rate?

From that premise the partial_drain should be invoked only for last

write().

Also, I am bit skeptical for adding changes to states while we are draining (that too partial one)... is this change driving complex changes and should we push back to have this implemented better in userland..?

Also, I'm not sure about any side-effect to drivers that expect the pause only during the running state. We might need some check for a capability flag?

In anyway, the timing is bad; it's too late for 5.10 to apply such a core change. Can we postpone this for 5.11?

Yes this needs more thinking, I am still not convinced kernel should handle it!

Regards

On 01-10-20, 10:28, Pierre-Louis Bossart wrote:

...

...

...

...

Hrm, this resulted in rather more complex changes than the original patch. It shows that introducing yet another state is no good idea for this particular case.

Since the possible application's behavior after this pause is as same as the normal pause (i.e. either resuming pause or dropping), I find it OK to take the original approach.

Thank you for the review. I think that I should resend the original patch.

Let's wait a bit for other opinions. We may add rather a new flag instead of introducing a new state, too, for example.

I was out for a week, back now ;-)

So bigger question is if kernel should handle this change or we ask userspace to handle this. Userland knows that bit rate is less so small buffer can be for longer duration so instead of sending dumb X bytes, should it not scale the buffer based on bit rate?

what about variable bit-rate? or cases where you have a 'bit reservoir' in previous frames?

You really cannot in general translate bytes to time with compressed data, which is the reason we introduced the API in the first place...

Userspace *may* know the duration for specific formats or based on metadata, but in some cases the only way to know is actually to decode the data.

I suggest we keep the compressed API based on bytes and non-periodic events when the bytes were consumed/generated.

Changing the API away from bytes was not proposed so not sure...

The SM in kernel might be bit more convoluted so was wondering if we can handle this in userland. The changelog for this patch says that for test case was sending whole file, surely that is not an optimal approach. Should we allow folks to send whole file to kernel and then issue partial drain?

Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):

On Thu, 08 Oct 2020 11:49:24 +0200, Gyeongtaek Lee wrote:

...

On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:

...

...

The SM in kernel might be bit more convoluted so was wondering if we can handle this in userland. The changelog for this patch says that for test case was sending whole file, surely that is not an optimal approach.

It's rather common to have to deal with very small files, even with PCM, e.g. for notifications. It's actually a classic test case that exposes design issues in drivers, where e.g. the last part of the notification is not played.

...

Should we allow folks to send whole file to kernel and then issue partial drain?

I don't think there should be a conceptual limitation here. If the userspace knows that the last part of the file is smaller than a fragment it should be able to issue a drain (or partial drain if it's a gapless solution).

However now that I think of it, I am not sure what happens if the file is smaller than a fragment. That may very well be a limitation in the design.

Thanks for the comments.

Actually, problem can be occurred with big file also. Application usually requests draining after sending last frame. If user clicks pause button after draining, pause will be failed and the file just be played until end. If application stop and start playback for this case, start of last frame will be heard again because stop sets state to SETUP, and write is needed to set the state to PREPARED for start. If bitrate of the file is low, time stamp will be reversed and be heard weird. I also hope this problem can be handled in userspace easily but I couldn't find a way for now.

I think that this is the time that I should share fixed patch following the comments to help the discussion. Following opinions are added to the patch.

1. it's be much nicer to have a new state - Takashi

Well, it wasn't me; I'm not against the new state *iff* it would end up with cleaner code. Admittedly, the new state can be more "consistent" regarding the state transition. If we allow the PAUSE state during DRAINING, it'll lead to multiple states after resuming the pause.

...

2. We can add this state to asound.h so the user space can be updated. - Jaroslav
3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav

I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION with a change in asound.h not in compress_offload.h. Should I increase SNDRV_PCM_VERSION also?

Yes, if we really add the PCM state, it's definitely needed.

...

And what happened if I request back-porting a patch which changes VERSION to stables?

If we introduce the new change, it must be safe to the old kernels, too. The problem is only about the compatibility of the user-space program, not about the kernel.

HOWEVER: I'm still concerned by the addition of a new PCM state. Jaroslav suggested two steps approach, (1) first add the state only in the uapi header, then use (2) the new state actually. But, this doesn't help much, simply because the step 1 won't catch real bugs.

Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I guess most of code can be compiled fine. So, at the step 1, no one notices it and bothered, either. But, at the step 2, you'll hit a problem.

Adding a new state is something like to add a new color to the traffic signal. In some countries, the car turning right at a crossing doesn't have to stop at a red signal. Suppose that we want to control it, and change the international rule by introducing a new color (say magenta) signal to stop the car turning right. That'll be a big confusion because most drivers are trained for only red, green and yellow signals.

Similarly, if we add a new PCM state, every program code that deals with the PCM state may be confused by the new state. It has to be reviewed and corrected manually, because it's no syntax problem the compiler may catch.

If there is a handshake between both side, this problem is gone. We can just add another flag / ioctl / whatever to activate the new behaviour.

Jaroslav

Hi Takashi, Jaroslav,

On 10-10-20, 11:08, Takashi Iwai wrote:

On Fri, 09 Oct 2020 19:43:40 +0200, Jaroslav Kysela wrote:

...

Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):

...

On Thu, 08 Oct 2020 11:49:24 +0200, Gyeongtaek Lee wrote:

...

On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:

...

...

The SM in kernel might be bit more convoluted so was wondering if we can handle this in userland. The changelog for this patch says that for test case was sending whole file, surely that is not an optimal approach.

It's rather common to have to deal with very small files, even with PCM, e.g. for notifications. It's actually a classic test case that exposes design issues in drivers, where e.g. the last part of the notification is not played.

...

Should we allow folks to send whole file to kernel and then issue partial drain?

I don't think there should be a conceptual limitation here. If the userspace knows that the last part of the file is smaller than a fragment it should be able to issue a drain (or partial drain if it's a gapless solution).

However now that I think of it, I am not sure what happens if the file is smaller than a fragment. That may very well be a limitation in the design.

Thanks for the comments.

Actually, problem can be occurred with big file also. Application usually requests draining after sending last frame. If user clicks pause button after draining, pause will be failed and the file just be played until end. If application stop and start playback for this case, start of last frame will be heard again because stop sets state to SETUP, and write is needed to set the state to PREPARED for start. If bitrate of the file is low, time stamp will be reversed and be heard weird. I also hope this problem can be handled in userspace easily but I couldn't find a way for now.

I think that this is the time that I should share fixed patch following the comments to help the discussion. Following opinions are added to the patch.

1. it's be much nicer to have a new state - Takashi

Well, it wasn't me; I'm not against the new state *iff* it would end up with cleaner code. Admittedly, the new state can be more "consistent" regarding the state transition. If we allow the PAUSE state during DRAINING, it'll lead to multiple states after resuming the pause.

...

2. We can add this state to asound.h so the user space can be updated. - Jaroslav
3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav

I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION with a change in asound.h not in compress_offload.h. Should I increase SNDRV_PCM_VERSION also?

Yes, if we really add the PCM state, it's definitely needed.

...

And what happened if I request back-porting a patch which changes VERSION to stables?

If we introduce the new change, it must be safe to the old kernels, too. The problem is only about the compatibility of the user-space program, not about the kernel.

HOWEVER: I'm still concerned by the addition of a new PCM state. Jaroslav suggested two steps approach, (1) first add the state only in the uapi header, then use (2) the new state actually. But, this doesn't help much, simply because the step 1 won't catch real bugs.

Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I guess most of code can be compiled fine. So, at the step 1, no one notices it and bothered, either. But, at the step 2, you'll hit a problem.

Adding a new state is something like to add a new color to the traffic signal. In some countries, the car turning right at a crossing doesn't have to stop at a red signal. Suppose that we want to control it, and change the international rule by introducing a new color (say magenta) signal to stop the car turning right. That'll be a big confusion because most drivers are trained for only red, green and yellow signals.

Similarly, if we add a new PCM state, every program code that deals with the PCM state may be confused by the new state. It has to be reviewed and corrected manually, because it's no syntax problem the compiler may catch.

If there is a handshake between both side, this problem is gone. We can just add another flag / ioctl / whatever to activate the new behaviour.

That's another tricky part. We do have already some handshake in alsa-lib to determine the supported protocol. However, if a code in question is outside that influence, we can't ensure that all belonging components understand the new one. e.g. if a program uses an intermediate library, it's free from alsa-lib changes. Or, imagine some plugin.

If this were a change of the API function, we may have a better control. We may provide different versioned symbols in the worst case, too. But, an enum is essentially hard-coded, so we have no direct influence once after it's compiled.

So what if we add another state but keep it in kernel (hidden from userspace)...?

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...

Thanks

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

Thanks

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..?

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..?

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

On Mon, 12 Oct 2020 16:46:56 +0200, Jaroslav Kysela wrote:

Dne 12. 10. 20 v 16:21 Takashi Iwai napsal(a):

...

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...

The core compress code already uses the state mechanism (although internally).

Also, it's really unclear if all drivers were checked, if the pause triggers can be called from the drain state (I know it's another point, but the drivers should probably offer a flag that they support this). And why to call the pause release callback when there's no pause (drain + release ioctl instead drain + pause + release ioctl)? It's a clear midlevel code fault. This protection should be there not in the hw drivers.

I refer the original patch: https://lore.kernel.org/alsa-devel/000c01d69585$228db6b0$67a92410$@samsung.c...

Point taken, and yes, this should be handled conditionally only for the drivers that do support such a mode.

Takashi

On Mon, 26 Oct 2020 10:18:38 +0100, Gyeongtaek Lee wrote:

With a stream with low bitrate, user can't pause or resume the stream near the end of the stream because current ALSA doesn't allow it. If the stream has very low bitrate enough to store whole stream into the buffer, user can't do anything except stop the stream and then restart it from the first because most of applications call draining after sending last frame to the kernel. If pause, resume are allowed during draining, user experience can be enhanced. To prevent malfunction in HW drivers which don't support pause during draining, pause during draining will only work if HW driver enable this feature explicitly by calling snd_compr_use_pause_in_draining().

Signed-off-by: Gyeongtaek Lee gt82.lee@samsung.com Cc: stable@vger.kernel.org

Could you restart the new thread? It's been hanging too deeply and hard to read through.

Since it's the revised patch, please give the revision number (v2 or such) and show what's different from the previous patches.

About the changes:

+/**

• • snd_compr_use_pause_in_draining - Allow pause and resume in draining state
• • @substream: compress substream to set
• • Allow pause and resume in draining state.
• • Only HW driver supports this transition can call this API.
• */

+static inline void snd_compr_use_pause_in_draining(

• 			struct snd_compr_stream *substream)
  

+{

• substream->device->use_pause_in_draining = true;

+}

How to set the flag is an open question. A natural way would be to set it somehow at creating the component object, but currently there seems no way to pass any flags.

thanks,

Takashi