lkml.org 
[lkml]   [2018]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream control
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.

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:02    [W:0.038 / U:22.984 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site