Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 15 Dec 2018 16:01:10 -0200
From:   Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To:     "Lucas A. M. =?UTF-8?B?TWFnYWxow6Nlcw==?=" <lucmaga@gmail.com>
Cc:     linux-media@vger.kernel.org, helen.koike@collabora.com,
        hverkuil@xs4all.nl, lkcamp@lists.libreplanetbr.org,
        linux-kernel@vger.kernel.org
Subject: Re: [Lkcamp][PATCH] media: vimc: Add vimc-streamer for stream
 control
Message-ID: <20181215160110.1a353219@coco.lan>
In-Reply-To: <20181215164631.8623-1-lucmaga@gmail.com>
References: <20181215164631.8623-1-lucmaga@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Lucas,


Em Sat, 15 Dec 2018 14:46:31 -0200
Lucas A. M. Magalh=C3=A3es <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=20
	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=20
https://lore.kernel.org/linux-media/ archive) pointing to the previous=20
approach, in order to help us to check what you're referring to.

Regards,
Mauro

Thanks,
Mauro