Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 13. 09. 23 9:44, Cezary Rojewski wrote:
On 2023-09-12 6:30 PM, Jaroslav Kysela wrote:
On 11. 09. 23 17:45, Cezary Rojewski wrote:
On 2023-09-11 10:43 AM, Cezary Rojewski wrote:

...


Could you help me understand what new code is needed? The
get_subformat() example raised more questions than answers. The patchset
defines snd_pcm_subformat_width(), perhaps you meant that I should
update that function by adding paramter 'format' to its parameters list
and handle it accordingly?

Any guidance would be much appreciated.

What I come up with is a hw_rule for subformat that I add in
snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX
ORed into hw->subformats in snd_pcm_hw_constraints_complete() make
things spin.

static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params,
                      struct snd_pcm_hw_rule *rule)
{
     struct snd_mask *subformat_mask = hw_param_mask(params,
SNDRV_PCM_HW_PARAM_SUBFORMAT);
     struct snd_mask *format_mask = hw_param_mask(params,
SNDRV_PCM_HW_PARAM_FORMAT);
     snd_pcm_format_t f;
     struct snd_mask m;
     int width;

     snd_mask_none(&m);
     snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD);
     snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX);

     pcm_for_each_format(f) {
         if (!snd_mask_test_format(format_mask, f))
             continue;

         width = snd_pcm_format_width(f);
         switch (width) {
         case 32:
             snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20);
             snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24);
             break;
         default:
             break;
         }
     }

     return snd_mask_refine(subformat_mask, &m);
}


However, this means snd_hdac_query_supported_pcm() becomes confusing as
you need to MSBITS_MAX regardless of what the codec supports.
After spending additional few hours on this, I'd say I preferred how
things look with MSBITS_32 instead. STD and MSBITS_MAX existing
simultaneously is confusing too.

This is not a correct implementation. Many things are missing including
the proper subformats filter. I sent my own version here for discussion:

https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@xxxxxxxx/

I do appreciate the input but I expected that, through guidance, this
patch gets updated. Sending a separate patch, not connected to this
patchset - not even a single reference within the commit message body -
is not nice.

The basic API extension is self-contained and I marked it as RFC. The connection was added to this thread.

I'd rather send v2 with your patch incorporated as a part of the
patchset.
No problem. Please, add these cosmetic changes to my patch:

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index f414f8fd217b..cb376e428f59 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1412,9 +1412,10 @@ static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params,
        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);
+       bool found;
+
        snd_mask_none(&m);
        snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD);
-       bool found;
        pcm_for_each_format(k) {
                if (!snd_mask_test(fmask, k))
                        continue;
@@ -1437,7 +1438,6 @@ static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params,
  * @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.
  * The STD and MAX subformats are handled automatically. If the driver

						Jaroslav

--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux