[alsa-devel] [PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[Takashi Iwai]

On Wed, 24 May 2017 16:19:06 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On May 24 2017 15:29, Takashi Iwai wrote:
> > OK, I now understand your idea.  Actually the word "proxy" was
> > misleading.  "proxy" implies the action, but PCM core can't know the
> > action the caller intended.  From PCM core POV, it's merely an
> > in-kernel buffer copy.  We don't need to invent new terms here (who is
> > the client?).
> 
> Currently, I have no idea for the better term.
>
> Data transmission is a kind of communication, thus there's a
> originator. I use the 'client' as the originator. Usually, userspace
> applications are the client, however in addressed issue in-kernel
> OSS/UAC1 drivers are the client. They perform as a proxy for their
> direct clients.

A good way is to just look through the kernel code and see what other
people use for the similar aim.


> > Apart from that, basically your idea is:
> > - Keep in-kernel buffer copy flag into substream->runtime.
> > - The ops handles both copy and silence.
> > - Move the whole transfer action to each driver instead of looping in
> >    PCM core.
> >
> > I find the first point is acceptable, from the current usage pattern,
> > as there is little chance to mix up both user-space buffer and
> > kernel-space buffer.
> 
> Practically, it's rarely for clients to request copy operation for
> source or destination on several memory spaces.
> 
> > Though, another side of coin is that, by embedding in runtime, you can
> > easily overlook the in-kernel copy case, in comparison with passing
> > via the argument.  So it's not always plus.
> 
> However, in current place, drivers with kernel/kernel copying is quite
> rare. At least, they're not major ones. Features for them should be
> selectable by configurations. In this point, such argument-oriented
> implementation is exaggerated for the features. It's not worth to
> change existent kernel APIs.

The additional kconfig is also both merit and demerit.  More kconfigs
are more mess, in general, and we're trying really hard not to add a
new kconfig if it doesn't give a very clear benefit.  And, this case
is in a gray zone.

Also, the rarity in the code isn't always a good judgment point,
either.  Even though it's used only in few places of the code, it's
the code most of distros enable.  So practically seen, it's almost
always enabled.


> > The second point is as same as mine.
> >
> > The potential problem is the third point.  It'd require a larger
> > change, and it's error-prone, because you'll have to translate the
> > passed pointer in each driver to morph it to different values, either
> > the linear buffer pointer or the vector, in addition to the user-space
> > and kernel-space check.
> >
> > In the PCM core code, we did that in that way, but it's OK since it's
> > done only there thus it's manageable.  However, forcing the complex
> > cast in each driver implementation is better to avoid.
> >
> > How to make each driver implementing less error-prone codes -- that's
> > what we need to reconsider further.
> 
> I have no objection for your view. It's natural idea for software
> developers.
> 
> Essentially, it's not better idea to produce one callback function for
> two types of buffer access. Type casting is enough dangerous and make
> it hard to trace arguments, but current PCM core implementation
> heavily utilizes it. I can assume to add two callback operations to
> driver programming API; for interleaved buffer and non-interleaved
> buffer.

Well, the original copy/silence ops aren't bad in that perspective;
their purposes are simple, just copy or fill silence on the given
position from the given pointer, no matter which mode it is.  So I
think we should keep this semantic simplicity.

I really appreciate your patches, but I pushed back just by a simple
reason: I already experimented that approach in the past, thus I know
it doesn't fly.  By moving the complexity from PCM core to the driver
ops (e.g. looping over channels inside callback) makes the PCM code
simpler, but at the same time, it makes the callback more complex than
now.  Keeping the driver side implementation simpler is much more
benefit overall.

And, taking a look back, I believe that merging both copy and silence
operations into a single ops was a bad idea, too.  Although it helped
reducing the code -- which is a good sign in one side -- it turned out
to make the code flow in the callback more complicated.  You'll have
to do ugly NULL-check in each place.

Then, now we're dealing with another conditional for in-kernel copy...
It makes the callback even more complicated.

So, for now, I'm inclined to go back and just to add a new ops for
in-kernel copying.  Then the __user pointer cast isn't needed, and the
purpose of each ops is clear.  The drawback is, of course, a slightly
more code than the merged ops, but it's the cost for clarity.


But one thing I'm experimenting now, while we're at it, is to change
the arguments passed to copy/silence ops.  So far, they take frames.
It's good in most cases, but for copy/silence, it's often rather a
burden than a benefit.  Instead, passing bytes fit better in many
callback implementations, and above all, it makes the meaning of
position and size consistent in both interleaved and non-interleaved
modes.  This allowed me to unify the transfer codes in PCM core.

I'm going to submit a demo-patchset shortly later.


thanks,

Takashi