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