On 2023-09-21 8:41 AM, Jaroslav Kysela wrote:
On 18. 09. 23 15:39, Cezary Rojewski wrote:
Substream value is currently hardcoded to SNDRV_PCM_SUBFORMAT_STD.
Update the constraint procedure so that subformat selection is not
ignored. Case STD is always supported as most PCMs do not care about
subformat.
Suggested-by: Jaroslav Kysela <perex@xxxxxxxx>
Better Co-developed mark. Also I would move whole code to the first
patch. It does not make sense to split the mandatory code.
Another option is to increase the protocol version to the separate patch
where all necessary code changes are applied (for MSBITS_MAX). But it
may break backports, so the change should be aligned with the SUBFMT
defines.
While most of my feedback is below, if we decide that "subformat as
first class citizen" approach is good-to-go, this patch gets
re-authorized in v3 as the input on the constraint part from your end is
major.
+ struct snd_mask *sfmask;
m_rw -> sfmask renaming. I prefer universal name to allow easy reuse in
future.
Ack.
+ for (sf = hw->subformats; sf->mask && !found; sf++) {
My proposal [1] defined SNDRV_PCM_FORMAT_CONSTRAINT_END value not
relying to zero format (which is U8) and zero subformat to skip the
MSBIT_MAX setting bellow. After some thought, if the driver sets
SNDRV_PCM_SUBFORMAT_MSBITS_STD, the result will be similar, thus the
mask can be zero and the code may be reduced. So no objection for this
change.
+ if (!found && snd_pcm_format_linear(f))
+ snd_mask_set(&m, (__force
unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
+ }
+exit:
+ return snd_mask_refine(sfmask, &m);
+}
+
+static int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime
*runtime,
+ unsigned int cond,
+ struct snd_pcm_hardware *hw)
+{
Because your change does not assume that this constraint is called from
the drivers, the comments and EXPORT_SYMBOL() lines were removed from
the original proposal [1]. I believe that the standalone constraint is
better at this time - reduce code, the use of the subformat extension is
not mandatory.
Thank you for thorough feedback, Jaroslav. This is much appreciated.
Before I comment on the rest of the comments, let me provide a summary:
I believe that most of the subject comes down to: subformat as
mainstream API -or- subformat as niche API.
If the general opinion of the developers is: let's go for the latter
until we have more users, I have no problem with merging the patches 1 &
2 and addressing most of the review in 1:1 fashion as indeed many parts
of the proposed API lose their purpose.
My view is different as I'd like subformat to become mainstream. To be
honest, the object that allowed it to happen has been suggested by you
Jaroslav - the struct made of { format; mask; }. It allows to describe
what subformat _is_ in explicit fashion and by being exposed in
snd_pcm_hardware becomes standard part of the API. As previously
suggested, I believe it could replace ->msbits in future too.
Question to the fellow developers: What's your take on the subject?
Kind regards,
Czarek
[1]
https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@xxxxxxxx/
https://lore.kernel.org/alsa-devel/20230913103716.67315-1-perex@xxxxxxxx/
