lkml.org 
[lkml]   [2018]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control
From
Date
Hi Mauro,

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@gmail.com> 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
>> unstack.
>
> 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[1].
>
> [1] 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@vger.kernel.org", 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 [1] but also when activating the
pipeline [2].

But in any case this should be better written in the commit message.


[1]
Every entity calls vimc_propagate_frame()
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n506
That calls the process_frame() of each entity directly connected, that
calls vimc_propagate_frame() again:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-common.c#n237

[2]
.s_stream is calling the .s_stream of the subdevices directly connected
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n355


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:
https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst
"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
all).

Mauro, with that said, do you still think we should send this to stable?

Thanks
Helen

>
> 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
> unstack.
> ...
>
> 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.
>
> Regards,
> Mauro
>
> Thanks,
> Mauro
>

\
 
 \ /
  Last update: 2018-12-15 19:40    [W:0.040 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site