[alsa-devel] [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests
USB Audio Class version 2.0 supports three different parameter block sizes for CUR requests, which are 1 byte (5.2.3.1 Layout 1 Parameter Block), 2 bytes (5.2.3.2 Layout 2 Parameter Block) and 4 bytes (5.2.3.3 Layout 3 Parameter Block). Use the correct size according to the specific control as it was already done for UACv1. The allocated block size for control requests is increased to support the 4 byte worst case.
Signed-off-by: Julian Scheel julian@jusst.de --- sound/usb/mixer.c | 83 +++++++++++++++++++++++++++++++++++++++++-------------- sound/usb/mixer.h | 2 ++ 2 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 4a49944..675725b 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -233,6 +233,14 @@ static int convert_signed_value(struct usb_mixer_elem_info *cval, int val) if (val >= 0x8000) val -= 0x10000; break; + case USB_MIXER_U32: + val &= 0xffffffff; + break; + case USB_MIXER_S32: + val &= 0xffffffff; + if (val >= 0x80000000) + val -= 0x100000000; + break; } return val; } @@ -253,6 +261,9 @@ static int convert_bytes_value(struct usb_mixer_elem_info *cval, int val) case USB_MIXER_S16: case USB_MIXER_U16: return val & 0xffff; + case USB_MIXER_S32: + case USB_MIXER_U32: + return val & 0xffffffff; } return 0; /* not reached */ } @@ -328,14 +339,26 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret) { struct snd_usb_audio *chip = cval->head.mixer->chip; - unsigned char buf[2 + 3 * sizeof(__u16)]; /* enough space for one range */ + unsigned char buf[4 + 3 * sizeof(__u32)]; /* enough space for one range */ unsigned char *val; int idx = 0, ret, size; __u8 bRequest;
if (request == UAC_GET_CUR) { bRequest = UAC2_CS_CUR; - size = sizeof(__u16); + switch (cval->val_type) { + case USB_MIXER_S32: + case USB_MIXER_U32: + size = 4; + break; + case USB_MIXER_S16: + case USB_MIXER_U16: + size = 2; + break; + default: + size = 1; + break; + } } else { bRequest = UAC2_CS_RANGE; size = sizeof(buf); @@ -446,7 +469,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, int request, int validx, int value_set) { struct snd_usb_audio *chip = cval->head.mixer->chip; - unsigned char buf[2]; + unsigned char buf[4]; int idx = 0, val_len, err, timeout = 10;
validx += cval->idx_off; @@ -454,8 +477,19 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, if (cval->head.mixer->protocol == UAC_VERSION_1) { val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1; } else { /* UAC_VERSION_2 */ - /* audio class v2 controls are always 2 bytes in size */ - val_len = sizeof(__u16); + switch (cval->val_type) { + case USB_MIXER_S32: + case USB_MIXER_U32: + val_len = 4; + break; + case USB_MIXER_S16: + case USB_MIXER_U16: + val_len = 2; + break; + default: + val_len = 1; + break; + }
/* FIXME */ if (request != UAC_SET_CUR) { @@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, value_set = convert_bytes_value(cval, value_set); buf[0] = value_set & 0xff; buf[1] = (value_set >> 8) & 0xff; + buf[2] = (value_set >> 16) & 0xff; + buf[3] = (value_set >> 24) & 0xff; err = snd_usb_autoresume(chip); if (err < 0) return -EIO; @@ -799,24 +835,25 @@ static int check_input_term(struct mixer_build *state, int id, /* feature unit control information */ struct usb_feature_control_info { const char *name; - unsigned int type; /* control type (mute, volume, etc.) */ + int type; /* data type for uac1 */ + int type_uac2; /* data type for uac2 if different from uac1, else -1 */ };
static struct usb_feature_control_info audio_feature_info[] = { - { "Mute", USB_MIXER_INV_BOOLEAN }, - { "Volume", USB_MIXER_S16 }, - { "Tone Control - Bass", USB_MIXER_S8 }, - { "Tone Control - Mid", USB_MIXER_S8 }, - { "Tone Control - Treble", USB_MIXER_S8 }, - { "Graphic Equalizer", USB_MIXER_S8 }, /* FIXME: not implemeted yet */ - { "Auto Gain Control", USB_MIXER_BOOLEAN }, - { "Delay Control", USB_MIXER_U16 }, /* FIXME: U32 in UAC2 */ - { "Bass Boost", USB_MIXER_BOOLEAN }, - { "Loudness", USB_MIXER_BOOLEAN }, + { "Mute", USB_MIXER_INV_BOOLEAN, -1 }, + { "Volume", USB_MIXER_S16, -1 }, + { "Tone Control - Bass", USB_MIXER_S8, -1 }, + { "Tone Control - Mid", USB_MIXER_S8, -1 }, + { "Tone Control - Treble", USB_MIXER_S8, -1 }, + { "Graphic Equalizer", USB_MIXER_S8, -1 }, /* FIXME: not implemeted yet */ + { "Auto Gain Control", USB_MIXER_BOOLEAN, -1 }, + { "Delay Control", USB_MIXER_U16, USB_MIXER_U32 }, + { "Bass Boost", USB_MIXER_BOOLEAN, -1 }, + { "Loudness", USB_MIXER_BOOLEAN, -1 }, /* UAC2 specific */ - { "Input Gain Control", USB_MIXER_S16 }, - { "Input Gain Pad Control", USB_MIXER_S16 }, - { "Phase Inverter Control", USB_MIXER_BOOLEAN }, + { "Input Gain Control", USB_MIXER_S16, -1 }, + { "Input Gain Pad Control", USB_MIXER_S16, -1 }, + { "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 }, };
/* private_free callback */ @@ -1241,7 +1278,13 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid); cval->control = control; cval->cmask = ctl_mask; - cval->val_type = audio_feature_info[control-1].type; + if (state->mixer->protocol == UAC_VERSION_1) + cval->val_type = audio_feature_info[control-1].type; + else /* UAC_VERSION_2 */ + cval->val_type = audio_feature_info[control-1].type_uac2 > -1 ? + audio_feature_info[control-1].type_uac2 : + audio_feature_info[control-1].type; + if (ctl_mask == 0) { cval->channels = 1; /* master channel */ cval->master_readonly = readonly_mask; diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h index d3268f0..3417ef3 100644 --- a/sound/usb/mixer.h +++ b/sound/usb/mixer.h @@ -33,6 +33,8 @@ enum { USB_MIXER_U8, USB_MIXER_S16, USB_MIXER_U16, + USB_MIXER_S32, + USB_MIXER_U32, };
typedef void (*usb_mixer_elem_dump_func_t)(struct snd_info_buffer *buffer,
On Thu, 13 Aug 2015 13:04:46 +0200, Julian Scheel wrote:
USB Audio Class version 2.0 supports three different parameter block sizes for CUR requests, which are 1 byte (5.2.3.1 Layout 1 Parameter Block), 2 bytes (5.2.3.2 Layout 2 Parameter Block) and 4 bytes (5.2.3.3 Layout 3 Parameter Block). Use the correct size according to the specific control as it was already done for UACv1. The allocated block size for control requests is increased to support the 4 byte worst case.
Signed-off-by: Julian Scheel julian@jusst.de
sound/usb/mixer.c | 83 +++++++++++++++++++++++++++++++++++++++++-------------- sound/usb/mixer.h | 2 ++ 2 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 4a49944..675725b 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -233,6 +233,14 @@ static int convert_signed_value(struct usb_mixer_elem_info *cval, int val) if (val >= 0x8000) val -= 0x10000; break;
- case USB_MIXER_U32:
val &= 0xffffffff;break;- case USB_MIXER_S32:
val &= 0xffffffff;if (val >= 0x80000000)val -= 0x100000000;break;
All these are superfluous, as int on Linux is always 32bit.
} return val; } @@ -253,6 +261,9 @@ static int convert_bytes_value(struct usb_mixer_elem_info *cval, int val) case USB_MIXER_S16: case USB_MIXER_U16: return val & 0xffff;
- case USB_MIXER_S32:
- case USB_MIXER_U32:
return val & 0xffffffff;
Ditto.
} return 0; /* not reached */ } @@ -328,14 +339,26 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret) { struct snd_usb_audio *chip = cval->head.mixer->chip;
- unsigned char buf[2 + 3 * sizeof(__u16)]; /* enough space for one range */
unsigned char buf[4 + 3 * sizeof(__u32)]; /* enough space for one range */ unsigned char *val; int idx = 0, ret, size; __u8 bRequest;
if (request == UAC_GET_CUR) { bRequest = UAC2_CS_CUR;
size = sizeof(__u16);
switch (cval->val_type) {case USB_MIXER_S32:case USB_MIXER_U32:size = 4;break;case USB_MIXER_S16:case USB_MIXER_U16:size = 2;break;default:size = 1;break;
Please split this into a small helper function, e.g. uac2_ctl_value_size(). The same stuff is used in below, too.
}@@ -446,7 +469,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, int request, int validx, int value_set) { struct snd_usb_audio *chip = cval->head.mixer->chip;
- unsigned char buf[2];
unsigned char buf[4]; int idx = 0, val_len, err, timeout = 10;
validx += cval->idx_off;
@@ -454,8 +477,19 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, if (cval->head.mixer->protocol == UAC_VERSION_1) { val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1; } else { /* UAC_VERSION_2 */
/* audio class v2 controls are always 2 bytes in size */val_len = sizeof(__u16);
switch (cval->val_type) {case USB_MIXER_S32:case USB_MIXER_U32:val_len = 4;break;case USB_MIXER_S16:case USB_MIXER_U16:val_len = 2;break;default:val_len = 1;break;}/* FIXME */ if (request != UAC_SET_CUR) {
@@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, value_set = convert_bytes_value(cval, value_set); buf[0] = value_set & 0xff; buf[1] = (value_set >> 8) & 0xff;
- buf[2] = (value_set >> 16) & 0xff;
- buf[3] = (value_set >> 24) & 0xff;
It's not smart but OK, just a subtle matter...
err = snd_usb_autoresume(chip); if (err < 0) return -EIO; @@ -799,24 +835,25 @@ static int check_input_term(struct mixer_build *state, int id, /* feature unit control information */ struct usb_feature_control_info { const char *name;
- unsigned int type; /* control type (mute, volume, etc.) */
- int type; /* data type for uac1 */
- int type_uac2; /* data type for uac2 if different from uac1, else -1 */
};
static struct usb_feature_control_info audio_feature_info[] = {
- { "Mute", USB_MIXER_INV_BOOLEAN },
- { "Volume", USB_MIXER_S16 },
- { "Tone Control - Bass", USB_MIXER_S8 },
- { "Tone Control - Mid", USB_MIXER_S8 },
- { "Tone Control - Treble", USB_MIXER_S8 },
- { "Graphic Equalizer", USB_MIXER_S8 }, /* FIXME: not implemeted yet */
- { "Auto Gain Control", USB_MIXER_BOOLEAN },
- { "Delay Control", USB_MIXER_U16 }, /* FIXME: U32 in UAC2 */
- { "Bass Boost", USB_MIXER_BOOLEAN },
- { "Loudness", USB_MIXER_BOOLEAN },
- { "Mute", USB_MIXER_INV_BOOLEAN, -1 },
- { "Volume", USB_MIXER_S16, -1 },
- { "Tone Control - Bass", USB_MIXER_S8, -1 },
- { "Tone Control - Mid", USB_MIXER_S8, -1 },
- { "Tone Control - Treble", USB_MIXER_S8, -1 },
- { "Graphic Equalizer", USB_MIXER_S8, -1 }, /* FIXME: not implemeted yet */
- { "Auto Gain Control", USB_MIXER_BOOLEAN, -1 },
- { "Delay Control", USB_MIXER_U16, USB_MIXER_U32 },
- { "Bass Boost", USB_MIXER_BOOLEAN, -1 },
- { "Loudness", USB_MIXER_BOOLEAN, -1 }, /* UAC2 specific */
- { "Input Gain Control", USB_MIXER_S16 },
- { "Input Gain Pad Control", USB_MIXER_S16 },
- { "Phase Inverter Control", USB_MIXER_BOOLEAN },
- { "Input Gain Control", USB_MIXER_S16, -1 },
- { "Input Gain Pad Control", USB_MIXER_S16, -1 },
- { "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 },
};
/* private_free callback */ @@ -1241,7 +1278,13 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid); cval->control = control; cval->cmask = ctl_mask;
- cval->val_type = audio_feature_info[control-1].type;
- if (state->mixer->protocol == UAC_VERSION_1)
cval->val_type = audio_feature_info[control-1].type;- else /* UAC_VERSION_2 */
cval->val_type = audio_feature_info[control-1].type_uac2 > -1 ?
Usually ">= 0" is used.
audio_feature_info[control-1].type_uac2 :audio_feature_info[control-1].type;
Since audio_feature_info[control-1] is referred so many times, it's better to put in a temporary variable.
thanks,
Takashi
Am 14.08.2015 um 15:17 schrieb Takashi Iwai:
On Thu, 13 Aug 2015 13:04:46 +0200, Julian Scheel wrote:
USB Audio Class version 2.0 supports three different parameter block sizes for CUR requests, which are 1 byte (5.2.3.1 Layout 1 Parameter Block), 2 bytes (5.2.3.2 Layout 2 Parameter Block) and 4 bytes (5.2.3.3 Layout 3 Parameter Block). Use the correct size according to the specific control as it was already done for UACv1. The allocated block size for control requests is increased to support the 4 byte worst case.
Signed-off-by: Julian Scheel julian@jusst.de
sound/usb/mixer.c | 83 +++++++++++++++++++++++++++++++++++++++++-------------- sound/usb/mixer.h | 2 ++ 2 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 4a49944..675725b 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -233,6 +233,14 @@ static int convert_signed_value(struct usb_mixer_elem_info *cval, int val) if (val >= 0x8000) val -= 0x10000; break;
- case USB_MIXER_U32:
val &= 0xffffffff;break;- case USB_MIXER_S32:
val &= 0xffffffff;if (val >= 0x80000000)val -= 0x100000000;break;All these are superfluous, as int on Linux is always 32bit.
Of course. Thanks.
} return val; } @@ -253,6 +261,9 @@ static int convert_bytes_value(struct usb_mixer_elem_info *cval, int val) case USB_MIXER_S16: case USB_MIXER_U16: return val & 0xffff;
- case USB_MIXER_S32:
- case USB_MIXER_U32:
return val & 0xffffffff;Ditto.
} return 0; /* not reached */ } @@ -328,14 +339,26 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret) { struct snd_usb_audio *chip = cval->head.mixer->chip;
- unsigned char buf[2 + 3 * sizeof(__u16)]; /* enough space for one range */
unsigned char buf[4 + 3 * sizeof(__u32)]; /* enough space for one range */ unsigned char *val; int idx = 0, ret, size; __u8 bRequest;
if (request == UAC_GET_CUR) { bRequest = UAC2_CS_CUR;
size = sizeof(__u16);
switch (cval->val_type) {case USB_MIXER_S32:case USB_MIXER_U32:size = 4;break;case USB_MIXER_S16:case USB_MIXER_U16:size = 2;break;default:size = 1;break;Please split this into a small helper function, e.g. uac2_ctl_value_size(). The same stuff is used in below, too.
Ack.
}@@ -446,7 +469,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, int request, int validx, int value_set) { struct snd_usb_audio *chip = cval->head.mixer->chip;
- unsigned char buf[2];
unsigned char buf[4]; int idx = 0, val_len, err, timeout = 10;
validx += cval->idx_off;
@@ -454,8 +477,19 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, if (cval->head.mixer->protocol == UAC_VERSION_1) { val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1; } else { /* UAC_VERSION_2 */
/* audio class v2 controls are always 2 bytes in size */val_len = sizeof(__u16);
switch (cval->val_type) {case USB_MIXER_S32:case USB_MIXER_U32:val_len = 4;break;case USB_MIXER_S16:case USB_MIXER_U16:val_len = 2;break;default:val_len = 1;break;}/* FIXME */ if (request != UAC_SET_CUR) {
@@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, value_set = convert_bytes_value(cval, value_set); buf[0] = value_set & 0xff; buf[1] = (value_set >> 8) & 0xff;
- buf[2] = (value_set >> 16) & 0xff;
- buf[3] = (value_set >> 24) & 0xff;
It's not smart but OK, just a subtle matter...
Hm, what would you like better?
err = snd_usb_autoresume(chip); if (err < 0) return -EIO; @@ -799,24 +835,25 @@ static int check_input_term(struct mixer_build *state, int id, /* feature unit control information */ struct usb_feature_control_info { const char *name;
- unsigned int type; /* control type (mute, volume, etc.) */
int type; /* data type for uac1 */
int type_uac2; /* data type for uac2 if different from uac1, else -1 */ };
static struct usb_feature_control_info audio_feature_info[] = {
- { "Mute", USB_MIXER_INV_BOOLEAN },
- { "Volume", USB_MIXER_S16 },
- { "Tone Control - Bass", USB_MIXER_S8 },
- { "Tone Control - Mid", USB_MIXER_S8 },
- { "Tone Control - Treble", USB_MIXER_S8 },
- { "Graphic Equalizer", USB_MIXER_S8 }, /* FIXME: not implemeted yet */
- { "Auto Gain Control", USB_MIXER_BOOLEAN },
- { "Delay Control", USB_MIXER_U16 }, /* FIXME: U32 in UAC2 */
- { "Bass Boost", USB_MIXER_BOOLEAN },
- { "Loudness", USB_MIXER_BOOLEAN },
- { "Mute", USB_MIXER_INV_BOOLEAN, -1 },
- { "Volume", USB_MIXER_S16, -1 },
- { "Tone Control - Bass", USB_MIXER_S8, -1 },
- { "Tone Control - Mid", USB_MIXER_S8, -1 },
- { "Tone Control - Treble", USB_MIXER_S8, -1 },
- { "Graphic Equalizer", USB_MIXER_S8, -1 }, /* FIXME: not implemeted yet */
- { "Auto Gain Control", USB_MIXER_BOOLEAN, -1 },
- { "Delay Control", USB_MIXER_U16, USB_MIXER_U32 },
- { "Bass Boost", USB_MIXER_BOOLEAN, -1 },
- { "Loudness", USB_MIXER_BOOLEAN, -1 }, /* UAC2 specific */
- { "Input Gain Control", USB_MIXER_S16 },
- { "Input Gain Pad Control", USB_MIXER_S16 },
- { "Phase Inverter Control", USB_MIXER_BOOLEAN },
{ "Input Gain Control", USB_MIXER_S16, -1 },
{ "Input Gain Pad Control", USB_MIXER_S16, -1 },
{ "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 }, };
/* private_free callback */
@@ -1241,7 +1278,13 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid); cval->control = control; cval->cmask = ctl_mask;
- cval->val_type = audio_feature_info[control-1].type;
- if (state->mixer->protocol == UAC_VERSION_1)
cval->val_type = audio_feature_info[control-1].type;- else /* UAC_VERSION_2 */
cval->val_type = audio_feature_info[control-1].type_uac2 > -1 ?Usually ">= 0" is used.
Ack.
audio_feature_info[control-1].type_uac2 :audio_feature_info[control-1].type;Since audio_feature_info[control-1] is referred so many times, it's better to put in a temporary variable.
Ack.
Thanks for reviewing! -Julian
On Fri, 14 Aug 2015 15:45:58 +0200, Julian Scheel wrote:
Am 14.08.2015 um 15:17 schrieb Takashi Iwai:
On Thu, 13 Aug 2015 13:04:46 +0200, Julian Scheel wrote:
@@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, value_set = convert_bytes_value(cval, value_set); buf[0] = value_set & 0xff; buf[1] = (value_set >> 8) & 0xff;
- buf[2] = (value_set >> 16) & 0xff;
- buf[3] = (value_set >> 24) & 0xff;
It's not smart but OK, just a subtle matter...
Hm, what would you like better?
A shorter code would be to to pass __le32 value pointer directly after calling cpu_to_le32(), for example. But it's a matter of taste, so it's OK to keep your code as is.
Takashi
Am 14.08.2015 um 15:51 schrieb Takashi Iwai:
On Fri, 14 Aug 2015 15:45:58 +0200, Julian Scheel wrote:
Am 14.08.2015 um 15:17 schrieb Takashi Iwai:
On Thu, 13 Aug 2015 13:04:46 +0200, Julian Scheel wrote:
@@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, value_set = convert_bytes_value(cval, value_set); buf[0] = value_set & 0xff; buf[1] = (value_set >> 8) & 0xff;
- buf[2] = (value_set >> 16) & 0xff;
- buf[3] = (value_set >> 24) & 0xff;
It's not smart but OK, just a subtle matter...
Hm, what would you like better?
A shorter code would be to to pass __le32 value pointer directly after calling cpu_to_le32(), for example. But it's a matter of taste, so it's OK to keep your code as is.
Thanks, I'll leave it as is for now and keep the le32 approach in mind for the future.
participants (2)
-
Julian Scheel -
Takashi Iwai