Re: [PATCH v4] media: vimc: Add vimc-streamer for stream control

Hi Lucas,

On 1/22/19 2:05 AM, Lucas A. M. Magalhaes wrote:

Add a linear pipeline logic for the stream control. It's created by walking backwards on the entity graph. When the stream starts it will simply loop through the pipeline calling the respective process_frame function of each entity.

Fixes: f2fe89061d797 ("vimc: Virtual Media Controller core, capture and sensor") Cc: stable@vger.kernel.org # for v4.20 Signed-off-by: Lucas A. M. Magalhães lucmaga@gmail.com

---

The actual approach for streaming frames on vimc uses a recursive logic[1]. This algorithm may cause problems as the stack usage increases a with the topology. For the actual topology almost 1Kb of stack is used if compiled with KASAN on a 64bit architecture. However the topology is fixed and hard-coded on vimc-core[2]. So it's a controlled situation if used as is.

[1] The stream starts on vim-sensor's thread https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc... It proceeds calling successively vimc_propagate_frame https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc... Then processes_frame on the next entity https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc... https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc... This goes until the loop ends on a vimc-capture device https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc...

[2]https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc...

Thanks the review Helen. I've made the change you pointed out. There was really a bug for single entity pipelines.

Changed from v3:

• Fix vimc_streamer_pipeline_init and vimc_streamer_pipeline_terminate for a

single entity pipeline.

• Remove unnecessary checks and comments.
• Alignment and style.

drivers/media/platform/vimc/Makefile | 3 +- drivers/media/platform/vimc/vimc-capture.c | 18 +- drivers/media/platform/vimc/vimc-common.c | 35 ---- drivers/media/platform/vimc/vimc-common.h | 15 +- drivers/media/platform/vimc/vimc-debayer.c | 26 +-- drivers/media/platform/vimc/vimc-scaler.c | 28 +-- drivers/media/platform/vimc/vimc-sensor.c | 56 ++---- drivers/media/platform/vimc/vimc-streamer.c | 188 ++++++++++++++++++++ drivers/media/platform/vimc/vimc-streamer.h | 38 ++++ 9 files changed, 260 insertions(+), 147 deletions(-) create mode 100644 drivers/media/platform/vimc/vimc-streamer.c create mode 100644 drivers/media/platform/vimc/vimc-streamer.h

<snip>

diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c index 32ca9c6172b1..93961a1e694f 100644 --- a/drivers/media/platform/vimc/vimc-sensor.c +++ b/drivers/media/platform/vimc/vimc-sensor.c @@ -16,8 +16,6 @@ */ #include <linux/component.h> -#include <linux/freezer.h> -#include <linux/kthread.h> #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/platform_device.h> @@ -201,38 +199,27 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = { .set_fmt = vimc_sen_set_fmt, }; -static int vimc_sen_tpg_thread(void *data) +static void *vimc_sen_process_frame(struct vimc_ent_device *ved,

• 		    const void *sink_frame)
  

{

• struct vimc_sen_device *vsen = data;
• unsigned int i;
• set_freezable();
• set_current_state(TASK_UNINTERRUPTIBLE);
• for (;;) {

• try_to_freeze();
  

• if (kthread_should_stop())
  

• 	break;
  

• tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
  

• struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,

• 				    ved);
  

• const struct vimc_pix_map *vpix;
• unsigned int frame_size;

• /* Send the frame to all source pads */
  

• for (i = 0; i < vsen->sd.entity.num_pads; i++)
  

• 	vimc_propagate_frame(&vsen->sd.entity.pads[i],
  

• 			     vsen->frame);
  

• /* Calculate the frame size */
• vpix = vimc_pix_map_by_code(vsen->mbus_format.code);
• frame_size = vsen->mbus_format.width * vpix->bpp *

•      vsen->mbus_format.height;
  

frame_size is set, but not used:

vimc-sensor.c:208:15: warning: variable 'frame_size' set but not used [-Wunused-but-set-variable]

Can you make a patch fixing this?

I'm not sure how I missed this sparse warning, weird.

• /* 60 frames per second */
  

• schedule_timeout(HZ/60);
  

• }
• return 0;

• tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
• return vsen->frame;

}

Regards,

Hans