On 2023-09-13 10:57 AM, Jaroslav Kysela wrote:
On 13. 09. 23 10:22, Cezary Rojewski wrote:
+struct snd_pcm_hw_constraint_subformat {
+ snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */
+ u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */
From what I know, we are dealing with u64 masks here. Why u32 here?
It's not true. See the removed code which calls
snd_pcm_hw_constraint_mask() for SNDRV_PCM_HW_PARAM_SUBFORMAT. Only the
format is handled for 64 bits and the handling of other bits is purely
optional, because masks can handle up to 256 bits:
#define SNDRV_MASK_MAX 256
struct snd_mask {
__u32 bits[(SNDRV_MASK_MAX+31)/32];
};
Because we used only few bits for SUBFORMAT, the 32 bit code is enough.
We can expand this latter without any impact to the user space interface.
Thanks for explaining. I must admit that this "balance" is confusing
though - up to 256 bits are supported, yet depending on the use-case,
size is lowered for optimization reasons.
+static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params,
+ struct snd_pcm_hw_rule *rule)
+{
+ const struct snd_pcm_hw_constraint_subformat *sf;
What's 'sf'? I'd suggest to be more descriptive here.
SubFormat. The larger name will not make the for loop more readable. The
function is small, so reader is not lost.
Decided to do a "mix" of shortcuts and explicitness - fmask for
format-mask, sfmask for subformat-mask and sf for cursor variable.
+ snd_pcm_format_t k;
Internally I was utilizing 'f' as that's what macro expects in its
declaration. s/k/f/
Copied from pcm_native.c - snd_pcm_hw_rule_format().
I understand but this is not a technical argument. It's clearly more
informative and cohesive to utilize 'f' in place of 'k'.
The aforementioned function should be updated in the future too.
+ struct snd_mask m;
+ struct snd_mask *fmask = hw_param_mask(params,
SNDRV_PCM_HW_PARAM_FORMAT);
+ struct snd_mask *mask = hw_param_mask(params,
SNDRV_PCM_HW_PARAM_SUBFORMAT);
So, the reason I opted for 'subformat_mask' and 'format_mask' is that
otherwise reader is crowded with ambiguous 'mask' and its relatives. I'd
avoid shortcuts when multiple variables touch the same subject.
s/fmask/format_mask/
s/mask/subformat_mask/
Too long, the function is small.
As mentioned above, decided to do a mix instead of ignoring completely.
I value readability much, and I believe many on the list value it too.
The combination of m+mask+fmask is confusing at best.
+ snd_mask_none(&m);
+ snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD);
+ bool found;
Suggestion is to add newline before declaration and execution blocks.
Also, why not reserve-christmass-tree model? There quite a few variables
here.
Yes it should be shuffled.
Ack and addressed in the v2 series.
+ pcm_for_each_format(k) {
+ if (!snd_mask_test(fmask, k))
+ continue;
Similarly here. A newline would effectively separate conditional
for-loop from the actual execution block.
It's questionable.
A so-called blob of text should be avoided. This is not a trivial part
of the code. Least we can do is flesh it out a bit plus provide a more
self-explanatory variable names.
+ found = false;
+ for (sf = rule->private; sf && sf->format !=
SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) {
+ if (sf->format != k)
+ continue;
+ found = true;
+ m.bits[0] |= sf->subformats;
+ break;
+ }
+ if (!found && snd_pcm_format_linear(k))
For my own education, why checking if format is linear is essential
here? Perhaps a comment?
Subformat MSBITS are valid only for linear formats, aren't? It does not
make sense to set MAX for other formats.
I cannot deduce any comment-to-add from this, so opted-out of adding any
comment.
+ snd_mask_set(&m, (__force
unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
+ }
+ return snd_mask_refine(mask, &m);
+}
+
+/**
+ * snd_pcm_hw_constraint_subformats - add a hw constraint subformats
rule
+ * @runtime: PCM runtime instance
+ * @cond: condition bits
+ * @subformats: array with struct snd_pcm_subformat elements
+ * @nmemd: size of array with struct snd_pcm_subformat elements
+ *
+ * This constraint will set relation between format and subformats.
I do not believe 'This constaint' brings any value. Reader is already
aware of it. Starting from the 'Set' part brings the same value with
fewer words.
Copied from snd_pcm_hw_constraint_msbits function.
Again, I do understand the origin of this, but this is not a technical
argument - if something can be improved even when copying, we should do so.
+ * The STD and MAX subformats are handled automatically. If the driver
+ * does not set this constraint, only STD and MAX subformats are
handled.
+ *
+ * Return: Zero if successful, or a negative error code on failure.
+ */
+int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime,
+ unsigned int cond,
+ struct snd_pcm_hw_constraint_subformat
*subformats)
+{
...
- err = snd_pcm_hw_constraint_mask(runtime,
SNDRV_PCM_HW_PARAM_SUBFORMAT,
- PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD));
- if (err < 0)
- return err;
+ if (!runtime->subformat_constraint) {
I'd try to avoid another special-bit in the runtime space. But I might
be wrong here and it's unavoidable. Let me ask though, why cannot we do
the constraint unconditionally?
This is for a special case when the drivers do not set
snd_pcm_hw_constraint_subformats (all current drivers). In this case,
the default is to handle STD and MAX subformat bits.
This constraint should be applied only one time. So this prevents to
install it twice.
I believe we could avoid special-case approach. Have a copy/intersection
helpers in place and utilize iterations-with-sentinel-entry. Provided
such in v2 of my series.
I'll send v2 to address some things from this discussion and kernel test
robot.
