Re: [PATCH v2 0/4] ALSA: compress_offload: Add 64-bit safe timestamp API
On Fri, 11 Jul 2025 13:56:47 +0200, Charles Keepax wrote:
On Fri, Jul 11, 2025 at 10:36:26AM +0100, Joris Verhaegen wrote:
The current compress offload timestamping API relies on struct snd_compr_tstamp, whose cumulative counters like copied_total are defined as __u32. On long-running high-resolution audio streams, these 32-bit counters can overflow, causing incorrect availability calculations.
This patch series introduces a parallel, 64-bit safe API to solve this problem while maintaining perfect backward compatibility with the existing UAPI. A new pointer64 operation and corresponding ioctls are added to allow the kernel to track counters using u64 and expose these full-width values to user-space.
The series is structured as follows:
Patch 1: Introduces the new internal pointer64 op, refactors the core logic to use it, and defines the new UAPI structs.
Patch 2: Exposes the SNDRV_COMPRESS_TSTAMP64 ioctl.
Patch 3: Exposes the corresponding SNDRV_COMPRESS_AVAIL64 ioctl.
Patch 4: Implements the new .pointer64 operation in various ASoC drivers that support compress offload.
This series has been tested on a Pixel 9 device. All compress offload use cases, including long-running playback, were verified to work correctly with the new 64-bit API, and no regressions were observed when using the legacy API.
Thanks, Joris (George) Verhaegen
Signed-off-by: Joris Verhaegen verhaegen@google.com
Would it not be slightly simpler to just update all the in kernel bits to use 64-bit and then only convert to 32-bit for the existing 32-bit IOCTLs? Why do we need 32-bit callbacks into the drivers for example?
Right, it's a usual pattern to have only the 64bit ops in the kernel driver side while providing the 32bit stuff converted in the core layer. Having two different ops are rather confusing and superfluous after conversions.
If there are tons of users for this API, it'd be needed to convert gradually, and eventually drop the 32bit ops at the end. But in this case, there doesn't seem so many relevant drivers, hence the conversion can be done in a shot as done in your patch 4.
thanks,
Takashi
On Fri, 11 Jul 2025 14:41:05 +0200, Takashi Iwai wrote:
On Fri, 11 Jul 2025 13:56:47 +0200, Charles Keepax wrote:
On Fri, Jul 11, 2025 at 10:36:26AM +0100, Joris Verhaegen wrote:
The current compress offload timestamping API relies on struct snd_compr_tstamp, whose cumulative counters like copied_total are defined as __u32. On long-running high-resolution audio streams, these 32-bit counters can overflow, causing incorrect availability calculations.
This patch series introduces a parallel, 64-bit safe API to solve this problem while maintaining perfect backward compatibility with the existing UAPI. A new pointer64 operation and corresponding ioctls are added to allow the kernel to track counters using u64 and expose these full-width values to user-space.
The series is structured as follows:
Patch 1: Introduces the new internal pointer64 op, refactors the core logic to use it, and defines the new UAPI structs.
Patch 2: Exposes the SNDRV_COMPRESS_TSTAMP64 ioctl.
Patch 3: Exposes the corresponding SNDRV_COMPRESS_AVAIL64 ioctl.
Patch 4: Implements the new .pointer64 operation in various ASoC drivers that support compress offload.
This series has been tested on a Pixel 9 device. All compress offload use cases, including long-running playback, were verified to work correctly with the new 64-bit API, and no regressions were observed when using the legacy API.
Thanks, Joris (George) Verhaegen
Signed-off-by: Joris Verhaegen verhaegen@google.com
Would it not be slightly simpler to just update all the in kernel bits to use 64-bit and then only convert to 32-bit for the existing 32-bit IOCTLs? Why do we need 32-bit callbacks into the drivers for example?
Right, it's a usual pattern to have only the 64bit ops in the kernel driver side while providing the 32bit stuff converted in the core layer. Having two different ops are rather confusing and superfluous after conversions.
If there are tons of users for this API, it'd be needed to convert gradually, and eventually drop the 32bit ops at the end. But in this case, there doesn't seem so many relevant drivers, hence the conversion can be done in a shot as done in your patch 4.
Also, don't forget to increase the protocol version if you change the ABI.
thanks,
Takashi
On 11-07-25, 14:41, Takashi Iwai wrote:
On Fri, 11 Jul 2025 13:56:47 +0200,
Would it not be slightly simpler to just update all the in kernel bits to use 64-bit and then only convert to 32-bit for the existing 32-bit IOCTLs? Why do we need 32-bit callbacks into the drivers for example?
Right, it's a usual pattern to have only the 64bit ops in the kernel driver side while providing the 32bit stuff converted in the core layer. Having two different ops are rather confusing and superfluous after conversions.
If there are tons of users for this API, it'd be needed to convert gradually, and eventually drop the 32bit ops at the end. But in this case, there doesn't seem so many relevant drivers, hence the conversion can be done in a shot as done in your patch 4.
I agree we should do that. Kernel can be 64bit only while we keep maintaining the 32bit ioctls, cant drop that one
participants (2)
-
Takashi Iwai -
Vinod Koul