[alsa-devel] [PATCH] alsa-lib/tlv: fix handling of raw value ranges

[Benoît Thébaudeau]

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;
> >  		unsigned int pos, len;
> >  		len = int_index(tlv[1]);
> > +		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
> >  			return -EINVAL;
> >  		pos = 2;
> > +		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