On Sun, 04 Sep 2016 13:07:41 +0200, Takashi Sakamoto wrote:
On Sep 3 2016 20:38, Charles Keepax wrote:
On Fri, Sep 02, 2016 at 08:30:33PM +0900, Takashi Sakamoto wrote:
Sorry to be late. I'm a bit busy for my daily work and things related to my poor life.
On Sep 1 2016 00:40, Takashi Iwai wrote:
Which application do you have in mind?
At first, I did never suggest that 'TLV feature should handle byte stream' or 'TLV feature should be extended to somewhere'. I just addressed that 'Now, TLV feature is not used only for transmission of pure threshold information between applications/drivers, but it's also used for I/O operation. Unfortunately, protocol of TLV packet payload is not kept anymore.'
It's not my intension to discuss about this patchset in a point of 'byte stream'. It comes from your bias, and the other developers are led to the wrong direction, sigh. I really hope them to read my comment in this patchset carefully and compare them to actual code implementations in driver/core/library and applications[1][2][3].
Again, I have few interests about actual implementation of TLV feature in driver side and for what purposes applications are developed to utilize TLV feature. They're free for developers in each area now, and for future. What we should do is how to assist their activity via the design of APIs. This is my place in this patchset. It's not my intension to request extensions of TLV feature.
Applications would access via either alsa-lib or tinyalsa. And these
libraries do already care about how to access via TLV.
Without enough care due to implementation in kernel land.
As I already cleared, current TLV feature has a difficulty not to return the length of actually handled bytes[4]. Correspondingly, APIs in these libraries have defections, as APIs for I/O operation or transmission of arbitrary data, because applications cannot get to know the actual length of handled data.
/* This simulates the other processes to read it. */ packet->length = 100 - sizeof(struct snd_ctl_tlv); if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_READ, packet) < 0) { printf("ioctl(TLV_READ): %s\n", strerror(errno)); goto end_addition; } printf("struct snd_ctl_tlv.length: %d\n", packet->length); printf("But I store %ld bytes.\n", 6 * sizeof(unsigned int));Here you wrote 6 items into the control but the read said it gave you 100 items and you would have preferred it to have said 6 items? But you have created a 128 item long ALSA control if we forget about the TLV aspect (which in my view is just a means to access the control) and assume this was a regular ALSA control then you would never expect that to give you less data than the size of the control, so this behaviour would be expected.
You misunderstand the design of ALSA control core. I wrote a part of it, later.
As this was originally intended (at least AFAIK) as a mechanism to provide byte controls >512 bytes, my point is really just that I would expect this to work like a normal control. There is no need to pass the length actually processed because this isn't for arbitrary length data its just a way to read/write a control which is something that already has a defined size.
Just for current implementation, it's appropriate.
But for future, it may be certainly inappropriate.
There's no way for user space to get appropriate length in advance, this brings contradiction for content of given buffer. For example, in ALSA SoC part, the length is trimmed according to a parameter of driver instance, implicitly[5]. As a result, users are beguiled. They requests a certain length of buffer to be handled via TLV feature. Even if they receive success, a _part_ of buffer is actually handled. This is not good for software stacks in a point of abstraction layer of hardware. There's no transparency independent from hardwares. Application developers is forced to consider about device driver's codes which are not open via APIs.
That length you refer to is not some secret internal length it is the length of the control as returned by the info ioctl. I guess we could make the read/write fail if it was larger than the control, that might be better behaviour than just silently not handling all the data.
It's an abuse in a point of application interfaces.
'struct snd_ctl_elem_info.count' represents the number of members which can hold a value, in a element of a element set. It's not for something about TLV information.
And some core codes and user lands are written according to the design. Please investigate 'sound/core/control.c' in Linux kernel and 'src/control/*' in alsa-lib and 'alsactl/alsaloop/amixer' in alsa-utils. Then, we may find to lost something important in kernel land development.
The latest alsa-lib includes a document of the design of ALSA control core. It may help your understanding. http://www.alsa-project.org/alsa-doc/alsa-lib/control.html
Yeah, there are obviously some issues in the current implementation of wm_adsp and ASoC ext ctl. Although I'll unlikely take Sakamoto-san's patchset as is from a few reasons, these issues still should be addressed.
First off, passing the binary blob directly via TLV callback is incorrect from the ABI perspective. When Vinod proposed the idea via TLV access originally, we thought they the data is encoded in TLV format. Alas, the resulted code didn't do that and it slipped into the upstream without consideration.
Besides that, the second problem is the count value returned via snd_ctl_elem_info, as mentioned in the above. It's beyond the original control API design, and a kind of illegal usage. (Well, it's a philosophical argument: what one would expect for an element that has neither read nor write access...?)
So, at this point, the main question is whether we keep this access pattern as is, as a sort of official one, and put some exceptional rule. Charles, how is the situation? Has it been already deployed to real systems?
If we may still change the wm_adsp behavior, we may "fix" the first issue by passing the blob properly encoded in TLV format, at least. OTOH, if we need to keep the current ABI abuse as is, one idea is to add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of control, and we define more strictly how the code should be implemented. Currently we can judge this element as a one that has no read/write access but with tlv r/w. But it's way too unclear.
thanks,
Takashi