[alsa-devel] [PATCH] alsa-lib/tlv: fix handling of raw value ranges
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
E.g.: static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1), 48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0), }; SOC_SINGLE_TLV("Main Volume", BD3753X_VOL_GAIN, 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255 instead of the expected 142.
or: static const unsigned int max9850_tlv[] = { TLV_DB_RANGE_HEAD(4), 0x18, 0x1f, TLV_DB_SCALE_ITEM(-7450, 400, 0), 0x20, 0x33, TLV_DB_SCALE_ITEM(-4150, 200, 0), 0x34, 0x37, TLV_DB_SCALE_ITEM(-150, 100, 0), 0x38, 0x3f, TLV_DB_SCALE_ITEM(250, 50, 0), }; SOC_SINGLE_TLV("Headphone Volume", MAX9850_VOLUME, 0, 0x3f, 1, max9850_tlv),
snd_tlv_convert_from_dB(tlv, 0, 0x3f, -7450, &value, 0) returned 0 instead of the expected 0x18.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: { - long dbmin, dbmax, prev_rangemax; + long dbmin, dbmax, prev_submax; unsigned int pos, len; len = int_index(tlv[1]); - if (len > MAX_TLV_RANGE_SIZE) - return -EINVAL; - if (snd_tlv_get_dB_range(tlv, rangemin, rangemax, - &dbmin, &dbmax)) + if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL; - if (db_gain <= dbmin) { - *value = rangemin; - return 0; - } else if (db_gain >= dbmax) { - *value = rangemax; - return 0; - } pos = 2; - prev_rangemax = 0; - while (pos + 4 <= len) { - rangemin = (int)tlv[pos]; - rangemax = (int)tlv[pos + 1]; + prev_submax = 0; + do { + long submin, submax; + submin = (int)tlv[pos]; + submax = (int)tlv[pos + 1]; + if (rangemax < submax) + submax = rangemax; if (!snd_tlv_get_dB_range(tlv + pos + 2, - rangemin, rangemax, + submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2, - rangemin, rangemax, + submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) { - *value = xdir ? rangemin : prev_rangemax; + *value = xdir || pos == 2 ? submin : prev_submax; return 0; } - prev_rangemax = rangemax; + prev_submax = submax; + if (rangemax == submax) + break; pos += int_index(tlv[pos + 3]) + 4; - } - return -EINVAL; + } while (pos + 4 <= len); + *value = prev_submax; + return 0; } case SND_CTL_TLVT_DB_SCALE: { int min, step, max;
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
I guess the problem you actually have is that convert_to_dB does not work with these raw values?
static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1), 48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0), }; SOC_SINGLE_TLV("Main Volume", BD3753X_VOL_GAIN, 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255 instead of the expected 142.
But there is no guarantee that this mixer control will have only one of those values in the TLV's range. Furthermore, convert_to_dB will still fail for any raw value 1..47.
This TLV is just incomplete. Please report this bug to the driver's author.
As for the patch, while I'm not sure whether it is needed at all, I also don't quite understand the reason for all the changes.
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_submax;if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;prev_submax = 0;do {
Why is this not a while loop?
long submin, submax;submin = (int)tlv[pos];submax = (int)tlv[pos + 1];if (rangemax < submax)submax = rangemax;
Why is this check needed? Any why not return -EINVAL?
if (!snd_tlv_get_dB_range(tlv + pos + 2,
submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2,submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) {*value = xdir || pos == 2 ? submin : prev_submax; return 0; }prev_submax = submax;if (rangemax == submax)break;
Why this check?
pos += int_index(tlv[pos + 3]) + 4;
} while (pos + 4 <= len);*value = prev_submax;return 0;
Regards, Clemens
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
then the kernel driver will use the existing SOC_SINGLE_TLV macro, which, like most TLV macros, automatically sets min to 0. The driver then relies on the associated TLV range description to filter out invalid raw values.
This is not a driver/kernel issue. The TLV already filters out invalid raw values between range items, like 0x41 to 0x60 in the example above, so there is no reason why it would not also be the case for raw values before the 1st dB range and after the last one.
If you wanted to make the global dB range match the global raw range in the driver, almost all kernel TLV macros would have to have another version taking a min, which would not really be useful as explained above.
I guess the problem you actually have is that convert_to_dB does not work with these raw values?
No. These raw values are not supposed to be used. They are invalid for the hardware. The issue is that convert_from_dB may return raw values outside of all valid dB ranges.
static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1), 48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0), }; SOC_SINGLE_TLV("Main Volume", BD3753X_VOL_GAIN, 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255 instead of the expected 142.
But there is no guarantee that this mixer control will have only one of those values in the TLV's range.
What do you mean? convert_from_dB already relied on the fact that TLV ranges are sorted in ascending dB order.
Furthermore, convert_to_dB will still fail for any raw value 1..47.
This TLV is just incomplete. Please report this bug to the driver's author.
It is complete. See my explanations with the other example given above. There is no reason for invalid raw values to be able to exist between the dB ranges, and not before the 1st dB range or after the last one.
As for the patch, while I'm not sure whether it is needed at all, I also don't quite understand the reason for all the changes.
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_submax;if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;prev_submax = 0;do {Why is this not a while loop?
Because the added "if (len < 6" makes it pointless.
long submin, submax;submin = (int)tlv[pos];submax = (int)tlv[pos + 1];if (rangemax < submax)submax = rangemax;Why is this check needed? Any why not return -EINVAL?
Same as for snd_tlv_get_dB_range: to handle the case where the driver narrows down for a given control the range of values reported by the TLV.
The ranges accepted must be the intersection of the TLV ranges and of the global range of raw values for each control. All this issue can be summed up this way.
if (!snd_tlv_get_dB_range(tlv + pos + 2,
submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2,submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) {*value = xdir || pos == 2 ? submin : prev_submax; return 0; }prev_submax = submax;if (rangemax == submax)break;Why this check?
Same as for snd_tlv_get_dB_range: because for the case "if (rangemax < submax)" above, you have reached the final range item you may be interested in, so it's pointless to handle the following ones.
pos += int_index(tlv[pos + 3]) + 4;
} while (pos + 4 <= len);*value = prev_submax;return 0;Regards, Clemens
Regards, Benoît
At Fri, 30 Mar 2012 12:41:44 +0200 (CEST), Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
You can't prohibit the values that are exposed as accessible. The driver may return an error -EINVAL or such, but it doesn't mean that it's allowed to give a hole in the TLV dB definition.
Takashi
At Fri, 30 Mar 2012 13:44:43 +0200, Takashi Iwai wrote:
At Fri, 30 Mar 2012 12:41:44 +0200 (CEST), Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
You can't prohibit the values that are exposed as accessible. The driver may return an error -EINVAL or such, but it doesn't mean that it's allowed to give a hole in the TLV dB definition.
But, don't get me wrong with the statement above. Your fix can be an improvement indeed. The fact that the function relies on range_min/max blindly isn't described anywhere, and it'd be better to parse the range properly, of course, especially it's an exported function, not only used as an internal function.
There may be minor coding issues as Clemens suggested, but they can be easily fixed.
So, the motivation here is to make the code more robust and consistent although the examples you raised seem like rather buggy driver implementations.
Takashi
Takashi Iwai wrote:
At Fri, 30 Mar 2012 13:44:43 +0200, Takashi Iwai wrote:
At Fri, 30 Mar 2012 12:41:44 +0200 (CEST), Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
You can't prohibit the values that are exposed as accessible. The driver may return an error -EINVAL or such, but it doesn't mean that it's allowed to give a hole in the TLV dB definition.
But, don't get me wrong with the statement above. Your fix can be an improvement indeed. The fact that the function relies on range_min/max blindly isn't described anywhere, and it'd be better to parse the range properly, of course, especially it's an exported function, not only used as an internal function.
There may be minor coding issues as Clemens suggested, but they can be easily fixed.
So, the motivation here is to make the code more robust and consistent
Yes, that's what this patch aims at.
although the examples you raised seem like rather buggy driver implementations.
Why? And you say that there shouldn't be holes in TLV ranges. I don't see why either. I have not read anywhere that the ALSA APIs forbid this, and by the way it works. Moreover, the example above with some prohibited ranges would be a hardware example, not a driver example. It should be possible to handle all hardware cases.
Regards, Benoît
At Fri, 30 Mar 2012 15:07:27 +0200 (CEST), Benoît Thébaudeau wrote:
Takashi Iwai wrote:
At Fri, 30 Mar 2012 13:44:43 +0200, Takashi Iwai wrote:
At Fri, 30 Mar 2012 12:41:44 +0200 (CEST), Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
You can't prohibit the values that are exposed as accessible. The driver may return an error -EINVAL or such, but it doesn't mean that it's allowed to give a hole in the TLV dB definition.
But, don't get me wrong with the statement above. Your fix can be an improvement indeed. The fact that the function relies on range_min/max blindly isn't described anywhere, and it'd be better to parse the range properly, of course, especially it's an exported function, not only used as an internal function.
There may be minor coding issues as Clemens suggested, but they can be easily fixed.
So, the motivation here is to make the code more robust and consistent
Yes, that's what this patch aims at.
although the examples you raised seem like rather buggy driver implementations.
Why? And you say that there shouldn't be holes in TLV ranges. I don't see why either.
Because holes have been never defined to be allowed. Rather, in the API level, all values are supposed to be accessible.
It's possible that a driver temporarily returns an error when accessing some value. But, it's supposed to be an temporay error. Some drivers may abuse this to implement a hole.
I have not read anywhere that the ALSA APIs forbid this,
"Not forbidden" doesn't mean it's always allowed and recommended :)
and by the way it works. Moreover, the example above with some prohibited ranges would be a hardware example, not a driver example. It should be possible to handle all hardware cases.
The ALSA control values don't have to be exactly as same as the hardware raw values at all. Rather they should be converted to be human-friendly form. If there is a hole, it should be omitted and converted in the driver side to be continous.
Takashi
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
then the kernel driver will use the existing SOC_SINGLE_TLV macro, which, like most TLV macros, automatically sets min to 0. The driver then relies on the associated TLV range description to filter out invalid raw values.
All raw values described by the .info callback, returned by the .get callback, and especially accepted by the .put callback, are valid.
The TLV information just _describes_ values, it cannot _restrict_ them.
If a register has invalid values, the driver must somehow map between raw control values and valid register values.
static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1), 48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0), }; SOC_SINGLE_TLV(... 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255 instead of the expected 142.
But there is no guarantee that this mixer control will have only one of those values in the TLV's range.
What do you mean?
It _is_ possible to have raw values 1..47 and 143..255 (and convert_to_dB will fail for those).
As for the patch, while I'm not sure whether it is needed at all, I also don't quite understand the reason for all the changes.
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_submax;if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;prev_submax = 0;do {Why is this not a while loop?
Because the added "if (len < 6" makes it pointless.
Then why did you add the len<6 check? This is, in effect, just the same as the old while condition.
Regards, Clemens
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
then the kernel driver will use the existing SOC_SINGLE_TLV macro, which, like most TLV macros, automatically sets min to 0. The driver then relies on the associated TLV range description to filter out invalid raw values.
All raw values described by the .info callback, returned by the .get callback, and especially accepted by the .put callback, are valid.
The TLV information just _describes_ values, it cannot _restrict_ them.
If a register has invalid values, the driver must somehow map between raw control values and valid register values.
Now I understand how you see things. But is it an official and absolute ALSA rule, known by everybody, that may cause issues if not applied?
Here is my point of view. Let's take a real example, which corresponds to the volume table of the ROHM BD37534 on page 26: http://www.rohm.com/products/databook/audio/pdf/bd37531fv-e.pdf
This is what gave the 1st example of my patch comment: static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1), 48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0), }; SOC_SINGLE_TLV("Main Volume", BD3753X_VOL_GAIN, 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
This solution is the only possible one using existing ALSA SOC TLV macros. This indeed breaks the rule that you gave above, but is it really an issue? See below.
The hardware values are: 0x00 to 0x70: prohibited 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps 0xd0 to 0xfe: prohibited 0xff: mute
To respect your rule, the driver would have to implement its own .info/.get/.put callbacks, giving the following mapping: SW 0x00: HW 0xff: mute SW 0x01 to 0x5f: HW 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps
But why compel the driver to complicate things this way if it can work with standard macros and holes in the TLV ranges?
Do you think that it is not a so common case, and hence that it does not deserve to be handled by the general macros?
It is also the case for the TLV320AIC3104, page 0/register 19/field D6-D3 on page 51: http://www.ti.com/lit/ds/symlink/tlv320aic3104.pdf
It is also probably the case for many other existing hardware.
Do you think that the solution would be rather to add common kernel ALSA stuff to handle such mappings?
As for the patch, while I'm not sure whether it is needed at all, I also don't quite understand the reason for all the changes.
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_submax;if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;prev_submax = 0;do {Why is this not a while loop?
Because the added "if (len < 6" makes it pointless.
Then why did you add the len<6 check? This is, in effect, just the same as the old while condition.
Because it's useless to wait until the end of the loop to return -EINVAL, and with my patch, the after-loop code becomes the fallback case for beyond-final- range values, so it avoids adding a weird extra check there.
Regards, Benoît
Benoît Thébaudeau wrote:
To respect your rule, the driver would have to implement its own .info/.get/.put callbacks, giving the following mapping: SW 0x00: HW 0xff: mute SW 0x01 to 0x5f: HW 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps
It would rather be: SW 0x00: HW 0xff: mute SW 0x01 to 0x5f: HW 0xcf to 0x71: -79 dB to +15 dB by +1 dB steps
Regards, Benoît
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
All raw values described by the .info callback, returned by the .get callback, and especially accepted by the .put callback, are valid.
The TLV information just _describes_ values, it cannot _restrict_ them.
If a register has invalid values, the driver must somehow map between raw control values and valid register values.
Now I understand how you see things. But is it an official and absolute ALSA rule,
Yes.
known by everybody,
No.
that may cause issues if not applied?
Yes.
The hardware values are: 0x00 to 0x70: prohibited 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps 0xd0 to 0xfe: prohibited 0xff: mute
To respect your rule, the driver would have to implement its own .info/.get/.put callbacks, giving the following mapping: SW 0x00: HW 0xff: mute SW 0x01 to 0x5f: HW 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps
But why compel the driver to complicate things this way if it can work with standard macros and holes in the TLV ranges?
Because it _cannot_ work with standard macros and holes.
It is also probably the case for many other existing hardware.
Do you think that the solution would be rather to add common kernel ALSA stuff to handle such mappings?
Yes.
I'll see if I can cobble together some TLV checking code ...
As for the patch, while I'm not sure whether it is needed at all, I also don't quite understand the reason for all the changes.
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_submax;if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;prev_submax = 0;do {Why is this not a while loop?
Because the added "if (len < 6" makes it pointless.
Then why did you add the len<6 check? This is, in effect, just the same as the old while condition.
Because it's useless to wait until the end of the loop to return -EINVAL,
It is not necessary to optimize for error cases.
and with my patch, the after-loop code becomes the fallback case for beyond- final-range values, so it avoids adding a weird extra check there.
But why does this force you to use do instead of while?
Regards, Clemens
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
Because it's useless to wait until the end of the loop to return -EINVAL,
It is not necessary to optimize for error cases.
It's not an optimization, but a simplification. Otherwise, pos would have to be tested again after the loop to differentiate the loop-skipped case from the at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.
and with my patch, the after-loop code becomes the fallback case for beyond- final-range values, so it avoids adding a weird extra check there.
But why does this force you to use do instead of while?
It doesn't. It's just that it's useless to keep a while here as the 1st iteration will necessarily occur thanks to the previous if.
Regards, Benoît
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
In all the hardware examples that I've found, the worst case for holes is always, like for the BD37534: mute hole valid dB range hole
So it's perhaps not worth developing something very complicated in the kernel to handle all possible cases of TLV range holes. I think that most real-life cases will be handled if we consider a min-max raw HW range combined with a mute value automatically set at the extreme value of the bit-field, with an optional inversion. Something like:
#define SOC_SINGLE_MUTABLE_TLV(xname, reg, shift, min, max, invert, tlv_array)
In the BD driver example that I gave: static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1), 48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0), }; SOC_SINGLE_TLV("Main Volume", BD3753X_VOL_GAIN, 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
would become: static const DECLARE_TLV_DB_SCALE(bd3753x_vol_fader_gain_att_tlv, -8000, 100, 1); SOC_SINGLE_MUTABLE_TLV("Main Volume", BD3753X_VOL_GAIN, 0, 0x71, 0xcf, 1, bd3753x_vol_fader_gain_att_tlv),
The range exposed by .info would be 0 to 0x5f, 0 being the mute HW value 0xff, and 0x01 to 0x5f matching the HW 0xcf to 0x71.
The SOC_*_MUTABLE_TLV set of macros would use dedicated .info/.get/.put callbacks. They could theoretically be merged into the snd_soc_*_volsw callbacks if a new field were added to struct soc_mixer_control to indicate this use case. Or the invert field could be replaced with a set of option flags.
Regards, Benoît
At Fri, 30 Mar 2012 17:20:21 +0200 (CEST), Benoît Thébaudeau wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
In all the hardware examples that I've found, the worst case for holes is always, like for the BD37534: mute hole valid dB range hole
So it's perhaps not worth developing something very complicated in the kernel to handle all possible cases of TLV range holes.
Well, it's not only about the TLV dB information. The user-space has no idea where holes exist, and we have no way to expose this information, so far. That is, you can access to some random raw values and now you'll get always errors. This is obviously a bug of the driver.
So, please don't take it as an example to _fix_ the alsa-lib code. Making it working for such a buggy driver isn't called as a fix. It's called as a workaround. And, a workaround is usually ugly.
However, beyond that, there is a good advantage in your change; it can make the code more robust. We should concentrate on that point.
Takashi
Takashi Iwai wrote:
Benoît Thébaudeau wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
In all the hardware examples that I've found, the worst case for holes is always, like for the BD37534: mute hole valid dB range hole
So it's perhaps not worth developing something very complicated in the kernel to handle all possible cases of TLV range holes.
Well, it's not only about the TLV dB information. The user-space has no idea where holes exist, and we have no way to expose this information, so far. That is, you can access to some random raw values and now you'll get always errors. This is obviously a bug of the driver.
So, please don't take it as an example to _fix_ the alsa-lib code. Making it working for such a buggy driver isn't called as a fix. It's called as a workaround. And, a workaround is usually ugly.
However, beyond that, there is a good advantage in your change; it can make the code more robust. We should concentrate on that point.
Yes. This is not what I meant here. Clemens seemed to be about to propose a generic solution in the kernel to handle hardware volume range holes so as to remove them when exposed to the user space. What I meant in my previous message was that his solution would probably be sufficient if it only handled one hole between the mute value and the other values, and another hole after the valid values. That would ease the development of drivers for these cases, and thus avoid buggy drivers in the first place.
Regards, Benoît
At Fri, 30 Mar 2012 15:58:51 +0200 (CEST), Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
Because it's useless to wait until the end of the loop to return -EINVAL,
It is not necessary to optimize for error cases.
It's not an optimization, but a simplification. Otherwise, pos would have to be tested again after the loop to differentiate the loop-skipped case from the at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.
Unless it's really a hot path, such a micro-optimization doesn't give much value.
Rather better to keep the standard style that makes people easier to read the code. A do-while loop with a large block is usually less readable than a normal while loop.
Takashi
Takashi Iwai wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
Because it's useless to wait until the end of the loop to return -EINVAL,
It is not necessary to optimize for error cases.
It's not an optimization, but a simplification. Otherwise, pos would have to be tested again after the loop to differentiate the loop-skipped case from the at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.
Unless it's really a hot path, such a micro-optimization doesn't give much value.
Rather better to keep the standard style that makes people easier to read the code. A do-while loop with a large block is usually less readable than a normal while loop.
Then, the patch becomes:
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: { - long dbmin, dbmax, prev_rangemax; + long dbmin, dbmax, prev_submax; unsigned int pos, len; len = int_index(tlv[1]); - if (len > MAX_TLV_RANGE_SIZE) - return -EINVAL; - if (snd_tlv_get_dB_range(tlv, rangemin, rangemax, - &dbmin, &dbmax)) + if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL; - if (db_gain <= dbmin) { - *value = rangemin; - return 0; - } else if (db_gain >= dbmax) { - *value = rangemax; - return 0; - } pos = 2; - prev_rangemax = 0; + prev_submax = 0; while (pos + 4 <= len) { - rangemin = (int)tlv[pos]; - rangemax = (int)tlv[pos + 1]; + long submin, submax; + submin = (int)tlv[pos]; + submax = (int)tlv[pos + 1]; + if (rangemax < submax) + submax = rangemax; if (!snd_tlv_get_dB_range(tlv + pos + 2, - rangemin, rangemax, + submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2, - rangemin, rangemax, + submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) { - *value = xdir ? rangemin : prev_rangemax; + *value = xdir || pos == 2 ? submin : prev_submax; return 0; } - prev_rangemax = rangemax; + prev_submax = submax; + if (rangemax == submax) + break; pos += int_index(tlv[pos + 3]) + 4; } - return -EINVAL; + *value = prev_submax; + return 0; } case SND_CTL_TLVT_DB_SCALE: { int min, step, max;
Hi Takashi, Clemens, all,
Takashi Iwai wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
Because it's useless to wait until the end of the loop to return -EINVAL,
It is not necessary to optimize for error cases.
It's not an optimization, but a simplification. Otherwise, pos would have to be tested again after the loop to differentiate the loop-skipped case from the at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.
Unless it's really a hot path, such a micro-optimization doesn't give much value.
Rather better to keep the standard style that makes people easier to read the code. A do-while loop with a large block is usually less readable than a normal while loop.
Would you agree with the following revision?
[PATCH] alsa-lib/tlv: improve robustness of raw value ranges
snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is exported, it is better for robustness and consistency to parse the range properly, which this patch does.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: { - long dbmin, dbmax, prev_rangemax; + long dbmin, dbmax, prev_submax; unsigned int pos, len; len = int_index(tlv[1]); - if (len > MAX_TLV_RANGE_SIZE) - return -EINVAL; - if (snd_tlv_get_dB_range(tlv, rangemin, rangemax, - &dbmin, &dbmax)) + if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL; - if (db_gain <= dbmin) { - *value = rangemin; - return 0; - } else if (db_gain >= dbmax) { - *value = rangemax; - return 0; - } pos = 2; - prev_rangemax = 0; + prev_submax = 0; while (pos + 4 <= len) { - rangemin = (int)tlv[pos]; - rangemax = (int)tlv[pos + 1]; + long submin, submax; + submin = (int)tlv[pos]; + submax = (int)tlv[pos + 1]; + if (rangemax < submax) + submax = rangemax; if (!snd_tlv_get_dB_range(tlv + pos + 2, - rangemin, rangemax, + submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2, - rangemin, rangemax, + submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) { - *value = xdir ? rangemin : prev_rangemax; + *value = xdir || pos == 2 ? submin : prev_submax; return 0; } - prev_rangemax = rangemax; + prev_submax = submax; + if (rangemax == submax) + break; pos += int_index(tlv[pos + 3]) + 4; } - return -EINVAL; + *value = prev_submax; + return 0; } case SND_CTL_TLVT_DB_SCALE: { int min, step, max;
At Mon, 21 May 2012 21:38:22 +0200 (CEST), Benoît Thébaudeau wrote:
Hi Takashi, Clemens, all,
Takashi Iwai wrote:
Benoît Thébaudeau wrote:
Clemens Ladisch wrote:
I'll see if I can cobble together some TLV checking code ...
Great.
Because it's useless to wait until the end of the loop to return -EINVAL,
It is not necessary to optimize for error cases.
It's not an optimization, but a simplification. Otherwise, pos would have to be tested again after the loop to differentiate the loop-skipped case from the at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.
Unless it's really a hot path, such a micro-optimization doesn't give much value.
Rather better to keep the standard style that makes people easier to read the code. A do-while loop with a large block is usually less readable than a normal while loop.
Would you agree with the following revision?
Yes, the patch looks good to me. But I couldn't apply it because the mail is encoded in quoted-printable. Could you fix it, or if it's difficult, give an attachment?
thanks,
Takashi
[PATCH] alsa-lib/tlv: improve robustness of raw value ranges
snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is exported, it is better for robustness and consistency to parse the range properly, which this patch does.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_rangemax;
long dbmin, dbmax, prev_submax; unsigned int pos, len; len = int_index(tlv[1]);
if (len > MAX_TLV_RANGE_SIZE)return -EINVAL;if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,&dbmin, &dbmax))
if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;
if (db_gain <= dbmin) {*value = rangemin;return 0;} else if (db_gain >= dbmax) {*value = rangemax;return 0;} pos = 2;prev_rangemax = 0;
prev_submax = 0; while (pos + 4 <= len) {
rangemin = (int)tlv[pos];rangemax = (int)tlv[pos + 1];
long submin, submax;submin = (int)tlv[pos];submax = (int)tlv[pos + 1];if (rangemax < submax)submax = rangemax; if (!snd_tlv_get_dB_range(tlv + pos + 2,
rangemin, rangemax,
submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2,
rangemin, rangemax,
submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) {
*value = xdir ? rangemin : prev_rangemax;
*value = xdir || pos == 2 ? submin : prev_submax; return 0; }
prev_rangemax = rangemax;
prev_submax = submax;if (rangemax == submax)break; pos += int_index(tlv[pos + 3]) + 4; }
return -EINVAL;
*value = prev_submax;return 0; } case SND_CTL_TLVT_DB_SCALE: { int min, step, max;
Takashi Iwai wrote:
Yes, the patch looks good to me. But I couldn't apply it because the mail is encoded in quoted-printable. Could you fix it, or if it's difficult, give an attachment?
Sorry about that. I can't avoid that with my current mailer, so I attach the patch. Tell me if it's OK for you like this.
Regards, Benoît
At Tue, 22 May 2012 04:19:12 +0200 (CEST), Benoît Thébaudeau wrote:
Takashi Iwai wrote:
Yes, the patch looks good to me. But I couldn't apply it because the mail is encoded in quoted-printable. Could you fix it, or if it's difficult, give an attachment?
Sorry about that. I can't avoid that with my current mailer, so I attach the patch. Tell me if it's OK for you like this.
It's OK to put an attachment, but I failed to apply it. Please check the patch you sent can be really applied cleanly to alsa-lib git tree.
thanks,
Takashi
Regards, Benoît [2 improve-tlv-raw-val-robustness.patch <text/x-patch (base64)>] alsa-lib/tlv: improve robustness of raw value ranges
snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is exported, it is better for robustness and consistency to parse the range properly, which this patch does.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_rangemax;
long dbmin, dbmax, prev_submax; unsigned int pos, len; len = int_index(tlv[1]);
if (len > MAX_TLV_RANGE_SIZE)return -EINVAL;if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,&dbmin, &dbmax))
if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;
if (db_gain <= dbmin) {*value = rangemin;return 0;} else if (db_gain >= dbmax) {*value = rangemax;return 0;} pos = 2;prev_rangemax = 0;
prev_submax = 0; while (pos + 4 <= len) {
rangemin = (int)tlv[pos];rangemax = (int)tlv[pos + 1];
long submin, submax;submin = (int)tlv[pos];submax = (int)tlv[pos + 1];if (rangemax < submax)submax = rangemax; if (!snd_tlv_get_dB_range(tlv + pos + 2,
rangemin, rangemax,
submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2,
rangemin, rangemax,
submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) {
*value = xdir ? rangemin : prev_rangemax;
*value = xdir || pos == 2 ? submin : prev_submax; return 0; }
prev_rangemax = rangemax;
prev_submax = submax;if (rangemax == submax)break; pos += int_index(tlv[pos + 3]) + 4; }
return -EINVAL;
*value = prev_submax;return 0; } case SND_CTL_TLVT_DB_SCALE: { int min, step, max;
Hi Takashi,
Takashi Iwai wrote:
It's OK to put an attachment, but I failed to apply it. Please check the patch you sent can be really applied cleanly to alsa-lib git tree.
My copy/paste damaged tabs. Hopefully, this should now work.
Regards, Benoît
At Tue, 22 May 2012 18:06:34 +0200 (CEST), Benoît Thébaudeau wrote:
Hi Takashi,
Takashi Iwai wrote:
It's OK to put an attachment, but I failed to apply it. Please check the patch you sent can be really applied cleanly to alsa-lib git tree.
My copy/paste damaged tabs. Hopefully, this should now work.
Thanks, applied now.
Takashi
Regards, Benoît [2 improve-tlv-raw-val-robustness.patch <text/x-patch (base64)>] alsa-lib/tlv: improve robustness of raw value ranges
snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is exported, it is better for robustness and consistency to parse the range properly, which this patch does.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com
--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {
long dbmin, dbmax, prev_rangemax;
long dbmin, dbmax, prev_submax;
if (len > MAX_TLV_RANGE_SIZE)
if (len < 6 || len > MAX_TLV_RANGE_SIZE) return -EINVAL;
if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,&dbmin, &dbmax))return -EINVAL;if (db_gain <= dbmin) {*value = rangemin;return 0;} else if (db_gain >= dbmax) {*value = rangemax;return 0;}prev_rangemax = 0;
prev_submax = 0;
rangemin = (int)tlv[pos];rangemax = (int)tlv[pos + 1];
long submin, submax;submin = (int)tlv[pos];submax = (int)tlv[pos + 1];if (rangemax < submax)submax = rangemax; if (!snd_tlv_get_dB_range(tlv + pos + 2,
rangemin, rangemax,
submin, submax, &dbmin, &dbmax) && db_gain >= dbmin && db_gain <= dbmax) return snd_tlv_convert_from_dB(tlv + pos + 2,
rangemin, rangemax,
submin, submax, db_gain, value, xdir); else if (db_gain < dbmin) {
*value = xdir ? rangemin : prev_rangemax;
*value = xdir || pos == 2 ? submin : prev_submax; return 0; }
prev_rangemax = rangemax;
prev_submax = submax;if (rangemax == submax)break; pos += int_index(tlv[pos + 3]) + 4;
return -EINVAL;
*value = prev_submax;return 0;
On 30 March 2012 11:41, Benoît Thébaudeau benoit.thebaudeau@advansee.com wrote:
Clemens Ladisch wrote:
Benoît Thébaudeau wrote:
In case of a TLV dB range with all items having raw value ranges strictly within the main raw value range reported by the driver, snd_tlv_convert_from_dB() returned one of the main raw range boundaries, which was outside all dB range items.
But the main raw range boundaries _are_ valid values.
Not always. For instance, if a codec register has the following volume values: 0x00 to 0x20: prohibited 0x21 to 0x40: some dB values 0x41 to 0x60: prohibited 0x61 to 0xff: some dB values
In the userspace to kernel interface. A mixer control must have min/max values. When writing the value, all values within the range of min to max must be valid values. There are just too many user land tools that rely on this fact. Think of the use case or "Next step louder please". If the currently written value is 0x40, the current user land mixer apps would try to write 0x41, without even bothering to look at the dB tables. If you instead change the driver to map: 0x21 to 0x40 -> 0x00 to 0x1f 0x61 to 0xff -> 0x20 to 0xBE and set min to 0, and max to 0xbe The user space tools will then work fine.
The dB conversion tables are just a friendly way of displaying a slightly more useful figure to the user than a percentage. If you have hardware with gaps like the above, the kernel driver will have to be fixed to remove the prohibited ranges in the kernel to userspace API. This sort of conversion is done in many other alsa kernel drivers.
Kind Regards
James
Hi James,
On 2 June 2012 10:09, James Courtier-Dutton james.dutton@gmail.com wrote:
In the userspace to kernel interface. A mixer control must have min/max values. When writing the value, all values within the range of min to max must be valid values. There are just too many user land tools that rely on this fact. Think of the use case or "Next step louder please". If the currently written value is 0x40, the current user land mixer apps would try to write 0x41, without even bothering to look at the dB tables. If you instead change the driver to map: 0x21 to 0x40 -> 0x00 to 0x1f 0x61 to 0xff -> 0x20 to 0xBE and set min to 0, and max to 0xbe The user space tools will then work fine.
The dB conversion tables are just a friendly way of displaying a slightly more useful figure to the user than a percentage. If you have hardware with gaps like the above, the kernel driver will have to be fixed to remove the prohibited ranges in the kernel to userspace API. This sort of conversion is done in many other alsa kernel drivers.
Thanks for the details. I'll do that.
Regards, Benoît
participants (4)
-
Benoît Thébaudeau -
Clemens Ladisch -
James Courtier-Dutton -
Takashi Iwai