On 2023-09-21 8:25 AM, Jaroslav Kysela wrote:
On 18. 09. 23 15:39, Cezary Rojewski wrote:
Improve granularity of format selection for S32/U32 formats by adding constants representing 20, 24 and MAX most significant bits.
To make it easy for drivers to utilize those constants, introduce snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former is self-explanatory, the latter returns the bit-per-sample value based on format and subformat characteristics of the provided hw_params. snd_pcm_hw_copy() helps with copying a hardware parameters space as with introduction of subformats the operations becomes non-trivial.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
...
struct snd_pcm_hardware { unsigned int info; /* SNDRV_PCM_INFO_* */ u64 formats; /* SNDRV_PCM_FMTBIT_* */ + struct snd_pcm_subformat *subformats;
I don't think that it's required to add subformats to the hardware template. The new constraint can handle subformat refining without the template modifications (pointer to map table is stored privately to this constraint).
After iterating over the feedback, modified the field to u32 and all the handlers to acknowledge that it is S32_LE-specific. I agree with statement that we can expand on the API in the future if there's demand for it.
Also, I miss the constraint handling here. Without the constraint, the new API is not functional and it does not make sense to split the constraint code to other patch.
Ack, merging the two.
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p);
This may be probably inline function. See bellow.
+ kfree(runtime->hw.subformats);
Do we really need to do an assumption about allocations for this field? The driver may use a static structure. No problem, when this is not added to runtime->hw.
This code is gone in v3 since there is no need to kfree() a u32.
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p) +{ + snd_pcm_subformat_t subformat = params_subformat(p); + snd_pcm_format_t format = params_format(p); + int width;
+ switch (format) { + case SNDRV_PCM_FORMAT_S32_LE: + case SNDRV_PCM_FORMAT_U32_LE: + case SNDRV_PCM_FORMAT_S32_BE: + case SNDRV_PCM_FORMAT_U32_BE: + width = snd_pcm_subformat_width(subformat);
This is not a correct implementation. The width should be returned for MAX subformat (== snd_pcm_format_width value). See bellow.
snd_pcm_subformat_width() returns 0 if subformat==MAX and fallthrough ensures snd_pcm_format_width() is returned in such case.
+int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)
This function should probably have two arguments - format + subformat to return the correct information. The MAX subformat should return snd_pcm_format_width value.
My intention is to have two granular functions. One for obtaining subformat-width explicitly and the other for calculating bits-per-sample.
+int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct snd_pcm_hardware *from)
This function is not required, if only the constraint function handles the subformat refining.
Ack, removing in v3.