Re: [PATCH v4] media: vimc: Add vimc-streamer for stream control
Hi Hans, > > 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. Sorry about that. I will be more careful next time with this warnings. Just sent the next version. Regards, Lucas A. M. Magalhães
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: sta...@vger.kernel.org # for v4.20
> Signed-off-by: Lucas A. M. Magalhães
> ---
>
> 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-sensor.c#n204
> It proceeds calling successively vimc_propagate_frame
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-common.c#n210
> Then processes_frame on the next entity
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-scaler.c#n349
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n483
> This goes until the loop ends on a vimc-capture device
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-capture.c#n358
>
> [2]https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-core.c#n80
>
> 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
>
> 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
> -#include
> -#include
> #include
> #include
> #include
> @@ -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(>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(>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(>tpg, 0, 0, vsen->frame);
> + return vsen->frame;
> }
Regards,
Hans
Re: [PATCH v4] media: vimc: Add vimc-streamer for stream control
Hi Lucas,
On 1/21/19 11:05 PM, 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: sta...@vger.kernel.org # for v4.20
> Signed-off-by: Lucas A. M. Magalhães
lgtm, thanks for the patch.
Acked-by: Helen Koike
Regards,
Helen
> ---
>
> 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-sensor.c#n204
> It proceeds calling successively vimc_propagate_frame
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-common.c#n210
> Then processes_frame on the next entity
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-scaler.c#n349
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n483
> This goes until the loop ends on a vimc-capture device
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-capture.c#n358
>
> [2]https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-core.c#n80
>
> 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
>
> diff --git a/drivers/media/platform/vimc/Makefile
> b/drivers/media/platform/vimc/Makefile
> index 4b2e3de7856e..c4fc8e7d365a 100644
> --- a/drivers/media/platform/vimc/Makefile
> +++ b/drivers/media/platform/vimc/Makefile
> @@ -5,6 +5,7 @@ vimc_common-objs := vimc-common.o
> vimc_debayer-objs := vimc-debayer.o
> vimc_scaler-objs := vimc-scaler.o
> vimc_sensor-objs := vimc-sensor.o
> +vimc_streamer-objs := vimc-streamer.o
>
> obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc_capture.o vimc_common.o
> vimc-debayer.o \
> - vimc_scaler.o vimc_sensor.o
> + vimc_scaler.o vimc_sensor.o vimc_streamer.o
> diff --git a/drivers/media/platform/vimc/vimc-capture.c
> b/drivers/media/platform/vimc/vimc-capture.c
> index aaeddf24b042..93837d9eecd2 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -24,6 +24,7 @@
> #include
>
> #include "vimc-common.h"
> +#include "vimc-streamer.h"
>
> #define VIMC_CAP_DRV_NAME "vimc-capture"
>
> @@ -44,7 +45,7 @@ struct vimc_cap_device {
> spinlock_t qlock;
> struct mutex lock;
> u32 sequence;
> - struct media_pipeline pipe;
> + struct vimc_stream stream;
> };
>
> static const struct v4l2_pix_format fmt_default = {
> @@ -248,14 +249,13 @@ static int vimc_cap_start_streaming(struct vb2_queue
> *vq, unsigned int count)
> vcap->sequence = 0;
>
> /* Start the media pipeline */
> - ret = media_pipeline_start(entity, >pipe);
> + ret = media_pipeline_start(entity, >stream.pipe);
> if (ret) {
> vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> return ret;
> }
>
> - /* Enable streaming from the pipe */
> - ret = vimc_pipeline_s_stream(>vdev.entity, 1);
> + ret = vimc_streamer_s_stream(>stream, >ved, 1);
> if (ret) {
> media_pipeline_stop(entity);
> vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> @@ -273,8 +273,7 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
> {
> struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>
> - /* Disable streaming from the pipe */
> -
[PATCH v4] media: vimc: Add vimc-streamer for stream control
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: sta...@vger.kernel.org # for v4.20
Signed-off-by: Lucas A. M. Magalhães
---
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-sensor.c#n204
It proceeds calling successively vimc_propagate_frame
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-common.c#n210
Then processes_frame on the next entity
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-scaler.c#n349
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-debayer.c#n483
This goes until the loop ends on a vimc-capture device
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-capture.c#n358
[2]https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vimc/vimc-core.c#n80
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
diff --git a/drivers/media/platform/vimc/Makefile
b/drivers/media/platform/vimc/Makefile
index 4b2e3de7856e..c4fc8e7d365a 100644
--- a/drivers/media/platform/vimc/Makefile
+++ b/drivers/media/platform/vimc/Makefile
@@ -5,6 +5,7 @@ vimc_common-objs := vimc-common.o
vimc_debayer-objs := vimc-debayer.o
vimc_scaler-objs := vimc-scaler.o
vimc_sensor-objs := vimc-sensor.o
+vimc_streamer-objs := vimc-streamer.o
obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc_capture.o vimc_common.o vimc-debayer.o
\
- vimc_scaler.o vimc_sensor.o
+ vimc_scaler.o vimc_sensor.o vimc_streamer.o
diff --git a/drivers/media/platform/vimc/vimc-capture.c
b/drivers/media/platform/vimc/vimc-capture.c
index aaeddf24b042..93837d9eecd2 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -24,6 +24,7 @@
#include
#include "vimc-common.h"
+#include "vimc-streamer.h"
#define VIMC_CAP_DRV_NAME "vimc-capture"
@@ -44,7 +45,7 @@ struct vimc_cap_device {
spinlock_t qlock;
struct mutex lock;
u32 sequence;
- struct media_pipeline pipe;
+ struct vimc_stream stream;
};
static const struct v4l2_pix_format fmt_default = {
@@ -248,14 +249,13 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq,
unsigned int count)
vcap->sequence = 0;
/* Start the media pipeline */
- ret = media_pipeline_start(entity, >pipe);
+ ret = media_pipeline_start(entity, >stream.pipe);
if (ret) {
vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
return ret;
}
- /* Enable streaming from the pipe */
- ret = vimc_pipeline_s_stream(>vdev.entity, 1);
+ ret = vimc_streamer_s_stream(>stream, >ved, 1);
if (ret) {
media_pipeline_stop(entity);
vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
@@ -273,8 +273,7 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
{
struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
- /* Disable streaming from the pipe */
- vimc_pipeline_s_stream(>vdev.entity, 0);
+ vimc_streamer_s_stream(>stream, >ved, 0);
/* Stop the media pipeline */
media_pipeline_stop(>vdev.entity);
@@ -355,8 +354,8 @@ static void vimc_cap_comp_unbind(struct device *comp,
struct device *master,
kfree(vcap);
}
-static void vimc_cap_process_frame(struct
