Re: [alsa-devel] [PATCH v4 4/6] core: add API header and driver header files
+struct snd_compr_runtime {
- snd_pcm_state_t state;
- struct snd_compr_ops *ops;
- void *buffer;
Can we define buffer as char *?
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items? Can we have negative size here?
- unsigned int fragments;
- size_t hw_pointer;
- size_t app_pointer;
- size_t bytes_written;
- size_t bytes_copied;
- wait_queue_head_t sleep;
+};
On Tue, Dec 13, 2011 at 06:19:49PM +0530, Nallasellan, Singaravelan wrote:
+struct snd_compr_runtime {
- snd_pcm_state_t state;
- struct snd_compr_ops *ops;
- void *buffer;
Can we define buffer as char *?
Clearly we *can* but why would we want to do that for a pointer to unstructured data?
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items? Can we have negative size here?
Again, what is the advantage in doing this?
+struct snd_compr_runtime {
- snd_pcm_state_t state;
- struct snd_compr_ops *ops;
- void *buffer;
Can we define buffer as char *?
Clearly we *can* but why would we want to do that for a pointer to unstructured data?
True, it will always be pointer to bytes. u8 could also be used. No big advantage here. It will be clear.
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items? Can we have negative size here?
Again, what is the advantage in doing this?
The size will be positive anyways. Why don't we do it? No big advantage here as well. It will be clear.
At Tue, 13 Dec 2011 18:19:49 +0530, Nallasellan, Singaravelan wrote:
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items?
size_t is unsigned.
But, it needs consideration whether size_t is the best choice, since size_t is basically a long, thus its size is different between 32bit and 64bit architectures. This may lead to mess in many cases when you think of overlapping.
Takashi
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items?
size_t is unsigned.
But, it needs consideration whether size_t is the best choice, since size_t is basically a long, thus its size is different between 32bit and 64bit architectures. This may lead to mess in many cases when you think of overlapping.
Sorry for my mess on size_t. I think the assumption here is 32 bits. Should u32 be fine here?
At Tue, 13 Dec 2011 19:21:30 +0530, Nallasellan, Singaravelan wrote:
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items?
size_t is unsigned.
But, it needs consideration whether size_t is the best choice, since size_t is basically a long, thus its size is different between 32bit and 64bit architectures. This may lead to mess in many cases when you think of overlapping.
Sorry for my mess on size_t. I think the assumption here is 32 bits. Should u32 be fine here?
This depends pretty much on the demand and the implementation. Unless a buffer over 4GB is needed, u32 should be OK for buffer_size, etc. (Or, in this case, unsigned int would be fine, too. It's not necessarily be limited to 32bit. u32 or such is used usually for the type that must be 32bit.)
But, the questions remain for hw_pointer, app_pointer, bytes_* fields. Are these supposed to be 32bit or more? This question is more important than the buffer size. The buffer size would be rarely over 32bit. But, if hw_pointer or such represents the accumulated position like ALSA PCM does, you'll face the overlapping problem. 32bit limit can be reached easily in the real use case.
When this changes between 32bit and 64bit, think twice about the 32bit compat-layer on 64bit arch (and the corresponding user-space implementation)...
Takashi
On Tue, 2011-12-13 at 15:04 +0100, Takashi Iwai wrote:
At Tue, 13 Dec 2011 19:21:30 +0530, Nallasellan, Singaravelan wrote:
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items?
size_t is unsigned.
But, it needs consideration whether size_t is the best choice, since size_t is basically a long, thus its size is different between 32bit and 64bit architectures. This may lead to mess in many cases when you think of overlapping.
Sorry for my mess on size_t. I think the assumption here is 32 bits. Should u32 be fine here?
This depends pretty much on the demand and the implementation. Unless a buffer over 4GB is needed, u32 should be OK for buffer_size, etc. (Or, in this case, unsigned int would be fine, too. It's not necessarily be limited to 32bit. u32 or such is used usually for the type that must be 32bit.)
I don't think someone would need buffer of 4GB :)
But, the questions remain for hw_pointer, app_pointer, bytes_* fields. Are these supposed to be 32bit or more? This question is more important than the buffer size. The buffer size would be rarely over 32bit. But, if hw_pointer or such represents the accumulated position like ALSA PCM does, you'll face the overlapping problem. 32bit limit can be reached easily in the real use case.
the pointer are offsets within the ring buffer. there are cumulative counters which will get overlapped after the size_t/u32 value.
When this changes between 32bit and 64bit, think twice about the 32bit compat-layer on 64bit arch (and the corresponding user-space implementation)...
Is it a fair assumption that userspace compiled for 32 bit should work with 32 bit kernel and same for 64 bit, or does the ABI mandate it be consistent across 32 and 64 bits. In the latter case we should change the ones in kernel ABI to consistent size of u32/u64. IMO u32 should be fine, please let me know if anyone thinks otherwise. In former size_t should be fine :)
At Tue, 13 Dec 2011 19:54:27 +0530, Vinod Koul wrote:
On Tue, 2011-12-13 at 15:04 +0100, Takashi Iwai wrote:
At Tue, 13 Dec 2011 19:21:30 +0530, Nallasellan, Singaravelan wrote:
- size_t buffer_size;
- size_t fragment_size;
Can we define buffer_size and fragment_size as unsigned items?
size_t is unsigned.
But, it needs consideration whether size_t is the best choice, since size_t is basically a long, thus its size is different between 32bit and 64bit architectures. This may lead to mess in many cases when you think of overlapping.
Sorry for my mess on size_t. I think the assumption here is 32 bits. Should u32 be fine here?
This depends pretty much on the demand and the implementation. Unless a buffer over 4GB is needed, u32 should be OK for buffer_size, etc. (Or, in this case, unsigned int would be fine, too. It's not necessarily be limited to 32bit. u32 or such is used usually for the type that must be 32bit.)
I don't think someone would need buffer of 4GB :)
But, the questions remain for hw_pointer, app_pointer, bytes_* fields. Are these supposed to be 32bit or more? This question is more important than the buffer size. The buffer size would be rarely over 32bit. But, if hw_pointer or such represents the accumulated position like ALSA PCM does, you'll face the overlapping problem. 32bit limit can be reached easily in the real use case.
the pointer are offsets within the ring buffer.
OK, then it's no problem to assume 32bit is enough.
there are cumulative counters which will get overlapped after the size_t/u32 value.
Which field specifically? The cumulative counter has been a problem.
When this changes between 32bit and 64bit, think twice about the 32bit compat-layer on 64bit arch (and the corresponding user-space implementation)...
Is it a fair assumption that userspace compiled for 32 bit should work with 32 bit kernel and same for 64 bit, or does the ABI mandate it be consistent across 32 and 64 bits.
The 32bit user-space must work with 64bit kernel. It means, it'd be much easier if ABI definition is compatible between 32 and 64bit.
That is, using a long type in the ABI definition should be definitely avoided. We had long fields in ALSA PCM field definitions, and this caused lots of really nasty problems. So, I tell you -- don't do it again. Learn from the history :)
Of course, this is about the ABI definition. But, the internal structure (types, etc) usually follows the ABI requirement. So I stated the questions here.
In the latter case we should change the ones in kernel ABI to consistent size of u32/u64. IMO u32 should be fine, please let me know if anyone thinks otherwise. In former size_t should be fine :)
thanks,
Takashi
On Tue, 2011-12-13 at 16:01 +0100, Takashi Iwai wrote:
At Tue, 13 Dec 2011 19:54:27 +0530, Vinod Koul wrote:
On Tue, 2011-12-13 at 15:04 +0100, Takashi Iwai wrote:
At Tue, 13 Dec 2011 19:21:30 +0530, Nallasellan, Singaravelan wrote:
> + size_t buffer_size; > + size_t fragment_size; Can we define buffer_size and fragment_size as unsigned items?
size_t is unsigned.
But, it needs consideration whether size_t is the best choice, since size_t is basically a long, thus its size is different between 32bit and 64bit architectures. This may lead to mess in many cases when you think of overlapping.
Sorry for my mess on size_t. I think the assumption here is 32 bits. Should u32 be fine here?
This depends pretty much on the demand and the implementation. Unless a buffer over 4GB is needed, u32 should be OK for buffer_size, etc. (Or, in this case, unsigned int would be fine, too. It's not necessarily be limited to 32bit. u32 or such is used usually for the type that must be 32bit.)
I don't think someone would need buffer of 4GB :)
But, the questions remain for hw_pointer, app_pointer, bytes_* fields. Are these supposed to be 32bit or more? This question is more important than the buffer size. The buffer size would be rarely over 32bit. But, if hw_pointer or such represents the accumulated position like ALSA PCM does, you'll face the overlapping problem. 32bit limit can be reached easily in the real use case.
the pointer are offsets within the ring buffer.
OK, then it's no problem to assume 32bit is enough.
Great :)
there are cumulative counters which will get overlapped after the size_t/u32 value.
Which field specifically? The cumulative counter has been a problem.
There are two fields in snd_compr_runtime, bytes_written and bytes_copied. Both are counting cumulative bytes written to ring buffer and bytes copied by DSP. Assumption is that they will wrap once they reach max. That should not cause issue in core. Then the core will wrap around the timestamp value. My thought was user lib should always take care of kernel wrapping and provide sufficiently large tstamp value to application.
When this changes between 32bit and 64bit, think twice about the 32bit compat-layer on 64bit arch (and the corresponding user-space implementation)...
Is it a fair assumption that userspace compiled for 32 bit should work with 32 bit kernel and same for 64 bit, or does the ABI mandate it be consistent across 32 and 64 bits.
The 32bit user-space must work with 64bit kernel. It means, it'd be much easier if ABI definition is compatible between 32 and 64bit.
That is, using a long type in the ABI definition should be definitely avoided. We had long fields in ALSA PCM field definitions, and this caused lots of really nasty problems. So, I tell you -- don't do it again. Learn from the history :)
Of course, this is about the ABI definition. But, the internal structure (types, etc) usually follows the ABI requirement. So I stated the questions here.
Right. I think we should be pretty decent if we just change the kernel APIs to u32. Based on that all other non cumulative counter should be u32 and cumulative ones u64 :) That was we will not care what bit userland is, and otherthings will work as expected.
Thanks
participants (4)
-
Mark Brown -
Nallasellan, Singaravelan -
Takashi Iwai -
Vinod Koul