dB gain

dB gain

[James Courtier-Dutton]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

Hi,

I have made a new attempt at adding support for dB gain reports in
alsamixer.

See the attached diffs against the current alsa hg repository. 27-5-2006

It shows an example of support for the snd-ca0106 driver.
As one can see, this adds minimal amounts of code to each sound card
driver. 22 new lines + 1 per volume control making 33 lines total.

Any comments?

James






[-- Attachment #2: alsa-kernel-db14.diff --]
[-- Type: text/x-patch, Size: 11592 bytes --]

diff -r 551da56a592c core/control.c
--- a/core/control.c	Fri May 19 19:22:34 2006 +0200
+++ b/core/control.c	Sat May 27 19:53:59 2006 +0100
@@ -241,6 +241,7 @@ struct snd_kcontrol *snd_ctl_new1(const 
 	kctl.info = ncontrol->info;
 	kctl.get = ncontrol->get;
 	kctl.put = ncontrol->put;
+	kctl.info2 = ncontrol->info2;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
 	return snd_ctl_new(&kctl, access);
@@ -570,6 +571,172 @@ static int snd_ctl_card_info(struct snd_
 	}
 	kfree(info);
 	return 0;
+}
+
+static int uint32_to_message(struct snd_ctl_misc *misc, u32 value)
+{
+	unsigned int pointer = misc->length;
+	u32 *store = (u32*) &misc->message[pointer];
+	*store = value;
+	misc->length += sizeof(u32);
+	return 0;
+}
+
+static int message_to_uint32(struct snd_ctl_misc *misc, unsigned int *value)
+{
+	unsigned int pointer = misc->length;
+	u32 *store = (u32*) &misc->message[pointer];
+	*value = *store;
+	misc->length += sizeof(u32);
+	return 0;
+}
+
+
+static int add_int32_tlv_to_container(struct snd_ctl_misc *misc, unsigned int type, unsigned int value)
+{
+	uint32_to_message(misc, type);
+	uint32_to_message(misc, 4);
+	uint32_to_message(misc, value);
+	/* FIXME: Check for overflows */
+	return 0;
+}
+
+static int add_message_tlv_to_container(
+	struct snd_ctl_misc *misc, unsigned int length, unsigned char *message)
+{
+	unsigned int pointer = misc->length;
+	unsigned int n;
+	for(n=0; n<length; n++) {
+		misc->message[pointer++] = message[n];
+	}
+	misc->length = pointer;
+	/* FIXME: Check for overflows */
+	return 0;
+}
+
+
+static int container_open(struct snd_ctl_misc *misc, unsigned int type)
+{
+	uint32_to_message(misc, type);
+	uint32_to_message(misc, 0); /* This gets updated at container close */
+	return 0;
+}
+
+static int container_close(struct snd_ctl_misc *misc)
+{
+	u32 *store = (u32*) &misc->message[sizeof(u32)];
+	*store = misc->length;
+	return 0;
+}
+
+static int snd_ctl_misc_user(struct snd_ctl_file *ctl,
+				  struct snd_ctl_misc __user *_misc)
+{
+	struct snd_ctl_misc *misc;
+	struct snd_card *card = ctl->card;
+	struct snd_kcontrol *kctl;
+	struct snd_ctl_elem_info2 info2;
+	int result=0;
+	int numid;
+	unsigned int received_length;
+	unsigned int tlv_type;
+	unsigned int tlv_length;
+	unsigned int tlv_value;
+	unsigned int container_type;
+	unsigned int container_length;
+	
+	misc = kmalloc(2048, GFP_KERNEL);
+	if (misc == NULL)
+		return -ENOMEM;
+
+	/* Only copy _misc: version and length from user-space */
+	if (copy_from_user(misc, _misc, 8)) {
+		result = -EFAULT;
+		goto exit_free;
+	}
+	/* Sanity check */
+	snd_printk("misc->version=%d\n",misc->version);
+	if (misc->version!=1) {
+		result = -EINVAL;
+		goto exit_free;
+	}
+	received_length = misc->length;
+	snd_printk("misc->length=%d\n",misc->length);
+	if (received_length>2040 || received_length<8) {
+		result = -EINVAL;
+		goto exit_free;
+	}
+	/* Copy the entire _misc from user-space */
+	if (copy_from_user(misc, _misc, received_length)) {
+		result = -EFAULT;
+		goto exit_free;
+	}
+	misc->length=0;
+	message_to_uint32(misc, &container_type);
+	snd_printk("container_type=%d\n",container_type);
+	switch (container_type) {
+		case SND_MISC_DB_SCALE:
+			message_to_uint32(misc, &container_length);
+			/* FIXME: Do more sanity checks here */
+			message_to_uint32(misc, &tlv_type);
+			snd_printk("tlv_type=%d\n",tlv_type);
+			switch (tlv_type) {
+				case SND_MISC_ELEM_NUMID:
+					message_to_uint32(misc, &tlv_length);
+					snd_printk("tlv_length=%d\n",tlv_length);
+					if (tlv_length != 4) {
+						result = -EINVAL;
+						goto exit_free;
+					}
+					message_to_uint32(misc, &tlv_value);
+					snd_printk("tlv_value=%d\n",tlv_value);
+					break;
+				default:
+					result = -EINVAL;
+					goto exit_free;
+					break;
+			}
+			break;
+		default:
+			result = -EINVAL;
+			goto exit_free;
+	}
+	numid=tlv_value;
+	//snd_printk("numid=%d\n", tlv_value);
+	down_read(&card->controls_rwsem);
+	kctl = snd_ctl_find_numid(card, numid);
+	if (kctl == NULL) {
+		snd_printk("stop 1\n");
+		result = -ENOENT;
+		goto exit_up;
+	}
+	if ( !(kctl->info2) ) { 
+		snd_printk("stop 2\n");
+		result = -EINVAL;
+		goto exit_up;
+	}
+	if (kctl->info2(kctl, 1, &info2)) {
+		snd_printk("stop 3\n");
+		result = -EINVAL;
+		goto exit_up;
+	}
+		;
+	snd_printk("ok 1\n");
+	misc->version=1;
+	misc->length=0;
+	container_open(misc, SND_MISC_DB_SCALE);
+	add_message_tlv_to_container(misc, info2.length, info2.message);
+	container_close(misc);
+	if (copy_to_user(_misc, misc, received_length))
+		result = -EFAULT;
+	snd_printk("ok 2\n");
+	
+exit_up:
+	up_read(&card->controls_rwsem);
+exit_free:
+	kfree(misc);
+exit:
+	return result;
 }
 
 static int snd_ctl_elem_list(struct snd_card *card,
@@ -1103,6 +1270,8 @@ static long snd_ctl_ioctl(struct file *f
 		return snd_ctl_elem_add_user(ctl, argp, 1);
 	case SNDRV_CTL_IOCTL_ELEM_REMOVE:
 		return snd_ctl_elem_remove(ctl, argp);
+	case SNDRV_CTL_IOCTL_MISC:
+		return snd_ctl_misc_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
 		return snd_ctl_subscribe_events(ctl, ip);
 	case SNDRV_CTL_IOCTL_POWER:
diff -r 551da56a592c include/asound.h
--- a/include/asound.h	Fri May 19 19:22:34 2006 +0200
+++ b/include/asound.h	Sat May 27 19:53:59 2006 +0100
@@ -818,6 +818,44 @@ struct snd_ctl_elem_value {
         unsigned char reserved[128-sizeof(struct timespec)];
 };
 
+struct snd_ctl_misc {
+	u32 version;
+	u32 length;
+	unsigned char message[];
+};
+
+struct snd_ctl_elem_db_scale_type1 {
+	/* Type=1 for simple
+	 * dB = (X * db_per_devision) - db_offset
+	 * This equation is used instead of a more normal
+	 * dB = (( X - db_offset) * db_numerator) / db_denominator
+	 * in order to avoid any devision by zero possibilities,
+	 * save 4 bytes per db_scale and avoid floating point maths.
+	 *
+	 * Types >= 2 are for future more complex conversion functions.
+	 * All types have to have a length value, in case the user land
+	 * alsa lib does not yet understand the type. The length field allows
+	 * user land to safely skip this TLV.
+	 */
+	u32 type; /* 1 */
+	u32 length; /* 12 */
+	s32 db_per_devision; /* Use db per devision * 100 */
+	s32 db_offset; /* Use dB offset * 100. This identifies the 0dB point */
+	s32 db_mute; /* The value, before conversion, that is equivalent to MUTE */
+};
+
+struct snd_ctl_elem_info2 {
+	u32 length;
+	unsigned char *message;
+};
+
+
+#define SND_MISC_DB_SCALE 2
+/* Request */
+#define SND_MISC_ELEM_NUMID 2
+/* Response */
+#define SND_MISC_ELEM_DB_SCALE 2
+
 enum {
 	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
 	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
@@ -833,6 +871,7 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_REMOVE = _IOWR('U', 0x19, struct snd_ctl_elem_id),
 	SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE = _IOWR('U', 0x20, int),
 	SNDRV_CTL_IOCTL_HWDEP_INFO = _IOR('U', 0x21, struct snd_hwdep_info),
+	SNDRV_CTL_IOCTL_MISC = _IOR('U', 0x22, struct snd_ctl_misc),
 	SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE = _IOR('U', 0x30, int),
 	SNDRV_CTL_IOCTL_PCM_INFO = _IOWR('U', 0x31, struct snd_pcm_info),
 	SNDRV_CTL_IOCTL_PCM_PREFER_SUBDEVICE = _IOW('U', 0x32, int),
diff -r 551da56a592c include/control.h
--- a/include/control.h	Fri May 19 19:22:34 2006 +0200
+++ b/include/control.h	Sat May 27 19:53:59 2006 +0100
@@ -30,6 +30,7 @@ typedef int (snd_kcontrol_info_t) (struc
 typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
 typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
 typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
+typedef int (snd_kcontrol_info2_t) (struct snd_kcontrol * kcontrol, int type, struct snd_ctl_elem_info2 * uinfo);
 
 struct snd_kcontrol_new {
 	snd_ctl_elem_iface_t iface;	/* interface identifier */
@@ -42,6 +43,7 @@ struct snd_kcontrol_new {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	snd_kcontrol_info2_t *info2;
 	unsigned long private_value;
 };
 
@@ -58,6 +60,7 @@ struct snd_kcontrol {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	snd_kcontrol_info2_t *info2;
 	unsigned long private_value;
 	void *private_data;
 	void (*private_free)(struct snd_kcontrol *kcontrol);
diff -r 551da56a592c pci/ca0106/ca0106_mixer.c
--- a/pci/ca0106/ca0106_mixer.c	Fri May 19 19:22:34 2006 +0200
+++ b/pci/ca0106/ca0106_mixer.c	Sat May 27 19:53:59 2006 +0100
@@ -362,6 +362,28 @@ static int snd_ca0106_spdif_put(struct s
         return change;
 }
 
+static struct snd_ctl_elem_db_scale_type1 db_scale_1 =
+{
+ /* Scale from -51.50 dB to +12.00 dB in steps of 0.25 dB. 256 steps. */
+	.type=1,
+	.length=12,
+	.db_per_devision=25,
+	.db_offset=5175,
+	.db_mute=0
+};
+
+static int snd_ca0106_info2_db_scale_1(struct snd_kcontrol *kcontrol, int type,
+				  struct snd_ctl_elem_info2 *uinfo)
+{
+	if (type==1) {
+		uinfo->length=sizeof(struct snd_ctl_elem_db_scale_type1);
+		uinfo->message=(unsigned char*) &db_scale_1;
+        	return 0;
+	}
+	return -EINVAL; /* type not implemented */
+}
+
+
 static int snd_ca0106_volume_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
@@ -466,13 +488,14 @@ static int snd_ca0106_i2c_volume_put(str
 	return change;
 }
 
-#define CA_VOLUME(xname,chid,reg) \
+#define CA_VOLUME(xname,chid,reg,db_function) \
 {								\
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname,	\
 	.info =	 snd_ca0106_volume_info,			\
 	.get =   snd_ca0106_volume_get,				\
 	.put =   snd_ca0106_volume_put,				\
-	.private_value = ((chid) << 8) | (reg)			\
+	.private_value = ((chid) << 8) | (reg),			\
+	.info2 = db_function					\
 }
 
 #define I2C_VOLUME(xname,chid) \
@@ -487,25 +510,33 @@ static int snd_ca0106_i2c_volume_put(str
 
 static struct snd_kcontrol_new snd_ca0106_volume_ctls[] __devinitdata = {
 	CA_VOLUME("Analog Front Playback Volume",
-		  CONTROL_FRONT_CHANNEL, PLAYBACK_VOLUME2),
+		  CONTROL_FRONT_CHANNEL, PLAYBACK_VOLUME2,
+		  &snd_ca0106_info2_db_scale_1),
         CA_VOLUME("Analog Rear Playback Volume",
-		  CONTROL_REAR_CHANNEL, PLAYBACK_VOLUME2),
+		  CONTROL_REAR_CHANNEL, PLAYBACK_VOLUME2,
+		  &snd_ca0106_info2_db_scale_1),
 	CA_VOLUME("Analog Center/LFE Playback Volume",
-		  CONTROL_CENTER_LFE_CHANNEL, PLAYBACK_VOLUME2),
+		  CONTROL_CENTER_LFE_CHANNEL, PLAYBACK_VOLUME2,
+		  &snd_ca0106_info2_db_scale_1),
         CA_VOLUME("Analog Side Playback Volume",
-		  CONTROL_UNKNOWN_CHANNEL, PLAYBACK_VOLUME2),
-
+		  CONTROL_UNKNOWN_CHANNEL, PLAYBACK_VOLUME2,
+		  &snd_ca0106_info2_db_scale_1),
         CA_VOLUME("IEC958 Front Playback Volume",
-		  CONTROL_FRONT_CHANNEL, PLAYBACK_VOLUME1),
+		  CONTROL_FRONT_CHANNEL, PLAYBACK_VOLUME1,
+		  &snd_ca0106_info2_db_scale_1),
 	CA_VOLUME("IEC958 Rear Playback Volume",
-		  CONTROL_REAR_CHANNEL, PLAYBACK_VOLUME1),
+		  CONTROL_REAR_CHANNEL, PLAYBACK_VOLUME1,
+		  &snd_ca0106_info2_db_scale_1),
 	CA_VOLUME("IEC958 Center/LFE Playback Volume",
-		  CONTROL_CENTER_LFE_CHANNEL, PLAYBACK_VOLUME1),
+		  CONTROL_CENTER_LFE_CHANNEL, PLAYBACK_VOLUME1,
+		  &snd_ca0106_info2_db_scale_1),
 	CA_VOLUME("IEC958 Unknown Playback Volume",
-		  CONTROL_UNKNOWN_CHANNEL, PLAYBACK_VOLUME1),
+		  CONTROL_UNKNOWN_CHANNEL, PLAYBACK_VOLUME1,
+		  &snd_ca0106_info2_db_scale_1),
 
         CA_VOLUME("CAPTURE feedback Playback Volume",
-		  1, CAPTURE_CONTROL),
+		  1, CAPTURE_CONTROL,
+		  &snd_ca0106_info2_db_scale_1),
 
         I2C_VOLUME("Phone Capture Volume", 0),
         I2C_VOLUME("Mic Capture Volume", 1),

[-- Attachment #3: alsa-lib-db14.diff --]
[-- Type: text/x-patch, Size: 12398 bytes --]

diff -r d5a6770e7faf include/control.h
--- a/include/control.h	Fri May 19 18:26:41 2006 +0200
+++ b/include/control.h	Sat May 27 19:54:17 2006 +0100
@@ -52,6 +52,9 @@ typedef struct snd_aes_iec958 {
 
 /** CTL card info container */
 typedef struct _snd_ctl_card_info snd_ctl_card_info_t;
+
+/** CTL misc container */
+typedef struct _snd_ctl_misc snd_ctl_misc_t;
 
 /** CTL element identifier container */
 typedef struct _snd_ctl_elem_id snd_ctl_elem_id_t;
@@ -212,6 +215,7 @@ int snd_ctl_poll_descriptors_revents(snd
 int snd_ctl_poll_descriptors_revents(snd_ctl_t *ctl, struct pollfd *pfds, unsigned int nfds, unsigned short *revents);
 int snd_ctl_subscribe_events(snd_ctl_t *ctl, int subscribe);
 int snd_ctl_card_info(snd_ctl_t *ctl, snd_ctl_card_info_t *info);
+int snd_ctl_misc(snd_ctl_t *ctl, snd_ctl_misc_t *misc);
 int snd_ctl_elem_list(snd_ctl_t *ctl, snd_ctl_elem_list_t * list);
 int snd_ctl_elem_info(snd_ctl_t *ctl, snd_ctl_elem_info_t *info);
 int snd_ctl_elem_read(snd_ctl_t *ctl, snd_ctl_elem_value_t *value);
@@ -485,6 +489,8 @@ int snd_hctl_elem_info(snd_hctl_elem_t *
 int snd_hctl_elem_info(snd_hctl_elem_t *elem, snd_ctl_elem_info_t * info);
 int snd_hctl_elem_read(snd_hctl_elem_t *elem, snd_ctl_elem_value_t * value);
 int snd_hctl_elem_write(snd_hctl_elem_t *elem, snd_ctl_elem_value_t * value);
+int snd_hctl_elem_misc(snd_hctl_elem_t *elem, snd_ctl_misc_t * misc);
+int snd_hctl_elem_get_db_gain(snd_hctl_elem_t *elem, long volume, long *db_gain);
 
 snd_hctl_t *snd_hctl_elem_get_hctl(snd_hctl_elem_t *elem);
 
diff -r d5a6770e7faf include/local.h
--- a/include/local.h	Fri May 19 18:26:41 2006 +0200
+++ b/include/local.h	Sat May 27 19:54:17 2006 +0100
@@ -46,6 +46,7 @@
 #define _snd_pcm_status sndrv_pcm_status
 
 #define _snd_ctl_card_info sndrv_ctl_card_info
+#define _snd_ctl_misc sndrv_ctl_misc
 #define _snd_ctl_elem_id sndrv_ctl_elem_id
 #define _snd_ctl_elem_list sndrv_ctl_elem_list
 #define _snd_ctl_elem_info sndrv_ctl_elem_info
diff -r d5a6770e7faf include/sound/asound.h
--- a/include/sound/asound.h	Fri May 19 18:26:41 2006 +0200
+++ b/include/sound/asound.h	Sat May 27 19:54:17 2006 +0100
@@ -26,6 +26,7 @@
 #if defined(LINUX) || defined(__LINUX__) || defined(__linux__)
 
 #include <linux/ioctl.h>
+#include <inttypes.h>
 
 #ifdef __KERNEL__
 
@@ -847,6 +848,18 @@ struct sndrv_ctl_elem_value {
         unsigned char reserved[128-sizeof(struct timespec)];
 };
 
+struct sndrv_ctl_misc {
+	uint32_t version;  /* This should be uint32_t */
+	uint32_t length; /* This should be uint32_t */
+	unsigned char message[];
+};
+
+#define SND_MISC_DB_SCALE 2
+/* Request */
+#define SND_MISC_ELEM_NUMID 2
+/* Response */
+#define SND_MISC_ELEM_DB_SCALE_TYPE1 1
+
 enum {
 	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
 	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct sndrv_ctl_card_info),
@@ -862,6 +875,7 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_REMOVE = _IOWR('U', 0x19, struct sndrv_ctl_elem_id),
 	SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE = _IOWR('U', 0x20, int),
 	SNDRV_CTL_IOCTL_HWDEP_INFO = _IOR('U', 0x21, struct sndrv_hwdep_info),
+	SNDRV_CTL_IOCTL_MISC = _IOR('U', 0x22, struct sndrv_ctl_misc),
 	SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE = _IOR('U', 0x30, int),
 	SNDRV_CTL_IOCTL_PCM_INFO = _IOWR('U', 0x31, struct sndrv_pcm_info),
 	SNDRV_CTL_IOCTL_PCM_PREFER_SUBDEVICE = _IOW('U', 0x32, int),
diff -r d5a6770e7faf src/control/control.c
--- a/src/control/control.c	Fri May 19 18:26:41 2006 +0200
+++ b/src/control/control.c	Sat May 27 19:54:17 2006 +0100
@@ -234,6 +234,20 @@ int snd_ctl_card_info(snd_ctl_t *ctl, sn
 {
 	assert(ctl && info);
 	return ctl->ops->card_info(ctl, info);
+}
+
+/**
+ * \brief Get/Set misc related information
+ * \param ctl CTL handle
+ * \param misc pointer
+ * \return 0 on success otherwise a negative error code
+ */
+int snd_ctl_misc(snd_ctl_t *ctl, snd_ctl_misc_t *misc)
+{
+	int err;
+	assert(ctl && misc);
+	err = ctl->ops->misc(ctl, misc);
+	return err;
 }
 
 /**
diff -r d5a6770e7faf src/control/control_hw.c
--- a/src/control/control_hw.c	Fri May 19 18:26:41 2006 +0200
+++ b/src/control/control_hw.c	Sat May 27 19:54:17 2006 +0100
@@ -128,6 +128,16 @@ static int snd_ctl_hw_card_info(snd_ctl_
 	return 0;
 }
 
+static int snd_ctl_hw_misc(snd_ctl_t *handle, snd_ctl_misc_t *misc)
+{
+	snd_ctl_hw_t *hw = handle->private_data;
+	if (ioctl(hw->fd, SNDRV_CTL_IOCTL_MISC, misc) < 0) {
+		//SYSERR("SNDRV_CTL_IOCTL_MISC failed");
+		return -errno;
+	}
+	return 0;
+}
+
 static int snd_ctl_hw_elem_list(snd_ctl_t *handle, snd_ctl_elem_list_t *list)
 {
 	snd_ctl_hw_t *hw = handle->private_data;
@@ -296,6 +306,7 @@ snd_ctl_ops_t snd_ctl_hw_ops = {
 	.async = snd_ctl_hw_async,
 	.subscribe_events = snd_ctl_hw_subscribe_events,
 	.card_info = snd_ctl_hw_card_info,
+	.misc = snd_ctl_hw_misc,
 	.element_list = snd_ctl_hw_elem_list,
 	.element_info = snd_ctl_hw_elem_info,
 	.element_add = snd_ctl_hw_elem_add,
diff -r d5a6770e7faf src/control/control_local.h
--- a/src/control/control_local.h	Fri May 19 18:26:41 2006 +0200
+++ b/src/control/control_local.h	Sat May 27 19:54:17 2006 +0100
@@ -27,6 +27,7 @@ typedef struct _snd_ctl_ops {
 	int (*async)(snd_ctl_t *handle, int sig, pid_t pid);
 	int (*subscribe_events)(snd_ctl_t *handle, int subscribe);
 	int (*card_info)(snd_ctl_t *handle, snd_ctl_card_info_t *info);
+	int (*misc)(snd_ctl_t *handle, snd_ctl_misc_t *info);
 	int (*element_list)(snd_ctl_t *handle, snd_ctl_elem_list_t *list);
 	int (*element_info)(snd_ctl_t *handle, snd_ctl_elem_info_t *info);
 	int (*element_add)(snd_ctl_t *handle, snd_ctl_elem_info_t *info);
diff -r d5a6770e7faf src/control/hcontrol.c
--- a/src/control/hcontrol.c	Fri May 19 18:26:41 2006 +0200
+++ b/src/control/hcontrol.c	Sat May 27 19:54:17 2006 +0100
@@ -49,6 +49,7 @@ to reduce overhead accessing the real co
 #include <fcntl.h>
 #include <sys/ioctl.h>
 #include <pthread.h>
+#include <inttypes.h>
 #ifndef DOC_HIDDEN
 #define __USE_GNU
 #endif
@@ -757,6 +758,181 @@ int snd_hctl_handle_events(snd_hctl_t *h
 	return count;
 }
 
+
+static int uint32_to_message(snd_ctl_misc_t *misc, uint32_t value)
+{
+	unsigned int pointer = misc->length;
+	uint32_t *store = (uint32_t*) &misc->message[pointer];
+	*store = value;
+	misc->length += sizeof(uint32_t);
+	return 0;
+}
+
+static int message_to_uint32(snd_ctl_misc_t *misc, uint32_t *value)
+{
+	unsigned int pointer = misc->length;
+	uint32_t *store = (uint32_t*) &misc->message[pointer];
+	*value = *store;
+	misc->length += sizeof(uint32_t);
+	return 0;
+}
+
+
+static int add_int32_tlv_to_container(snd_ctl_misc_t *misc, uint32_t type, uint32_t value)
+{
+	uint32_to_message(misc, type);
+	uint32_to_message(misc, 4);
+	uint32_to_message(misc, value);
+	/* FIXME: Check for overflows */
+	return 0;
+}
+
+static int add_message_tlv_to_container(
+	snd_ctl_misc_t *misc, uint32_t length, unsigned char *message)
+{
+	unsigned int pointer = misc->length;
+	unsigned int n;
+	for(n=0; n<length; n++) {
+		misc->message[pointer++] = message[n];
+	}
+	misc->length = pointer;
+	/* FIXME: Check for overflows */
+	return 0;
+}
+
+
+static int container_open(snd_ctl_misc_t *misc, uint32_t type)
+{
+	uint32_to_message(misc, type);
+	uint32_to_message(misc, 0); /* This gets updated at container close */
+	return 0;
+}
+
+static int container_close(snd_ctl_misc_t *misc)
+{
+	uint32_t *store = (uint32_t*) &misc->message[sizeof(uint32_t)];
+	*store = misc->length;
+	return 0;
+}
+
+
+struct snd_ctl_elem_db_scale_type1 {
+	/* Type=1 for simple
+	 * dB = (X * db_per_devision) - db_offset
+	 * This equation is used instead of a more normal
+	 * dB = (( X - db_offset) * db_numerator) / db_denominator
+	 * in order to avoid any devision by zero possibilities,
+	 * save 4 bytes per db_scale and avoid floating point maths.
+	 *
+	 * Types >= 2 are for future more complex conversion functions.
+	 * All types have to have a length value, in case the user land
+	 * alsa lib does not yet understand the type. The length field allows
+	 * user land to safely skip this TLV.
+	 */
+	uint32_t type; /* 1 */
+	uint32_t length; /* 12 */
+	int32_t db_per_devision; /* Use db per devision * 100 */
+	int32_t db_offset; /* Use dB offset * 100. This identifies the 0dB point */
+	int32_t db_mute; /* The value, before conversion, that is equivalent to MUTE */
+};
+
+/**
+ * \brief Get misc information for an HCTL element
+ * \param elem HCTL element
+ * \param info HCTL element information
+ * \return 0 otherwise a negative error code on failure
+ */
+int snd_hctl_elem_get_db_gain(snd_hctl_elem_t *elem, long volume, long *db_gain)
+{
+	snd_ctl_misc_t *misc;
+	int err=0;
+	unsigned int received_length;
+	unsigned int tlv_type;
+	unsigned int tlv_length;
+	unsigned int container_type;
+	unsigned int container_length;
+	struct snd_ctl_elem_db_scale_type1 *db_scale_1;
+
+	assert(elem);
+	assert(elem->hctl);
+	misc=malloc(2048);
+	if (misc == NULL) {
+		err = -ENOMEM;
+		return err;
+	}
+	misc->version=1;
+	misc->length=0;
+	container_open(misc, SND_MISC_DB_SCALE);
+	add_int32_tlv_to_container(misc, SND_MISC_ELEM_NUMID, elem->id.numid);
+	container_close(misc);
+	misc->length=2040;
+	
+	err = snd_ctl_misc(elem->hctl->ctl, misc);
+	if (err) goto _err;
+
+	received_length = misc->length;
+	misc->length=0;
+	message_to_uint32(misc, &container_type);
+	switch (container_type) {
+		case SND_MISC_DB_SCALE:
+			message_to_uint32(misc, &container_length);
+			db_scale_1 = (struct snd_ctl_elem_db_scale_type1*) &misc->message[misc->length];
+			/* FIXME: Do more sanity checks here */
+			message_to_uint32(misc, &tlv_type);
+			switch (tlv_type) {
+				case SND_MISC_ELEM_DB_SCALE_TYPE1:
+					message_to_uint32(misc, &tlv_length);
+					if (tlv_length != 12) {
+						err = -EINVAL;
+						goto _err;
+					}
+					misc->length+=tlv_length;
+					if (volume == db_scale_1->db_mute) {
+						*db_gain=-9999999; /* Muted */
+						break;
+					}
+					/* Scale ca0106: from -51.50 dB to +12.00 dB in steps of 0.75 dB. 256 steps. */
+					/* Scale emu10k1 dsp controls: from -39.60 dB to 0.00 dB in steps of 0.4 dB. 100 steps. */
+					*db_gain=(volume * db_scale_1->db_per_devision) - db_scale_1->db_offset; 
+					break;
+				default:
+					err = -EINVAL;
+					goto _err;
+					break;
+			}
+			break;
+		default:
+			err = -EINVAL;
+			goto _err;
+	}
+
+_err:
+	free(misc);
+	return err;
+#if 0
+	switch (tlv_value) {
+	case 1:  /* Scale from -51.50 dB to +12.00 dB in steps of 0.75 dB. 256 steps. */
+		if (volume == 0) {
+			*db_gain=-9999999; /* Muted */
+			break;
+		}
+		*db_gain=(volume * 25) - 5175; /* For ca0106 controls */
+		break;
+	case 2:  /* Scale from -39.60 dB to 0.00 dB in steps of 0.4 dB. 100 steps. */
+		if (volume == 0) {
+			*db_gain=-9999999; /* Muted */
+			break;
+		}
+		*db_gain=(volume * 40) - 4000; /* For emu10k1 dsp controls */
+		break;
+	default:
+		*db_gain=-9999999; /* Muted */
+		return 1;
+	}
+	return 0;
+#endif
+}
+
 /**
  * \brief Get information for an HCTL element
  * \param elem HCTL element
diff -r d5a6770e7faf src/mixer/simple_none.c
--- a/src/mixer/simple_none.c	Fri May 19 18:26:41 2006 +0200
+++ b/src/mixer/simple_none.c	Sat May 27 19:54:17 2006 +0100
@@ -971,12 +971,41 @@ static int get_volume_ops(snd_mixer_elem
 	return 0;
 }
 
-static int get_dB_ops(snd_mixer_elem_t *elem ATTRIBUTE_UNUSED,
-		      int dir ATTRIBUTE_UNUSED,
-		      snd_mixer_selem_channel_id_t channel ATTRIBUTE_UNUSED,
-		      long *value ATTRIBUTE_UNUSED)
-{
-	return -ENXIO;
+static int get_dB_ops(snd_mixer_elem_t *elem,
+		      int dir,
+		      snd_mixer_selem_channel_id_t channel,
+		      long *value)
+{
+	selem_none_t *s = snd_mixer_elem_get_private(elem);
+	selem_ctl_t *c;
+	int err=-EINVAL;
+	long volume, db_gain;
+	if (dir==SM_PLAY) {
+		c = &s->ctls[CTL_PLAYBACK_VOLUME];
+		if (c->type==2) {
+			if (get_volume_ops(elem, dir, channel, &volume)) 
+				goto _err;
+			if ((err = snd_hctl_elem_get_db_gain(c->elem, volume, &db_gain)) < 0)
+				goto _err;
+		} else
+			goto _err;
+	} else if (dir==SM_CAPT) {
+		c = &s->ctls[CTL_CAPTURE_VOLUME];
+		if (c->type==2) {
+			if (get_volume_ops(elem, dir, channel, &volume)) 
+				goto _err;
+			if ((err = snd_hctl_elem_get_db_gain(c->elem, volume, &db_gain)) < 0)
+				goto _err;
+		} else
+			goto _err;
+
+	} else
+		goto _err;
+	err=0;
+	*value=db_gain;
+_err:
+	//if (err) printf("get_dB_ops:err=%d\n",err);
+	return err;
 }
 
 static int get_switch_ops(snd_mixer_elem_t *elem, int dir,

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]



[-- Attachment #5: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: dB gain

[Takashi Iwai]

At Sat, 27 May 2006 20:03:44 +0100,
James Courtier-Dutton wrote:
> 
> Hi,
> 
> I have made a new attempt at adding support for dB gain reports in
> alsamixer.
> 
> See the attached diffs against the current alsa hg repository. 27-5-2006
> 
> It shows an example of support for the snd-ca0106 driver.
> As one can see, this adds minimal amounts of code to each sound card
> driver. 22 new lines + 1 per volume control making 33 lines total.
> 
> Any comments?

Apart from some small implementation details, it looks almost fine.
One remaining thing is the definition of protocol.  Shouldn't be numid
regardless of container type?  Now the numid is checked inside the
block of db container type.  I think this can be outside.

Also, a bit more sensible names are preferred for new stuff.
"misc" sounds too ambiguous to me, and "info2" is...

I'm not a native speaker, so I expect someone elese has better
proposal.


Takashi

Re: dB gain

[Jaroslav Kysela]

On Mon, 29 May 2006, Takashi Iwai wrote:

> Also, a bit more sensible names are preferred for new stuff.
> "misc" sounds too ambiguous to me, and "info2" is...

Perhaps 'desc' like description or 'extra' is more appropriate.

Also, I'm not sure, if hint is the right behaviour. Probably, the better 
way is to create a new identifier and ecapsulate min, max and step values 
into 32-bit word, so we won't eat much memory and the information for 
alsa-lib will be more abstract for your case. Of course, some hardware can 
have non-continuous dB scale so special expressions must be in alsa-lib.

					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>

Re: dB gain

[Jaroslav Kysela]

On Mon, 29 May 2006, Jaroslav Kysela wrote:

> On Mon, 29 May 2006, Takashi Iwai wrote:
> 
> > Also, a bit more sensible names are preferred for new stuff.
> > "misc" sounds too ambiguous to me, and "info2" is...
> 
> Perhaps 'desc' like description or 'extra' is more appropriate.
> 
> Also, I'm not sure, if hint is the right behaviour. Probably, the better 
> way is to create a new identifier and ecapsulate min, max and step values 
> into 32-bit word, so we won't eat much memory and the information for 
> alsa-lib will be more abstract for your case. Of course, some hardware can 
> have non-continuous dB scale so special expressions must be in alsa-lib.

Replying myself:

For example, 32-bits can be separated to:

12-bits manimal value (0-4095) => 0.05dB => -154.75dB - 50dB
12-bits maximal value (0-4095) => 0.05dB => -50dB - 154.75dB
8-bits step value     (0-255)  => 0.05dB => 0-12.75dB

It will satisfy nicely requirements for both drivers and has plenty space
to store more wide ranges.

Also, handling of first value as mute can be specified as a new 
indentifier in the TLV tree.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs

Re: dB gain

[Takashi Iwai]

At Mon, 29 May 2006 16:17:40 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Mon, 29 May 2006, Jaroslav Kysela wrote:
> 
> > On Mon, 29 May 2006, Takashi Iwai wrote:
> > 
> > > Also, a bit more sensible names are preferred for new stuff.
> > > "misc" sounds too ambiguous to me, and "info2" is...
> > 
> > Perhaps 'desc' like description or 'extra' is more appropriate.
> > 
> > Also, I'm not sure, if hint is the right behaviour. Probably, the better 
> > way is to create a new identifier and ecapsulate min, max and step values 
> > into 32-bit word, so we won't eat much memory and the information for 
> > alsa-lib will be more abstract for your case. Of course, some hardware can 
> > have non-continuous dB scale so special expressions must be in alsa-lib.
> 
> Replying myself:
> 
> For example, 32-bits can be separated to:
> 
> 12-bits manimal value (0-4095) => 0.05dB => -154.75dB - 50dB
> 12-bits maximal value (0-4095) => 0.05dB => -50dB - 154.75dB
> 8-bits step value     (0-255)  => 0.05dB => 0-12.75dB
> 
> It will satisfy nicely requirements for both drivers and has plenty space
> to store more wide ranges.
> 
> Also, handling of first value as mute can be specified as a new 
> indentifier in the TLV tree.

Well, the principal question is whether we need such a hack at all.

We don't need to store the values for _each_ element in the driver
side.  Most of elements have the same dB range, so the record is
shared.  For USB and HD-audio, the records are dynamically created,
anyway.

What we'll get by this compression is only the reduction of ioctl
size.  But, these ioctls are called not so often and no time-critical
task.


Takashi

Re: dB gain

[Jaroslav Kysela]

On Mon, 29 May 2006, Takashi Iwai wrote:

> What we'll get by this compression is only the reduction of ioctl
> size.  But, these ioctls are called not so often and no time-critical
> task.

Nope. I'm talking about saving code in alsa-lib. Having a special case in 
alsa-lib for all hints is not really good. If you can store all necessary 
ranges to 32-bit value (which is allocated anyway) is a win. Of course, 
all mechanisms as James proposed will not be changed. I see only one 
problem why my proposal is not acceptable - we must store also the 
identifier type to the 32-bit value in the kernel side. Also, having both 
min and max values is not necessary (it can be evaluated from step).

What about this (leaving 11 bits free for future use):

/*
 * macros defining dB hints
 */

/* continuous dB steps, input values are dB * 100 */
/* min value must be from -15475 .. 5000 */
/* step value must be from 0 .. 1270 */
#define SND_DB_CONT(mute, min, step) \
        (((mute) ? 1 : 0) | \
         (((((min) + 15475) / 5) & 0x0fff) << 12) | \
         ((((step) / 5) & 0xff) << 24))
/* special dB hint */
#define SND_DB_HINT(hint) \
        (0xff000000 | hint)


						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs

Re: dB gain

[James Courtier-Dutton]

Jaroslav Kysela wrote:
> On Mon, 29 May 2006, Takashi Iwai wrote:
> 
>> What we'll get by this compression is only the reduction of ioctl
>> size.  But, these ioctls are called not so often and no time-critical
>> task.
> 
> Nope. I'm talking about saving code in alsa-lib. Having a special case in 
> alsa-lib for all hints is not really good. If you can store all necessary 
> ranges to 32-bit value (which is allocated anyway) is a win. Of course, 
> all mechanisms as James proposed will not be changed. I see only one 
> problem why my proposal is not acceptable - we must store also the 
> identifier type to the 32-bit value in the kernel side. Also, having both 
> min and max values is not necessary (it can be evaluated from step).
> 
I don't understand where you think you are making a win with these
32-bit values? None of the information is stored in alsa-lib after the
function has returned to the user app. It is all temporary use.

I don't have min and max values.
I have:
int32_t db_per_devision; /* Use db per devision * 100 */
int32_t db_offset; /* Use dB offset * 100. This identifies the 0dB point */
int32_t db_mute; /* The value, before conversion, that is equivalent to
MUTE */

Now, not all controls have a mute at the minimum value.
If the minimum % is a mute, then the db_mute == this minimum % value.
If the minimum % is not mute, then the db_mute equals a value outside
the % range, so the comparison to db_mute is always false for that control.

The current type implemented is:
For simple  dB*100 = (X * db_per_devision) - db_offset
with a special case for detecting the mute value.

More complex conversion functions will have a different equation, and
therefore possibly a different amount of parameters, so one would just
implement a new type, and parse as many parameters from it as we need.

James

Re: dB gain

[Takashi Iwai]

At Mon, 29 May 2006 16:12:34 +0100,
James Courtier-Dutton wrote:
> 
> Jaroslav Kysela wrote:
> > On Mon, 29 May 2006, Takashi Iwai wrote:
> > 
> >> What we'll get by this compression is only the reduction of ioctl
> >> size.  But, these ioctls are called not so often and no time-critical
> >> task.
> > 
> > Nope. I'm talking about saving code in alsa-lib. Having a special case in 
> > alsa-lib for all hints is not really good. If you can store all necessary 
> > ranges to 32-bit value (which is allocated anyway) is a win. Of course, 
> > all mechanisms as James proposed will not be changed. I see only one 
> > problem why my proposal is not acceptable - we must store also the 
> > identifier type to the 32-bit value in the kernel side. Also, having both 
> > min and max values is not necessary (it can be evaluated from step).
> > 
> I don't understand where you think you are making a win with these
> 32-bit values? None of the information is stored in alsa-lib after the
> function has returned to the user app. It is all temporary use.

Maybe cached values to avoid too frequent ioctls?

But, still it won't save so much memory at all, given that we have at
most 100 controls with volumes...


Takashi

Re: dB gain

[Jaroslav Kysela]

On Mon, 29 May 2006, Takashi Iwai wrote:

> At Mon, 29 May 2006 16:12:34 +0100,
> James Courtier-Dutton wrote:
> > 
> > Jaroslav Kysela wrote:
> > > On Mon, 29 May 2006, Takashi Iwai wrote:
> > > 
> > >> What we'll get by this compression is only the reduction of ioctl
> > >> size.  But, these ioctls are called not so often and no time-critical
> > >> task.
> > > 
> > > Nope. I'm talking about saving code in alsa-lib. Having a special case in 
> > > alsa-lib for all hints is not really good. If you can store all necessary 
> > > ranges to 32-bit value (which is allocated anyway) is a win. Of course, 
> > > all mechanisms as James proposed will not be changed. I see only one 
> > > problem why my proposal is not acceptable - we must store also the 
> > > identifier type to the 32-bit value in the kernel side. Also, having both 
> > > min and max values is not necessary (it can be evaluated from step).
> > > 
> > I don't understand where you think you are making a win with these
> > 32-bit values? None of the information is stored in alsa-lib after the
> > function has returned to the user app. It is all temporary use.
> 
> Maybe cached values to avoid too frequent ioctls?
> 
> But, still it won't save so much memory at all, given that we have at
> most 100 controls with volumes...

Sorry, I was looking to the proposed patch very quickly so my ideas were
based on old implementation which is #if 0 #endifed for alsa-lib (there 
was only one 32-bit hint value specifying the dB expression).

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs

Re: dB gain

[James Courtier-Dutton]

Jaroslav Kysela wrote:
>>
>> But, still it won't save so much memory at all, given that we have at
>> most 100 controls with volumes...
> 
> Sorry, I was looking to the proposed patch very quickly so my ideas were
> based on old implementation which is #if 0 #endifed for alsa-lib (there 
> was only one 32-bit hint value specifying the dB expression).

Yes, I dropped the 32-bit hint value as a result of your comments some
time ago.

The latest patch does things quite a lot differently.

James

Re: dB gain

[James Courtier-Dutton]

Takashi Iwai wrote:
> At Sat, 27 May 2006 20:03:44 +0100,
> James Courtier-Dutton wrote:
>> Hi,
>>
>> I have made a new attempt at adding support for dB gain reports in
>> alsamixer.
>>
>> See the attached diffs against the current alsa hg repository. 27-5-2006
>>
>> It shows an example of support for the snd-ca0106 driver.
>> As one can see, this adds minimal amounts of code to each sound card
>> driver. 22 new lines + 1 per volume control making 33 lines total.
>>
>> Any comments?
> 
> Apart from some small implementation details, it looks almost fine.
> One remaining thing is the definition of protocol.  Shouldn't be numid
> regardless of container type?  Now the numid is checked inside the
> block of db container type.  I think this can be outside.
> 
> Also, a bit more sensible names are preferred for new stuff.
> "misc" sounds too ambiguous to me, and "info2" is...
> 
> I'm not a native speaker, so I expect someone elese has better
> proposal.
> 
> 
> Takashi

As for the naming, I called it misc because it can be used for anything
we want, depending on the TLVs requested. "misc" is short for
miscellaneous. So I thing that "misc" is appropriate, given it's
potential to be used for other stuff in future.


An explanation of the protocol:

The protocol is a request/response protocol
Request:
uint32_t version; // Protocol version
uint32_t length;  // Length of the user land data buffer. This is filled
with the response.
uint8_t data[];   // A block of data bytes malloc by user land,
containing the actual "request".

Inside the data one has TLVs (Type, Length, Values)

For requesting db gain scales:
Request:
uint32_t type = SND_MISC_DB_SCALE;
uint32_t length = 4; /* sizeof(following uint32_t)
uint32_t value = numid; /* Used by snd_ctl_find_numid() */

Note: numid is used because it is present in both userland and kernel
land as a unique identifier of the mixer control in question.
Of course, if we wished to get some different information, the type
would be a different value, and the resulting value could relate to
something else. How the value is interpreted is dependent on the type field.

Response:
This depends on the function used to do the conversion.
uint32_t type = SND_MISC_DB_SCALE;
uint32_t length = length of following data; /* In this example it will
be 20 */
uint8_t data[]  /* data returned from sound card driver info2() */

Note: the info2(kctl, 1, &info2). The "1" tells the driver to return
info2 info about the db_gain function.
For example, if it was "2", we could(not implemented yet) get the driver
to return all the info returned in the current info() function, but in
TLV form.

In a simple example the data[] contains:
uint32_t type = 1  /* For simple  dB = (X * db_per_devision) - db_offset */
uint32_t length = 12 /* 3 following uint32_t */
int32_t db_per_devision; /* Use db per devision * 100 */
int32_t db_offset; /* Use dB offset * 100. This identifies the 0dB point */
int32_t db_mute; /* The value, before conversion, that is equivalent to
MUTE */

There is the potential to request multiple parameters by encapsulating
more different TLVs in the same request, but this example only used one
for simplicity.

If the conversion, % to dB, function is more complex, on would add a new
type, e.g. 2, and then as much data as needed to describe the function.
One would then implement the type 2 processing in alsa-lib.

If there is a mismatch, and the user lib is of a different version from
the kernel, and the required type is not implemented, the GUI mixer app
would simply not display dB gain levels, as is the case with the current
 alsamixer.

James

Re: dB gain

[Takashi Iwai]

At Mon, 29 May 2006 15:50:01 +0100,
James Courtier-Dutton wrote:
> 
> Takashi Iwai wrote:
> > At Sat, 27 May 2006 20:03:44 +0100,
> > James Courtier-Dutton wrote:
> >> Hi,
> >>
> >> I have made a new attempt at adding support for dB gain reports in
> >> alsamixer.
> >>
> >> See the attached diffs against the current alsa hg repository. 27-5-2006
> >>
> >> It shows an example of support for the snd-ca0106 driver.
> >> As one can see, this adds minimal amounts of code to each sound card
> >> driver. 22 new lines + 1 per volume control making 33 lines total.
> >>
> >> Any comments?
> > 
> > Apart from some small implementation details, it looks almost fine.
> > One remaining thing is the definition of protocol.  Shouldn't be numid
> > regardless of container type?  Now the numid is checked inside the
> > block of db container type.  I think this can be outside.
> > 
> > Also, a bit more sensible names are preferred for new stuff.
> > "misc" sounds too ambiguous to me, and "info2" is...
> > 
> > I'm not a native speaker, so I expect someone elese has better
> > proposal.
> > 
> > 
> > Takashi
> 
> As for the naming, I called it misc because it can be used for anything
> we want, depending on the TLVs requested. "misc" is short for
> miscellaneous.

Yes, but it's an adjective.
So it sounds as if "snd_ctl_sexy" :)


> So I thing that "misc" is appropriate, given it's
> potential to be used for other stuff in future.
> 
> 
> An explanation of the protocol:
> 
> The protocol is a request/response protocol
> Request:
> uint32_t version; // Protocol version
> uint32_t length;  // Length of the user land data buffer. This is filled
> with the response.
> uint8_t data[];   // A block of data bytes malloc by user land,
> containing the actual "request".
> 
> Inside the data one has TLVs (Type, Length, Values)
> 
> For requesting db gain scales:
> Request:
> uint32_t type = SND_MISC_DB_SCALE;
> uint32_t length = 4; /* sizeof(following uint32_t)
> uint32_t value = numid; /* Used by snd_ctl_find_numid() */
> 
> Note: numid is used because it is present in both userland and kernel
> land as a unique identifier of the mixer control in question.
> Of course, if we wished to get some different information, the type
> would be a different value, and the resulting value could relate to
> something else. How the value is interpreted is dependent on the type field.

I agree to use numid.
My argument is that numid can belong to the common header block.

The code you find in control.c is anyway a kind of object oriented.
It's a message to a control element.
But in this protocol, you get numid at very last, even depending on
the type.  Thus the code looks like

	type = get_type();
	len = get_length();
	switch (type) {
	case DB_SCALE:
		numid = get_value();
		kctl = get_kcontrol(numid);
		kctl->info2(kctl, ptr);
		...
	}

When you add more different types,

	type = get_type();
	len = get_length();
	switch (type) {
	case DB_SCALE:
		numid = get_value();
		kctl = get_kcontrol(numid);
		kctl->info2(kctl, ptr);
		...
	case ANOTHER_DB_SCALE:
		numid = get_value();
		kctl = get_kcontrol(numid);
		kctl->info2(kctl, ptr);
		...
	case YET_ANOTHER_DATA:
		numid = get_value();
		kctl = get_kcontrol(numid);
		kctl->info2(kctl, ptr);
		...

	}

If the numid is in the common block, we can simply extract numid
first, then evaluate the type and value later, eventually call the
callback at the single place.


Takashi

Re: dB gain

[James Courtier-Dutton]

Takashi Iwai wrote:
> 
> I agree to use numid.
> My argument is that numid can belong to the common header block.
> 
> The code you find in control.c is anyway a kind of object oriented.
> It's a message to a control element.
> But in this protocol, you get numid at very last, even depending on
> the type.  Thus the code looks like
> 
> 	type = get_type();
> 	len = get_length();
> 	switch (type) {
> 	case DB_SCALE:
> 		numid = get_value();
> 		kctl = get_kcontrol(numid);
> 		kctl->info2(kctl, ptr);
> 		...
> 	}
> 
> When you add more different types,
> 
> 	type = get_type();
> 	len = get_length();
> 	switch (type) {
> 	case DB_SCALE:
> 		numid = get_value();
> 		kctl = get_kcontrol(numid);
> 		kctl->info2(kctl, ptr);
> 		...
> 	case ANOTHER_DB_SCALE:
> 		numid = get_value();
> 		kctl = get_kcontrol(numid);
> 		kctl->info2(kctl, ptr);
> 		...
> 	case YET_ANOTHER_DATA:
> 		numid = get_value();
> 		kctl = get_kcontrol(numid);
> 		kctl->info2(kctl, ptr);
> 		...
> 
> 	}
> 
> If the numid is in the common block, we can simply extract numid
> first, then evaluate the type and value later, eventually call the
> callback at the single place.
> 
> 
> Takashi

My point is that there will only be one DB_SCALE case in the request
handler in control.c in the kernel. It simply calls info2(). The driver
then returns a TLV. The returned TLV will have all the variations on
db_scales. control.c won't care what the actual TLV value means. It will
simply return it to user space, and let alsa-lib work out what it is.
Another call the snd_ctl_misc might not even need a numid. It might
instead be requesting some other information, like card name or some
other future bit of info needed. My point is this is extensible, without
breaking kernel/alsa-lib binary compatibility.

James

Re: dB gain

[Takashi Iwai]

At Mon, 29 May 2006 18:34:06 +0100,
James Courtier-Dutton wrote:
> 
> Takashi Iwai wrote:
> > 
> > I agree to use numid.
> > My argument is that numid can belong to the common header block.
> > 
> > The code you find in control.c is anyway a kind of object oriented.
> > It's a message to a control element.
> > But in this protocol, you get numid at very last, even depending on
> > the type.  Thus the code looks like
> > 
> > 	type = get_type();
> > 	len = get_length();
> > 	switch (type) {
> > 	case DB_SCALE:
> > 		numid = get_value();
> > 		kctl = get_kcontrol(numid);
> > 		kctl->info2(kctl, ptr);
> > 		...
> > 	}
> > 
> > When you add more different types,
> > 
> > 	type = get_type();
> > 	len = get_length();
> > 	switch (type) {
> > 	case DB_SCALE:
> > 		numid = get_value();
> > 		kctl = get_kcontrol(numid);
> > 		kctl->info2(kctl, ptr);
> > 		...
> > 	case ANOTHER_DB_SCALE:
> > 		numid = get_value();
> > 		kctl = get_kcontrol(numid);
> > 		kctl->info2(kctl, ptr);
> > 		...
> > 	case YET_ANOTHER_DATA:
> > 		numid = get_value();
> > 		kctl = get_kcontrol(numid);
> > 		kctl->info2(kctl, ptr);
> > 		...
> > 
> > 	}
> > 
> > If the numid is in the common block, we can simply extract numid
> > first, then evaluate the type and value later, eventually call the
> > callback at the single place.
> > 
> > 
> > Takashi
> 
> My point is that there will only be one DB_SCALE case in the request
> handler in control.c in the kernel. It simply calls info2(). The driver
> then returns a TLV. The returned TLV will have all the variations on
> db_scales. control.c won't care what the actual TLV value means. It will
> simply return it to user space, and let alsa-lib work out what it is.
> Another call the snd_ctl_misc might not even need a numid. It might
> instead be requesting some other information, like card name or some
> other future bit of info needed. My point is this is extensible, without
> breaking kernel/alsa-lib binary compatibility.

Sure, it's extensible.  But my point is that the complexity of this
encapsulation.

For example, in order to get a numid, you have to extract three
levels down.  The version + length, the type + length, again type +
length, then finally you reach to numid value.  Each extraction
requires a function call to convert from a byte-stream to an integer
and a check of the length.  I feel it's a bit too much abstraction.

We've learned that too much abstraction is rather more evil -- see the
implementation of alsa-lib and alsa-kernel API...


Takashi

Re: dB gain

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1292 bytes --]

On Mon, 29 May 2006, Takashi Iwai wrote:

> For example, in order to get a numid, you have to extract three
> levels down.  The version + length, the type + length, again type +
> length, then finally you reach to numid value.  Each extraction
> requires a function call to convert from a byte-stream to an integer
> and a check of the length.  I feel it's a bit too much abstraction.

Note that such complex things reminds me - why we can't transfer all 
"extra" data in one big binary array and let alsa-lib parse them?
Doing selection at the driver level seems too complex to me. TLV 
structure seems good. It might also reduce the driver code, because only 
a pointer to TLV data might be passed to control.c. We can create two 
ioctls:

1) for "global" not element specific data

	u32 data_length
	.... TLV data ....
		u32 tlv_item_length
		u32 tlv_item_type

2) for element specific data

	u32 data_length
	u32 numid
	.... TLV data ....
		u32 tlv_item_length
		u32 tlv_item_type

Also, I would handle all data in the host endian type to save parsing 
time.

					Jaroslav

-----
InterNet přes WIFI bez limitu dat, České Budějovice a okolí
Pokryté lokality: Šindlovy Dvory, Mokré, Litvínovice, Pekárenská, Srubec
Perex @ InterNet <internet@perex.cz>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: dB gain

[James Courtier-Dutton]

So, from what I can see, there have been no real reasons why I should
not commit my db gain code now.
Except for the actual name of the ioctl function. Should I also use a
different number?
Lets take a vote:
1) SNDRV_CTL_IOCTL_MISC = _IOR('U', 0x22, struct snd_ctl_misc),
2) SNDRV_CTL_IOCTL_INFO2 = _IOR('U', 0x22, struct snd_ctl_info2),
3) SNDRV_CTL_IOCTL_TLV = _IOR('U', 0x22, struct snd_ctl_tlv),
4) Something else...suggest.

Once we decide, I will commit it.

James

Re: dB gain

[Jaroslav Kysela]

On Tue, 30 May 2006, James Courtier-Dutton wrote:

> So, from what I can see, there have been no real reasons why I should
> not commit my db gain code now.

I would suggest to reduce the driver code (remove TLV parsing) and create 
two ioctls (one global and one numid based). Or we can even add the global 
ioctl later and parse TLV in driver, when we add something else to TLV.
This means that the ioctl semantics will change from "operate with TLV"
to "gather all possible information to TLV" which is all we need now.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs

Re: dB gain

[Takashi Iwai]

At Tue, 30 May 2006 19:59:58 +0100,
James Courtier-Dutton wrote:
> 
> So, from what I can see, there have been no real reasons why I should
> not commit my db gain code now.

No, please not yet.  The code is still not good enough for commit.

For example, some things to be fixed regarding the code (and coding
style) are below:

+static int uint32_to_message(struct snd_ctl_misc *misc, u32 value)
+{
+	unsigned int pointer = misc->length;
+	u32 *store = (u32*) &misc->message[pointer];

This cast isn't always allowed.

+	*store = value;
+	misc->length += sizeof(u32);
+	return 0;
+}

+static int add_message_tlv_to_container(
+	struct snd_ctl_misc *misc, unsigned int length, unsigned char *message)

Unconventional indentation.

+	unsigned int pointer = misc->length;
+	unsigned int n;
+	for(n=0; n<length; n++) {
+		misc->message[pointer++] = message[n];
+	}

What's wrong with memcpy()?
Anyway, please fix spaces and remove braces.

+static int snd_ctl_misc_user(struct snd_ctl_file *ctl,
+				  struct snd_ctl_misc __user *_misc)
+{
+	struct snd_ctl_misc *misc;
+	struct snd_card *card = ctl->card;
+	struct snd_kcontrol *kctl;
+	struct snd_ctl_elem_info2 info2;
+	int result=0;
+	int numid;
+	unsigned int received_length;
+	unsigned int tlv_type;
+	unsigned int tlv_length;
+	unsigned int tlv_value;
+	unsigned int container_type;
+	unsigned int container_length;
+	
+	misc = kmalloc(2048, GFP_KERNEL);
+	if (misc == NULL)
+		return -ENOMEM;

Why always 2048?

+	/* Only copy _misc: version and length from user-space */
+	if (copy_from_user(misc, _misc, 8)) {
+		result = -EFAULT;
+		goto exit_free;
+	}
+	/* Sanity check */
+	snd_printk("misc->version=%d\n",misc->version);
+	if (misc->version!=1) {
+		result = -EINVAL;
+		goto exit_free;
+	}
+	received_length = misc->length;
+	snd_printk("misc->length=%d\n",misc->length);
+	if (received_length>2040 || received_length<8) {
+		result = -EINVAL;
+		goto exit_free;
+	}

Fix spaces, remove debug prints.

+	misc->length=0;
+	message_to_uint32(misc, &container_type);
+	snd_printk("container_type=%d\n",container_type);
+	switch (container_type) {
+		case SND_MISC_DB_SCALE:
+			message_to_uint32(misc, &container_length);
+			/* FIXME: Do more sanity checks here */
+			message_to_uint32(misc, &tlv_type);
+			snd_printk("tlv_type=%d\n",tlv_type);
+			switch (tlv_type) {
+				case SND_MISC_ELEM_NUMID:
+					message_to_uint32(misc, &tlv_length);
+					snd_printk("tlv_length=%d\n",tlv_length);
+					if (tlv_length != 4) {
+						result = -EINVAL;
+						goto exit_free;
+					}
+					message_to_uint32(misc, &tlv_value);
+					snd_printk("tlv_value=%d\n",tlv_value);
+					break;
+				default:
+					result = -EINVAL;
+					goto exit_free;
+					break;
+			}
+			break;
+		default:
+			result = -EINVAL;
+			goto exit_free;
+	}

Fix spaces, remove debug prints.
Also, cases should be aligned to switch().

But, above all, such a large switch() is ugly.  Better to put the
container-type specific handling out of this function.

+	numid=tlv_value;
+	//snd_printk("numid=%d\n", tlv_value);
+	down_read(&card->controls_rwsem);
+	kctl = snd_ctl_find_numid(card, numid);
+	if (kctl == NULL) {
+		snd_printk("stop 1\n");
+		result = -ENOENT;
+		goto exit_up;
+	}
+	if ( !(kctl->info2) ) { 
+		snd_printk("stop 2\n");
+		result = -EINVAL;
+		goto exit_up;
+	}
+	if (kctl->info2(kctl, 1, &info2)) {
+		snd_printk("stop 3\n");
+		result = -EINVAL;
+		goto exit_up;
+	}
+		;
+	snd_printk("ok 1\n");
+	misc->version=1;
+	misc->length=0;
+	container_open(misc, SND_MISC_DB_SCALE);
+	add_message_tlv_to_container(misc, info2.length, info2.message);
+	container_close(misc);
+	if (copy_to_user(_misc, misc, received_length))
+		result = -EFAULT;
+	snd_printk("ok 2\n");

Although this whole block basically belongs only to DB_SCALE case, the
current code looks as if this block is commonly handled regardless of
the container type.

+struct snd_ctl_misc {
+	u32 version;
+	u32 length;
+	unsigned char message[];
+};

The empty array isn't portable.

+typedef int (snd_kcontrol_info2_t) (struct snd_kcontrol * kcontrol, int type, struct snd_ctl_elem_info2 * uinfo);

The type should be non-numeric but constants (bitwise define or
enum).


Takashi

Re: dB gain

[James Courtier-Dutton]

Takashi Iwai wrote:
> No, please not yet.  The code is still not good enough for commit.
>
> For example, some things to be fixed regarding the code (and coding
> style) are below:
>
> +static int uint32_to_message(struct snd_ctl_misc *misc, u32 value)
> +{
> +	unsigned int pointer = misc->length;
> +	u32 *store = (u32*) &misc->message[pointer];
>
> This cast isn't always allowed.
>
> +	*store = value;
> +	misc->length += sizeof(u32);
> +	return 0;
> +}
>
>   
What do I use instead?

+	misc->message[pointer++] = value & 0xff;
+	misc->message[pointer++] = (value >> 8) & 0xff;
+	misc->message[pointer++] = (value >> 16) & 0xff;
+	misc->message[pointer++] = (value >> 24) & 0xff;


> +static int add_message_tlv_to_container(
> +	struct snd_ctl_misc *misc, unsigned int length, unsigned char *message)
>
> Unconventional indentation.
>
> +	unsigned int pointer = misc->length;
> +	unsigned int n;
> +	for(n=0; n<length; n++) {
> +		misc->message[pointer++] = message[n];
> +	}
>
> What's wrong with memcpy()?
>   
Nothing wrong with memcpy().
> +static int snd_ctl_misc_user(struct snd_ctl_file *ctl,
> +				  struct snd_ctl_misc __user *_misc)
> +{
> +	struct snd_ctl_misc *misc;
> +	struct snd_card *card = ctl->card;
> +	struct snd_kcontrol *kctl;
> +	struct snd_ctl_elem_info2 info2;
> +	int result=0;
> +	int numid;
> +	unsigned int received_length;
> +	unsigned int tlv_type;
> +	unsigned int tlv_length;
> +	unsigned int tlv_value;
> +	unsigned int container_type;
> +	unsigned int container_length;
> +	
> +	misc = kmalloc(2048, GFP_KERNEL);
> +	if (misc == NULL)
> +		return -ENOMEM;
>
> Why always 2048?
>   
No particular reason, just seemed like a ball park value that I should 
not have to change any time soon. I could probably replace it with some 
getuser32 calls to get the required length from the userspace app. Is 
getuser32() the correct call to use instead of copy_from_user(), if all 
one needs is a 4 byte integer from a particular memory location?
> Also, cases should be aligned to switch().
>   
Ok.
> But, above all, such a large switch() is ugly.  Better to put the
> container-type specific handling out of this function.
>
> Although this whole block basically belongs only to DB_SCALE case, the
> current code looks as if this block is commonly handled regardless of
> the container type.
>   
That's true. I will look at putting the case action as a separate function.

> +struct snd_ctl_misc {
> +	u32 version;
> +	u32 length;
> +	unsigned char message[];
> +};
>
> The empty array isn't portable.
>   
Ok, how do I do this then?
The array is not actually empty, but it's size is dependent on the 
length field.

> +typedef int (snd_kcontrol_info2_t) (struct snd_kcontrol * kcontrol, int type, struct snd_ctl_elem_info2 * uinfo);
>
> The type should be non-numeric but constants (bitwise define or
> enum).
>   
I don't understand. This is almost an exact copy of:
typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, 
struct snd_ctl_elem_info * uinfo);
That is already in the code.

Re: dB gain

[Takashi Iwai]

At Wed, 31 May 2006 12:04:39 +0100,
James Courtier-Dutton wrote:
> 
> Takashi Iwai wrote:
> > No, please not yet.  The code is still not good enough for commit.
> >
> > For example, some things to be fixed regarding the code (and coding
> > style) are below:
> >
> > +static int uint32_to_message(struct snd_ctl_misc *misc, u32 value)
> > +{
> > +	unsigned int pointer = misc->length;
> > +	u32 *store = (u32*) &misc->message[pointer];
> >
> > This cast isn't always allowed.
> >
> > +	*store = value;
> > +	misc->length += sizeof(u32);
> > +	return 0;
> > +}
> >
> >   
> What do I use instead?

Use u32 array at the first place so that you need no cast.
If you do char -> u32 pointer conversion, you must guarantee the
alignment, at least.  Otherwise the byte-wise conversion is needed
like you suggested.

> > +static int snd_ctl_misc_user(struct snd_ctl_file *ctl,
> > +				  struct snd_ctl_misc __user *_misc)
> > +{
> > +	struct snd_ctl_misc *misc;
> > +	struct snd_card *card = ctl->card;
> > +	struct snd_kcontrol *kctl;
> > +	struct snd_ctl_elem_info2 info2;
> > +	int result=0;
> > +	int numid;
> > +	unsigned int received_length;
> > +	unsigned int tlv_type;
> > +	unsigned int tlv_length;
> > +	unsigned int tlv_value;
> > +	unsigned int container_type;
> > +	unsigned int container_length;
> > +	
> > +	misc = kmalloc(2048, GFP_KERNEL);
> > +	if (misc == NULL)
> > +		return -ENOMEM;
> >
> > Why always 2048?
> >   
> No particular reason, just seemed like a ball park value that I should 
> not have to change any time soon. I could probably replace it with some 
> getuser32 calls to get the required length from the userspace app. Is 
> getuser32() the correct call to use instead of copy_from_user(), if all 
> one needs is a 4 byte integer from a particular memory location?

(You mean get_user() with an integer argument?)
Yes, we should retrieve the minimal header info (length and type), do
sanity checks, then allocate the value array according to the given
length.

> > Also, cases should be aligned to switch().
> >   
> Ok.
> > But, above all, such a large switch() is ugly.  Better to put the
> > container-type specific handling out of this function.
> >
> > Although this whole block basically belongs only to DB_SCALE case, the
> > current code looks as if this block is commonly handled regardless of
> > the container type.
> >   
> That's true. I will look at putting the case action as a separate function.
> 
> > +struct snd_ctl_misc {
> > +	u32 version;
> > +	u32 length;
> > +	unsigned char message[];
> > +};
> >
> > The empty array isn't portable.
> >   
> Ok, how do I do this then?
> The array is not actually empty, but it's size is dependent on the 
> length field.

Yeah, I know.  And that usage is non-portable.

The callback doesn't have to know about the version.  So, simply
separate arguments like info2(ctl, type, length, value) should
suffice.

> > +typedef int (snd_kcontrol_info2_t) (struct snd_kcontrol * kcontrol, int type, struct snd_ctl_elem_info2 * uinfo);
> >
> > The type should be non-numeric but constants (bitwise define or
> > enum).
> >   
> I don't understand. This is almost an exact copy of:
> typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, 
> struct snd_ctl_elem_info * uinfo);
> That is already in the code.

I meant you pass the value "1" to the function, and the callback again
checks "1".  That's definitely to be avoided.  Define a constant or
use enum instead of 1.


Takashi

Re: dB gain

[Takashi Iwai]

At Mon, 29 May 2006 20:27:50 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Mon, 29 May 2006, Takashi Iwai wrote:
> 
> > For example, in order to get a numid, you have to extract three
> > levels down.  The version + length, the type + length, again type +
> > length, then finally you reach to numid value.  Each extraction
> > requires a function call to convert from a byte-stream to an integer
> > and a check of the length.  I feel it's a bit too much abstraction.
> 
> Note that such complex things reminds me - why we can't transfer all 
> "extra" data in one big binary array and let alsa-lib parse them?
> Doing selection at the driver level seems too complex to me. TLV 
> structure seems good. It might also reduce the driver code, because only 
> a pointer to TLV data might be passed to control.c. We can create two 
> ioctls:
> 
> 1) for "global" not element specific data
> 
> 	u32 data_length
> 	.... TLV data ....
> 		u32 tlv_item_length
> 		u32 tlv_item_type
> 
> 2) for element specific data
> 
> 	u32 data_length
> 	u32 numid
> 	.... TLV data ....
> 		u32 tlv_item_length
> 		u32 tlv_item_type
> 
> Also, I would handle all data in the host endian type to save parsing 
> time.

That sounds reasonable.
And, yes, passing u32 array looks good to me (except for "TLV" order
:)

Alternatively, we can define a special numid (e.g. -1) indicating the
global/unknown element.


Takashi