Re: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control
From: Helen Koike
Date: Sat Dec 15 2018 - 13:39:01 EST
On 12/15/18 4:01 PM, Mauro Carvalho Chehab wrote:
> Hi Lucas,
> Em Sat, 15 Dec 2018 14:46:31 -0200
> Lucas A. M. MagalhÃes <lucmaga@xxxxxxxxx> escreveu:
>> The previous code pipeline used the stack to walk on the graph and
>> process a frame. Basically the vimc-sensor entity starts a thread that
>> generates the frames and calls the propagate_process function to send
>> this frame to each entity linked with a sink pad. The propagate_process
>> will call the process_frame of the entities which will call the
>> propagate_frame for each one of it's sink pad. This cycle will continue
>> until it reaches a vimc-capture entity that will finally return and
> I didn't review the code yet, but I have a few comments about the
> way you're describing this patch.
> When you mention about a "previous code pipeline". Well, by adding it
> at the main body of the patch description, reviewers should expect
> that you're mentioning an implementation that already reached upstream.
> I suspect that this is not the case here, as I don't think we merged
> any recursive algorithm using the stack, as this is something that
> we shouldn't do at Kernelspace, as a 4K stack is usually not OK
> with recursive algorithms.
> So, it seems that this entire patch description (as-is) is bogus.
>  if this is not the case and a recursive approach was indeed
> sneaked into the Kernel, this is a bug. So, you should really
> use the "Fixes:" meta-tag indicating what changeset this patch is
> fixing, and a "Cc: stable@xxxxxxxxxxxxxxx", in order to hint
> stable maintainers that this require backports.
Just fyi, this is not the case, the current implementation in mainline
is bogus indeed (implemented by me when I was starting kernel
development, sorry about that and thanks Lucas for sending a fix). Not
only when propagating the frame  but also when activating the
But in any case this should be better written in the commit message.
Every entity calls vimc_propagate_frame()
That calls the process_frame() of each entity directly connected, that
calls vimc_propagate_frame() again:
.s_stream is calling the .s_stream of the subdevices directly connected
I was actually wondering if this is worthy in sending this to stable, as
this implementation is not a real problem, because the topology in vimc
is hardcoded and limited, and according to:
"It must fix a real bug that bothers people"
So as the topology is fixed (in the current implementation), the max
number of nested calls is 4 (in the sensor->debayer->scaler->capture
path), this doesn't triggers any bug to users. But this will be a
problem once we have the configfs API in vimc.
You could say that if your memory is low, this can be a problem in the
current implementation, but then your system won't have memory for any 4
nested function calls anyway (which I think the kernel wouldn't work at
Mauro, with that said, do you still think we should send this to stable?
> Please notice that the patch description will be stored forever
> at the git tree. Mentioning something that were never merged
> (and that, years from now people will hardly remember, and will
> have lots of trouble to seek as you didn't even mentioned any
> ML archive with the past solution) shouldn't be done.
> So, you should rewrite the entire patch description explaining
> what the current approach took by this patch does. Then, in order
> to make easier for reviewers to compare with a previous implementation,
> you can add a "---" line and then a description about why this approach
> is better than the first version, e. g. something like:
> [PATCH v2] media: vimc: Add vimc-streamer for stream control
> Add a logic that will create a linear pipeline walking
> backwards on the graph. When the stream starts it will simply
> loop through the pipeline calling the respective process_frame
> function for each entity on the pipeline.
> Signed-off-by: Your Name <your@email>
> v2: The previous approach were to use a recursive function that
> it was using the stack to walk on the graph and
> process a frame. Basically the vimc-sensor entity starts a thread that
> generates the frames and calls the propagate_process function to send
> this frame to each entity linked with a sink pad. The propagate_process
> will call the process_frame of the entities which will call the
> propagate_frame for each one of it's sink pad. This cycle will continue
> until it reaches a vimc-capture entity that will finally return and
> If the past approach was written by somebody else (or if you sent it
> a long time ago), please add an URL (if possible using
> https://lore.kernel.org/linux-media/ archive) pointing to the previous
> approach, in order to help us to check what you're referring to.