[alsa-devel] [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

On 21.09.2018 16:26, Jussi Laako wrote:
> On 21.09.2018 16:06, Takashi Iwai wrote:
>> On Fri, 21 Sep 2018 14:53:20 +0200,
>> Jussi Laako wrote:
>>>
>>> On 21.09.2018 11:43, Takashi Iwai wrote:
>>>> On Fri, 21 Sep 2018 10:35:49 +0200,
>>>> Jussi Laako wrote:
>>>>>
>>>>>> The caching is currently enabled for all elements, but changing it
>>>>>> should be trivial.  The patch below adds is_volatile flag to the
>>>>>> element, and you can set it to true in the quirk somehow for uncached
>>>>>> controls.
>>>>>
>>>>> This patch looks good and what I need! When trying to figure out where
>>>>> it is safe to set the volatile flag I thought that I could set it
>>>>> transparently in add_single_ctl_with_resume() in mixer_quirks.c based
>>>>> on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided
>>>>> snd_kcontrol_new access-member. However, add_single_ctl_with_resume()
>>>>> is allocating just size of usb_mixer_elem_list unlike
>>>>> snd_create_std_mono_ctl_offset() which in turn is allocating full
>>>>> usb_mixer_elem_info size. However, the mixer code seems to be assuming
>>>>> that the item is always usb_mixer_elem_info instead of just list
>>>>> header item. Is this allocation behavior correct and is the item
>>>>> turned into usb_mixer_elem_info somewhere, or is this some kind of
>>>>> bug? So can I safely turn the allocation in
>>>>> add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info
>>>>> instead and set the flag there while keeping correct behavior, or am I
>>>>> missing something?
>>>>
>>>> We can check SNDRV_CTL_ELEM_ACCESS_VOLATILE instead of introducing a
>>>> new flag like my patch, yes.  It's just a matter of easiness.
>>>> Since there is no link from usb_mixer_elem_info to the assigned
>>>> snd_kcontorl, we'd need to add an extra argument in the call path to
>>>> inform about the volatileness, and it may become messy.  We'll find
>>>> the simplest way.
>>>>
>>>> And, IMO, it'd be better to check the cache behavior at setting, since
>>>> the cache may be accessed via multiple paths (at reading each element
>>>> and at resuming the whole system).  But it's no requirement, either.
>>>
>>> I have tried with attached patch, on top of your patch, but I'm still
>>> getting cached values from libasound2. It didn't break anything
>>> either... However, I don't see a link between this cache behavior and
>>> specific user space mixer descriptor. Because polling the mixer
>>> element with snd_mixer_selem_get_capture_volume() keeps always
>>> returning the same value. While in parallel checking the value by
>>> running "amixer" from command line gives updated correct value... So
>>> is there a second layer of cache somewhere in libasound2 to explain
>>> this discrepancy? Because one process sees updated value while other
>>> one doesn't. Difference being that the ALSA descriptors are closed and
>>> reopened every time "amixer" is run...
>>
>> Erm, I should have looked your *actual* patch before replying.
>> Basically all your newly created elements are independent from the
>> other USB standard things, and they are all read-only.  That is, you
>> don't need to use add_single_ctl_with_resume(), but just create each
>> element via snd_ctl_new1() and add it via snd_ctl_add().
>> (Also you can pass the mixer object directly in private_value.)
>>
>> If it still doesn't work as expected, it's rather something wrong in
>> the alsa-lib side, or ACCESS_VOLATILE flag isn't passed / set
>> properly.
> 
> By the way is the add_single_ctl_with_resume() correct in allocating 
> just snd_mixer_elem_list instead of snd_mixer_elem_info? I got 
> suspicious about this when reading the sources. Because the code is 
> similar to snd_create_std_mono_ctl_offset() but they allocate different 
> size which they go on to pass to the same functions. And in quite many 
> places functions cast the snd_mixer_elem_list pointer to 
> snd_mixer_elem_info pointer to access the entire structure which would 
> of course be wrong if it's just snd_mixer_elem_list size item. The patch 
> I posted in my previous mail that changes add_single_ctl_with_resume() 
> to allocate snd_mixer_elem_info instead at least didn't seem to break 
> things...
> 
> OK, I already started going through the alsa-lib code, I suspect 
> something is going wrong there because different processes give 
> different values depending on process lifetime and at the driver level 
> there's no process descriptor context yet...

OK, problem solved! My original patch for the RME mixer quirks is good, 
no changes needed to that one.

Problematic caching happens inside alsa-lib in simple mixer interface, 
where snd_mixer_selem_get_capture_volume() always returns cached value 
for mixer value read and doesn't trigger update from kernel. This is now 
fixed in my user space by using snd_hctl_elem_read() instead which 
triggers update from kernel, and as a side effect also seems to trigger 
update to the cached value so that later call to 
snd_mixer_selem_get_capture_volume() returns updated value too. I'm not 
sure if something should be done about 
snd_mixer_selem_get_capture_volume() behavior or not, but now at least I 
know how to work around this by using different API layer.

In addition, I've applied attached patch on top of your volatile patch, 
but I think that part is not strictly necessary but maybe good thing to 
do. This attached patch fixes (?) the allocation behavior of 
add_single_ctl_with_resume() I was talking about and adds volatile check 
on snd_usb_get_cur_mix_value() entry which should be superfluous to your 
change later in the function.


Thanks gain!

	- Jussi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: volatile-fix-2.patch
Type: text/x-patch
Size: 1357 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20180922/2aaef94a/attachment.bin>