Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.
To:     Ruslan Bilovol <ruslan.bilovol@gmail.com>
Cc:     Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
References: <20180424172445.31928-2-jorge.sanjuan@codethink.co.uk>
 <20180427170649.27334-1-jorge.sanjuan@codethink.co.uk>
 <CAB=otbRKf57D52VRZgE=4XCLYaT9xE7eNsyLrBqmaghgB7LCfQ@mail.gmail.com>
From:   Jorge <jorge.sanjuan@codethink.co.uk>
Message-ID: <52c0eb21-0a69-7a2b-da8a-2d33ad4e11d2@codethink.co.uk>
Date:   Tue, 8 May 2018 10:43:03 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <CAB=otbRKf57D52VRZgE=4XCLYaT9xE7eNsyLrBqmaghgB7LCfQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 04/05/18 01:57, Ruslan Bilovol wrote:
> On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
> <jorge.sanjuan@codethink.co.uk> wrote:
>> This adds support for the MIXER UNIT in UAC3. All the information
>> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
>> read the rest of the logical cluster to obtain the channel config
>> as that wont make any difference in the current mixer behaviour.
>>
>> The name of the mixer unit is not yet requested as there is not
>> support for the UAC3 Class Specific String requests.
>>
>> Tested in an UAC3 device working as a HEADSET with a basic mixer
>> unit (same as the one in the BADD spec) with no controls.
> 
> So, after deeper looking into the code and after testing this patch,
> in your usecase (mixer with no controls) you'll never execute
> build_mixer_unit_ctl(), correct? So did you try to just fix issues with
> incorrect parsing of mixer unit descriptor?
> 
>>
>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
>> ---
>>   include/uapi/linux/usb/audio.h | 19 +++++++--
>>   sound/usb/mixer.c              | 88 ++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 97 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
>> index 3a78e7145689..13d98e6e0db1 100644
>> --- a/include/uapi/linux/usb/audio.h
>> +++ b/include/uapi/linux/usb/audio.h
>> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>>   static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>>                                                int protocol)
>>   {
>> -       return (protocol == UAC_VERSION_1) ?
>> -               &desc->baSourceID[desc->bNrInPins + 4] :
>> -               &desc->baSourceID[desc->bNrInPins + 6];
>> +       switch (protocol) {
>> +       case UAC_VERSION_1:
>> +               return &desc->baSourceID[desc->bNrInPins + 4];
>> +       case UAC_VERSION_2:
>> +               return &desc->baSourceID[desc->bNrInPins + 6];
>> +       case UAC_VERSION_3:
>> +               return &desc->baSourceID[desc->bNrInPins + 2];
>> +       default:
>> +               return NULL;
>> +       }
>> +}
>> +
>> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
>> +{
>> +       return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
>> +               desc->baSourceID[desc->bNrInPins];
>>   }
>>
>>   static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 301ad61ed426..3503f4840ec3 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>>   }
>>
>>   /*
>> + * Get logical cluster information for UAC3 devices.
>> + */
>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
>> +{
>> +       struct uac3_cluster_header_descriptor c_header;
>> +       int err;
>> +
>> +       err = snd_usb_ctl_msg(state->chip->dev,
>> +                       usb_rcvctrlpipe(state->chip->dev, 0),
>> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
>> +                       cluster_id,
>> +                       snd_usb_ctrl_intf(state->chip),
>> +                       &c_header, sizeof(c_header));
>> +       if (err < 0)
>> +               goto error;
>> +       if (err != sizeof(c_header)) {
>> +               err = -EIO;
>> +               goto error;
>> +       }
>> +
>> +       return c_header.bNrChannels;
>> +
>> +error:
>> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
>> +       return err;
>> +}
>> +
>> +/*
>> + * Get number of channels for a Mixer Unit.
>> + */
>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>> +                                      struct uac_mixer_unit_descriptor *desc)
>> +{
>> +       int mu_channels;
>> +
>> +       if (desc->bLength < 11)
>> +               return -EINVAL;
>> +       if (!desc->bNrInPins)
>> +               return -EINVAL;
>> +
>> +       switch (state->mixer->protocol) {
>> +       case UAC_VERSION_1:
>> +       case UAC_VERSION_2:
>> +       default:
>> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
>> +               break;
>> +       case UAC_VERSION_3:
>> +               mu_channels = get_cluster_channels_v3(state,
>> +                               uac3_mixer_unit_wClusterDescrID(desc));
>> +               break;
>> +       }
>> +
>> +       if (!mu_channels)
>> +               return -EINVAL;
>> +
>> +       return mu_channels;
>> +}
>> +
>> +/*
>>    * parse the source unit recursively until it reaches to a terminal
>>    * or a branched unit.
>>    */
>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>>                                  term->name = le16_to_cpu(d->wClockSourceStr);
>>                                  return 0;
>>                          }
>> +                       case UAC3_MIXER_UNIT: {
>> +                               struct uac_mixer_unit_descriptor *d = p1;
>> +
>> +                               err = uac_mixer_unit_get_channels(state, d);
>> +                               if (err < 0)
>> +                                       return err;
>> +
>> +                               term->channels = err;
>> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
>> +
>> +                               return 0;
>> +                       }
>>                          default:
>>                                  return -ENODEV;
>>                          }
>> @@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>>    */
>>   static void build_mixer_unit_ctl(struct mixer_build *state,
>>                                   struct uac_mixer_unit_descriptor *desc,
>> -                                int in_pin, int in_ch, int unitid,
>> -                                struct usb_audio_term *iterm)
>> +                                int in_pin, int in_ch, int num_outs,
>> +                                int unitid, struct usb_audio_term *iterm)
>>   {
>>          struct usb_mixer_elem_info *cval;
>> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>>          unsigned int i, len;
>>          struct snd_kcontrol *kctl;
>>          const struct usbmix_name_map *map;
>> @@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>>          int input_pins, num_ins, num_outs;
>>          int pin, ich, err;
>>
>> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>> +       err = uac_mixer_unit_get_channels(state, desc);
>> +       if (err < 0) {
>>                  usb_audio_err(state->chip,
>>                                "invalid MIXER UNIT descriptor %d\n",
>>                                unitid);
>> -               return -EINVAL;
>> +               return err;
>>          }
>>
>> +       num_outs = err;
>> +       input_pins = desc->bNrInPins;
>> +
>>          num_ins = 0;
>>          ich = 0;
>>          for (pin = 0; pin < input_pins; pin++) {
>> @@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>>                                  }
>>                          }
>>                          if (ich_has_controls)
>> -                               build_mixer_unit_ctl(state, desc, pin, ich,
>> +                               build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
>>                                                       unitid, &iterm);
> 
> So with current sources we will never reach this place. In your
> usecase (mixer with no
> controls) it obviously won't go inside.
> However, I created a mixer with controls but still
> build_mixer_unit_ctl() isn't executed.
> This is because UAC3 input terminal parsing always returns 0 channels, this is
> what still needs to be implemented (see comment "REVISIT: UAC3 IT doesn't
> have channels/cfg" in check_input_term)

Thanks for testing this!

I missed that one with the number of input channels. Regarding the 
"REVISIT: UAC3 IT doesn't have channels/cfg" comment, we can easily fix 
that using the helper function I added in this patch for reading the 
logical cluster's number of channels `get_cluster_channels_v3`. The 
channel config bitmap I don't see it being used at all so parsing 
iterm.channels should be enough. Any thoughts?

- Jorge

> 
> Beside of that, other part of this patch seems to work as expected, at least
> uac_mixer_unit_get_channels() gives correct results - I checked it for
> two-channels config, that is different comparing to BADD.
> 
> Thanks,
> Ruslan
>