[alsa-devel] [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Vinod Koul]

On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote:
> On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> > On Sep 5 2016 05:45, Takashi Iwai wrote:
> > >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.
> > 
> > OK. I welcome to abandon this patchset ;)
> > 
> > >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.
> > 
> > +1
> > 
> > >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.
> > 
> > +1
> > 
> > >(Well, it's a philosophical argument: what one would expect for an
> > > element that has neither read nor write access...?)
> > 
> > It's an element with no sense for applications. A waste of codes in kernel
> > land.
> > 
> > >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.
> > 
> > The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit
> > time to switch my mind for this issue.
> 
> Perhaps we should add this as a topic for discussion at the Audio
> mini-conference? If the general feeling is that this feature is
> badly designed we should certainly be looking at what we can do
> to improve it.
> 
> I do very much like the idea of the additional access flag as I
> said. Wrapping the data in a TLV structure we would have to think
> about a little more though as the code has shipped in several
> kernel versions at this point.
> 
> We are starting to have a few customers use a 4.4 kernel which
> does include these controls, all our previous backports had used
> a system of partitioning the controls up into multiple 512 byte
> controls as these binary TLV controls were not supported. So
> there is a little bit of friction to major changes to the ABI,
> but I don't think its insurmountable as long as the functionality
> remains the same.
> 
> But we need to get Intel involved in that discussion too, as they
> have used these controls quite widely as well.

Sorry havent been able to follow this yet :(


But yes current Skylake Chromebooks ship with this code so we cant break it.

I am not sure what is the issue with API though. (sorry haven't read the
thread yet). The tlv was designed to allow people send bytes larger than 512
down to kernel. The Type cna be anything (implementation specific, though we
haven't used it yet), length the blob length and then the bytes blob.
We provide a tunnel and pass these to DSP. They maybe module coefficients,
hotwording blobs etc.

Sure, we can this at u-conf :) F2F is always much better ...

Thanks
-- 
~Vinod