Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai <tiwai@suse.de>]

On Mon, 12 Sep 2016 17:25:31 +0200,
Vinod Koul wrote:
> 
> 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.

Yes, and this part is missing in wm_adsp driver.  It passes the blob
without TLV encoding, i.e. starting from the offset zero without type
and length encoding.

> We provide a tunnel and pass these to DSP. They maybe module coefficients,
> hotwording blobs etc.

So, does Intel driver pass the blob in TLV format?  Then we have two
different implementations.

Also, still another point is to be decided: is passing an arbitrary
size via info callback for an element without read/write access bits
(but with TLV bit) a right behavior?

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

Well, the u-conf is still over a month ahead.  I think we should keep
discussing on ML before u-conf.


thanks,

Takashi