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

[Jussi Laako]

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...


Thanks,

	- Jussi