| Message ID | 20201015195746.264722-1-helen.koike@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: cap: refactor enable/disable stream to allow multistreaming | expand |
Hi, Am 15.10.20 um 21:57 schrieb Helen Koike: > Allow streaming from self picture path and main picture path at the same > time. > > Take care for s_stream() callbacks to not be called twice. > When starting a stream, s_stream(true) shouldn't be called for the isp > and the sensor if the other stream is already enabled (since it was > already called). > When stopping a stream, s_stream(false) shouldn't be called for isp and > the sensor if the other stream is still enabled. > > Remove the callback function scheme for navigating through the topology, > simplifying the code, improving readability, while calling > media_pipeline_{start,stop}() in the right order. > > Remove multistreaming item from the TODO list. > > Signed-off-by: Helen Koike <helen.koike@collabora.com> > > --- > Hello, > > Since we didn't reach an agreement on the helpers in the core[1], I'm > sending this patch to fix this limitation only for rkisp1. > > [1] https://patchwork.linuxtv.org/project/linux-media/cover/20200415013044.1778572-1-helen.koike@collabora.com/ > > If we decide to add the helpers in the future, we can clean up drivers > even more, but I don't want to block this feature. > > Overview of the patch: > ====================== > > * Rename rkisp1_stream_{start,stop}() to > rkisp1_cap_stream_{enable,disable}() to clarify the difference between > other stream enable/disable functions > > * Implement rkisp1_pipeline_stream_{enable,disable}() to replace > rkisp1_pipeline_{enable,disable}_cb() and rkisp1_pipeline_sink_walk(), > which were removed. > > * Call rkisp1_cap_stream_{enable,disable}() from > rkisp1_pipeline_stream_{enable,disable}() for better > unwind handling and function name semantics. > > * Call media_pipeline_{start,stop}() in the right order. > > * Remove item from TODO list (I also reviewed the use of the > is_streaming var in the code and lgtm). > > This patch was tested on rockpi4 board with: > ============================================ > > "media-ctl" "-d" "platform:rkisp1" "-r" > "media-ctl" "-d" "platform:rkisp1" "-l" "'imx219 4-0010':0 -> 'rkisp1_isp':0 [1]" > "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_selfpath':0 [1]" > "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]" > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"imx219 4-0010":0 [fmt:SRGGB10_1X10/1640x1232]' > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":0 [fmt:SRGGB10_1X10/1640x1232 crop: (0,0)/1600x1200]' > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":2 [fmt:YUYV8_2X8/1600x1200 crop: (0,0)/1500x1100]' > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":1 [fmt:YUYV8_2X8/900x800]' > > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "width=900,height=800," > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "pixelformat=422P" > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]' > > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "width=900,height=800," > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "pixelformat=422P" > > sleep 1 > > time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "--stream-mmap" "--stream-count" "100" & > time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "--stream-mmap" "--stream-count" "100" & > > wait > echo "Completed" > > Thanks > Helen > --- > drivers/staging/media/rkisp1/TODO | 3 - > drivers/staging/media/rkisp1/rkisp1-capture.c | 227 +++++++++--------- > 2 files changed, 113 insertions(+), 117 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > index e7c8398fc2cef..a2dd0ad951c25 100644 > --- a/drivers/staging/media/rkisp1/TODO > +++ b/drivers/staging/media/rkisp1/TODO > @@ -1,9 +1,6 @@ > * Fix pad format size for statistics and parameters entities. > * Fix checkpatch errors. > * Add uapi docs. Remember to add documentation of how quantization is handled. > -* streaming paths (mainpath and selfpath) check if the other path is streaming > -in several places of the code, review this, specially that it doesn't seem it > -supports streaming from both paths at the same time. > > NOTES: > * All v4l2-compliance test must pass. > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index b6f497ce3e95c..254936873c6c1 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -830,71 +830,43 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap, > } > > /* > - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb > - * @from: entity at which to start pipeline walk > - * @until: entity at which to stop pipeline walk > - * > - * Walk the entities chain starting at the pipeline video node and stop > - * all subdevices in the chain. > - * > - * If the until argument isn't NULL, stop the pipeline walk when reaching the > - * until entity. This is used to disable a partially started pipeline due to a > - * subdev start error. > + * Most of registers inside rockchip ISP1 have shadow register since > + * they must be not be changed during processing a frame. > + * Usually, each sub-module updates its shadow register after > + * processing the last pixel of a frame. > */ > -static int rkisp1_pipeline_sink_walk(struct media_entity *from, > - struct media_entity *until, > - int (*cb)(struct media_entity *from, > - struct media_entity *curr)) > +static void rkisp1_cap_stream_enable(struct rkisp1_capture *cap) > { > - struct media_entity *entity = from; > - struct media_pad *pad; > - unsigned int i; > - int ret; > - > - while (1) { > - pad = NULL; > - /* Find remote source pad */ > - for (i = 0; i < entity->num_pads; i++) { > - struct media_pad *spad = &entity->pads[i]; > - > - if (!(spad->flags & MEDIA_PAD_FL_SINK)) > - continue; > - pad = media_entity_remote_pad(spad); > - if (pad && is_media_entity_v4l2_subdev(pad->entity)) > - break; > - } > - if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) > - break; > + struct rkisp1_device *rkisp1 = cap->rkisp1; > + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; > > - entity = pad->entity; > - if (entity == until) > - break; > + cap->ops->set_data_path(cap); > + cap->ops->config(cap); > > - ret = cb(from, entity); > - if (ret) > - return ret; > + /* Setup a buffer for the next frame */ > + spin_lock_irq(&cap->buf.lock); > + rkisp1_set_next_buf(cap); > + cap->ops->enable(cap); > + /* It's safe to config ACTIVE and SHADOW regs for the > + * first stream. While when the second is starting, do NOT > + * force update because it also update the first one. > + * > + * The latter case would drop one more buf(that is 2) since > + * there's not buf in shadow when the second FE received. This's > + * also required because the second FE maybe corrupt especially > + * when run at 120fps. > + */ > + if (!other->is_streaming) { > + /* force cfg update */ > + rkisp1_write(rkisp1, > + RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); > + rkisp1_set_next_buf(cap); > } > - > - return 0; > -} > - > -static int rkisp1_pipeline_disable_cb(struct media_entity *from, > - struct media_entity *curr) > -{ > - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); > - > - return v4l2_subdev_call(sd, video, s_stream, false); > -} > - > -static int rkisp1_pipeline_enable_cb(struct media_entity *from, > - struct media_entity *curr) > -{ > - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); > - > - return v4l2_subdev_call(sd, video, s_stream, true); > + spin_unlock_irq(&cap->buf.lock); > + cap->is_streaming = true; > } > > -static void rkisp1_stream_stop(struct rkisp1_capture *cap) > +static void rkisp1_cap_stream_disable(struct rkisp1_capture *cap) > { > int ret; > > @@ -911,6 +883,81 @@ static void rkisp1_stream_stop(struct rkisp1_capture *cap) > } > } > > +/* > + * rkisp1_pipeline_stream_disable - disable nodes in the pipeline > + * > + * Call s_stream(false) in the reverse order from > + * rkisp1_pipeline_stream_enable() and disable the DMA engine. > + * Should be called before media_pipeline_stop() > + */ > +static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) > + __must_hold(&cap->rkisp1->stream_lock) > +{ > + struct rkisp1_device *rkisp1 = cap->rkisp1; > + > + rkisp1_cap_stream_disable(cap); > + > + /* > + * If the other capture is streaming, isp and sensor nodes shouldn't > + * be disabled, skip them. > + */ > + if (rkisp1->pipe.streaming_count < 2) { > + v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, > + false); > + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); > + } > + > + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, > + false); I wonder if the order of the disable matters. If it should be from the sensor up to the capture then we should call 'rkisp1_cap_stream_disable' at the end of this function. > +} > + > +/* > + * rkisp1_pipeline_stream_enable - enable nodes in the pipeline > + * > + * Enable the DMA Engine and call s_stream(true) through the pipeline. > + * Should be called after media_pipeline_start() > + */ > +static int rkisp1_pipeline_stream_enable(struct rkisp1_capture *cap) > + __must_hold(&cap->rkisp1->stream_lock) > +{ > + struct rkisp1_device *rkisp1 = cap->rkisp1; > + int ret; > + > + rkisp1_cap_stream_enable(cap); > + > + ret = v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, > + s_stream, true); > + if (ret) > + return ret; In case of error here, shouldn't we call 'rkisp1_cap_stream_disable'? > + > + /* > + * If the other capture is streaming, isp and sensor nodes are already > + * enabled, skip them. > + */ > + if (rkisp1->pipe.streaming_count > 1) > + return 0; > + > + ret = v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, true); > + if (ret) > + goto err_disable_rsz; > + > + ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, > + true); > + if (ret) > + goto err_disable_isp; > + > + return 0; > + > +err_disable_isp: > + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); > +err_disable_rsz: > + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, > + false); > + rkisp1_cap_stream_disable(cap); > + > + return ret; > +} > + > static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) > { > struct rkisp1_capture *cap = queue->drv_priv; > @@ -920,13 +967,8 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) > > mutex_lock(&cap->rkisp1->stream_lock); > > - rkisp1_stream_stop(cap); > + rkisp1_pipeline_stream_disable(cap); > media_pipeline_stop(&node->vdev.entity); > - ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL, > - rkisp1_pipeline_disable_cb); > - if (ret) > - dev_err(rkisp1->dev, > - "pipeline stream-off failed error:%d\n", ret); > > rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR); > > @@ -940,43 +982,6 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) > mutex_unlock(&cap->rkisp1->stream_lock); > } > > -/* > - * Most of registers inside rockchip ISP1 have shadow register since > - * they must be not be changed during processing a frame. > - * Usually, each sub-module updates its shadow register after > - * processing the last pixel of a frame. > - */ > -static void rkisp1_stream_start(struct rkisp1_capture *cap) > -{ > - struct rkisp1_device *rkisp1 = cap->rkisp1; > - struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; > - > - cap->ops->set_data_path(cap); > - cap->ops->config(cap); > - > - /* Setup a buffer for the next frame */ > - spin_lock_irq(&cap->buf.lock); > - rkisp1_set_next_buf(cap); > - cap->ops->enable(cap); > - /* It's safe to config ACTIVE and SHADOW regs for the > - * first stream. While when the second is starting, do NOT > - * force update because it also update the first one. > - * > - * The latter case would drop one more buf(that is 2) since > - * there's not buf in shadow when the second FE received. This's > - * also required because the second FE maybe corrupt especially > - * when run at 120fps. > - */ > - if (!other->is_streaming) { > - /* force cfg update */ > - rkisp1_write(rkisp1, > - RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); > - rkisp1_set_next_buf(cap); > - } > - spin_unlock_irq(&cap->buf.lock); > - cap->is_streaming = true; > -} > - > static int > rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) > { > @@ -1001,28 +1006,22 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) > goto err_pipe_pm_put; > } > The previous code with the 'rkisp1_pipeline_sink_walk' only followed enabled links. We should probably first check if the link between the resizer and the isp is enabled and also between the isp to the sensor. and return -EPIPE if not. Actually I think that the code that set the 'active_sensor' (callingrkisp1_get_remote_sensor) should move to here since this also check if the link to the sensor is enabled. Thanks, Dafna > - rkisp1_stream_start(cap); > - > - /* start sub-devices */ > - ret = rkisp1_pipeline_sink_walk(entity, NULL, > - rkisp1_pipeline_enable_cb); > - if (ret) > - goto err_stop_stream; > - > ret = media_pipeline_start(entity, &cap->rkisp1->pipe); > if (ret) { > dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret); > - goto err_pipe_disable; > + goto err_v4l2_pm_put; > } > + ret = rkisp1_pipeline_stream_enable(cap); > + if (ret) > + goto err_media_pipeline_stop; > > mutex_unlock(&cap->rkisp1->stream_lock); > > return 0; > > -err_pipe_disable: > - rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb); > -err_stop_stream: > - rkisp1_stream_stop(cap); > +err_media_pipeline_stop: > + media_pipeline_stop(entity); > +err_v4l2_pm_put: > v4l2_pipeline_pm_put(entity); > err_pipe_pm_put: > pm_runtime_put(cap->rkisp1->dev); >
On Fri, Oct 16, 2020 at 4:28 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > Hi, > > Am 15.10.20 um 21:57 schrieb Helen Koike: > > Allow streaming from self picture path and main picture path at the same > > time. > > > > Take care for s_stream() callbacks to not be called twice. > > When starting a stream, s_stream(true) shouldn't be called for the isp > > and the sensor if the other stream is already enabled (since it was > > already called). > > When stopping a stream, s_stream(false) shouldn't be called for isp and > > the sensor if the other stream is still enabled. > > > > Remove the callback function scheme for navigating through the topology, > > simplifying the code, improving readability, while calling > > media_pipeline_{start,stop}() in the right order. > > > > Remove multistreaming item from the TODO list. > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com> > > > > --- > > Hello, > > > > Since we didn't reach an agreement on the helpers in the core[1], I'm > > sending this patch to fix this limitation only for rkisp1. > > > > [1] https://patchwork.linuxtv.org/project/linux-media/cover/20200415013044.1778572-1-helen.koike@collabora.com/ > > > > If we decide to add the helpers in the future, we can clean up drivers > > even more, but I don't want to block this feature. > > > > Overview of the patch: > > ====================== > > > > * Rename rkisp1_stream_{start,stop}() to > > rkisp1_cap_stream_{enable,disable}() to clarify the difference between > > other stream enable/disable functions > > > > * Implement rkisp1_pipeline_stream_{enable,disable}() to replace > > rkisp1_pipeline_{enable,disable}_cb() and rkisp1_pipeline_sink_walk(), > > which were removed. > > > > * Call rkisp1_cap_stream_{enable,disable}() from > > rkisp1_pipeline_stream_{enable,disable}() for better > > unwind handling and function name semantics. > > > > * Call media_pipeline_{start,stop}() in the right order. > > > > * Remove item from TODO list (I also reviewed the use of the > > is_streaming var in the code and lgtm). > > > > This patch was tested on rockpi4 board with: > > ============================================ > > > > "media-ctl" "-d" "platform:rkisp1" "-r" > > "media-ctl" "-d" "platform:rkisp1" "-l" "'imx219 4-0010':0 -> 'rkisp1_isp':0 [1]" > > "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_selfpath':0 [1]" > > "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]" > > > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"imx219 4-0010":0 [fmt:SRGGB10_1X10/1640x1232]' > > > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":0 [fmt:SRGGB10_1X10/1640x1232 crop: (0,0)/1600x1200]' > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":2 [fmt:YUYV8_2X8/1600x1200 crop: (0,0)/1500x1100]' > > > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":1 [fmt:YUYV8_2X8/900x800]' > > > > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "width=900,height=800," > > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "pixelformat=422P" > > > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' > > "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]' > > > > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "width=900,height=800," > > "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "pixelformat=422P" > > > > sleep 1 > > > > time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "--stream-mmap" "--stream-count" "100" & > > time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "--stream-mmap" "--stream-count" "100" & > > > > wait > > echo "Completed" > > > > Thanks > > Helen > > --- > > drivers/staging/media/rkisp1/TODO | 3 - > > drivers/staging/media/rkisp1/rkisp1-capture.c | 227 +++++++++--------- > > 2 files changed, 113 insertions(+), 117 deletions(-) > > > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > > index e7c8398fc2cef..a2dd0ad951c25 100644 > > --- a/drivers/staging/media/rkisp1/TODO > > +++ b/drivers/staging/media/rkisp1/TODO > > @@ -1,9 +1,6 @@ > > * Fix pad format size for statistics and parameters entities. > > * Fix checkpatch errors. > > * Add uapi docs. Remember to add documentation of how quantization is handled. > > -* streaming paths (mainpath and selfpath) check if the other path is streaming > > -in several places of the code, review this, specially that it doesn't seem it > > -supports streaming from both paths at the same time. > > > > NOTES: > > * All v4l2-compliance test must pass. > > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > > index b6f497ce3e95c..254936873c6c1 100644 > > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > > @@ -830,71 +830,43 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap, > > } > > > > /* > > - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb > > - * @from: entity at which to start pipeline walk > > - * @until: entity at which to stop pipeline walk > > - * > > - * Walk the entities chain starting at the pipeline video node and stop > > - * all subdevices in the chain. > > - * > > - * If the until argument isn't NULL, stop the pipeline walk when reaching the > > - * until entity. This is used to disable a partially started pipeline due to a > > - * subdev start error. > > + * Most of registers inside rockchip ISP1 have shadow register since > > + * they must be not be changed during processing a frame. > > + * Usually, each sub-module updates its shadow register after > > + * processing the last pixel of a frame. > > */ > > -static int rkisp1_pipeline_sink_walk(struct media_entity *from, > > - struct media_entity *until, > > - int (*cb)(struct media_entity *from, > > - struct media_entity *curr)) > > +static void rkisp1_cap_stream_enable(struct rkisp1_capture *cap) > > { > > - struct media_entity *entity = from; > > - struct media_pad *pad; > > - unsigned int i; > > - int ret; > > - > > - while (1) { > > - pad = NULL; > > - /* Find remote source pad */ > > - for (i = 0; i < entity->num_pads; i++) { > > - struct media_pad *spad = &entity->pads[i]; > > - > > - if (!(spad->flags & MEDIA_PAD_FL_SINK)) > > - continue; > > - pad = media_entity_remote_pad(spad); > > - if (pad && is_media_entity_v4l2_subdev(pad->entity)) > > - break; > > - } > > - if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) > > - break; > > + struct rkisp1_device *rkisp1 = cap->rkisp1; > > + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; > > > > - entity = pad->entity; > > - if (entity == until) > > - break; > > + cap->ops->set_data_path(cap); > > + cap->ops->config(cap); > > > > - ret = cb(from, entity); > > - if (ret) > > - return ret; > > + /* Setup a buffer for the next frame */ > > + spin_lock_irq(&cap->buf.lock); > > + rkisp1_set_next_buf(cap); > > + cap->ops->enable(cap); > > + /* It's safe to config ACTIVE and SHADOW regs for the > > + * first stream. While when the second is starting, do NOT > > + * force update because it also update the first one. > > + * > > + * The latter case would drop one more buf(that is 2) since > > + * there's not buf in shadow when the second FE received. This's > > + * also required because the second FE maybe corrupt especially > > + * when run at 120fps. > > + */ > > + if (!other->is_streaming) { > > + /* force cfg update */ > > + rkisp1_write(rkisp1, > > + RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); > > + rkisp1_set_next_buf(cap); > > } > > - > > - return 0; > > -} > > - > > -static int rkisp1_pipeline_disable_cb(struct media_entity *from, > > - struct media_entity *curr) > > -{ > > - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); > > - > > - return v4l2_subdev_call(sd, video, s_stream, false); > > -} > > - > > -static int rkisp1_pipeline_enable_cb(struct media_entity *from, > > - struct media_entity *curr) > > -{ > > - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); > > - > > - return v4l2_subdev_call(sd, video, s_stream, true); > > + spin_unlock_irq(&cap->buf.lock); > > + cap->is_streaming = true; > > } > > > > -static void rkisp1_stream_stop(struct rkisp1_capture *cap) > > +static void rkisp1_cap_stream_disable(struct rkisp1_capture *cap) > > { > > int ret; > > > > @@ -911,6 +883,81 @@ static void rkisp1_stream_stop(struct rkisp1_capture *cap) > > } > > } > > > > +/* > > + * rkisp1_pipeline_stream_disable - disable nodes in the pipeline > > + * > > + * Call s_stream(false) in the reverse order from > > + * rkisp1_pipeline_stream_enable() and disable the DMA engine. > > + * Should be called before media_pipeline_stop() > > + */ > > +static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) > > + __must_hold(&cap->rkisp1->stream_lock) > > +{ > > + struct rkisp1_device *rkisp1 = cap->rkisp1; > > + > > + rkisp1_cap_stream_disable(cap); > > + > > + /* > > + * If the other capture is streaming, isp and sensor nodes shouldn't > > + * be disabled, skip them. > > + */ > > + if (rkisp1->pipe.streaming_count < 2) { > > + v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, > > + false); > > + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); > > + } > > + > > + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, > > + false); > > I wonder if the order of the disable matters. If it should be from the sensor up to the > capture then we should call 'rkisp1_cap_stream_disable' at the end of this > function. According to our downstream code [1] and my recollection of discussion with Rockchip, the power up order was: - ISP - sensor - PHY and power down the opposite. That said, the sensor drivers we had didn't implement s_power and used runtime PM at streaming time, so I think in practice the order ended up as: - ISP, - PHY, - sensor. The streaming enable order [2] was: - ISP, - PHY, - sensor and disable: - PHY, - sensor, - ISP. It's possible that the powering order probably was irrelevant and just the streaming itself is important or actually it doesn't matter. Some testing would be appreciated. [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/kernel/v4.4/drivers/media/platform/rockchip/isp1/dev.c;drc=4372c82fc2f0d63b4fbb5390e72464709f8a34e9;l=110 [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/kernel/v4.4/drivers/media/platform/rockchip/isp1/dev.c;drc=4372c82fc2f0d63b4fbb5390e72464709f8a34e9;l=175 Best regards, Tomasz
On 10/16/20 11:28 AM, Dafna Hirschfeld wrote: > Hi, > > Am 15.10.20 um 21:57 schrieb Helen Koike: >> Allow streaming from self picture path and main picture path at the same >> time. >> >> Take care for s_stream() callbacks to not be called twice. >> When starting a stream, s_stream(true) shouldn't be called for the isp >> and the sensor if the other stream is already enabled (since it was >> already called). >> When stopping a stream, s_stream(false) shouldn't be called for isp and >> the sensor if the other stream is still enabled. >> >> Remove the callback function scheme for navigating through the topology, >> simplifying the code, improving readability, while calling >> media_pipeline_{start,stop}() in the right order. >> >> Remove multistreaming item from the TODO list. >> >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> >> --- >> Hello, >> >> Since we didn't reach an agreement on the helpers in the core[1], I'm >> sending this patch to fix this limitation only for rkisp1. >> >> [1] https://patchwork.linuxtv.org/project/linux-media/cover/20200415013044.1778572-1-helen.koike@collabora.com/ >> >> If we decide to add the helpers in the future, we can clean up drivers >> even more, but I don't want to block this feature. >> >> Overview of the patch: >> ====================== >> >> * Rename rkisp1_stream_{start,stop}() to >> rkisp1_cap_stream_{enable,disable}() to clarify the difference between >> other stream enable/disable functions >> >> * Implement rkisp1_pipeline_stream_{enable,disable}() to replace >> rkisp1_pipeline_{enable,disable}_cb() and rkisp1_pipeline_sink_walk(), >> which were removed. >> >> * Call rkisp1_cap_stream_{enable,disable}() from >> rkisp1_pipeline_stream_{enable,disable}() for better >> unwind handling and function name semantics. >> >> * Call media_pipeline_{start,stop}() in the right order. >> >> * Remove item from TODO list (I also reviewed the use of the >> is_streaming var in the code and lgtm). >> >> This patch was tested on rockpi4 board with: >> ============================================ >> >> "media-ctl" "-d" "platform:rkisp1" "-r" >> "media-ctl" "-d" "platform:rkisp1" "-l" "'imx219 4-0010':0 -> 'rkisp1_isp':0 [1]" >> "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_selfpath':0 [1]" >> "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]" >> >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"imx219 4-0010":0 [fmt:SRGGB10_1X10/1640x1232]' >> >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":0 [fmt:SRGGB10_1X10/1640x1232 crop: (0,0)/1600x1200]' >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":2 [fmt:YUYV8_2X8/1600x1200 crop: (0,0)/1500x1100]' >> >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":1 [fmt:YUYV8_2X8/900x800]' >> >> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "width=900,height=800," >> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "pixelformat=422P" >> >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' >> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]' >> >> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "width=900,height=800," >> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "pixelformat=422P" >> >> sleep 1 >> >> time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "--stream-mmap" "--stream-count" "100" & >> time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "--stream-mmap" "--stream-count" "100" & >> >> wait >> echo "Completed" >> >> Thanks >> Helen >> --- >> drivers/staging/media/rkisp1/TODO | 3 - >> drivers/staging/media/rkisp1/rkisp1-capture.c | 227 +++++++++--------- >> 2 files changed, 113 insertions(+), 117 deletions(-) >> >> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO >> index e7c8398fc2cef..a2dd0ad951c25 100644 >> --- a/drivers/staging/media/rkisp1/TODO >> +++ b/drivers/staging/media/rkisp1/TODO >> @@ -1,9 +1,6 @@ >> * Fix pad format size for statistics and parameters entities. >> * Fix checkpatch errors. >> * Add uapi docs. Remember to add documentation of how quantization is handled. >> -* streaming paths (mainpath and selfpath) check if the other path is streaming >> -in several places of the code, review this, specially that it doesn't seem it >> -supports streaming from both paths at the same time. >> NOTES: >> * All v4l2-compliance test must pass. >> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >> index b6f497ce3e95c..254936873c6c1 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >> @@ -830,71 +830,43 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap, >> } >> /* >> - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb >> - * @from: entity at which to start pipeline walk >> - * @until: entity at which to stop pipeline walk >> - * >> - * Walk the entities chain starting at the pipeline video node and stop >> - * all subdevices in the chain. >> - * >> - * If the until argument isn't NULL, stop the pipeline walk when reaching the >> - * until entity. This is used to disable a partially started pipeline due to a >> - * subdev start error. >> + * Most of registers inside rockchip ISP1 have shadow register since >> + * they must be not be changed during processing a frame. >> + * Usually, each sub-module updates its shadow register after >> + * processing the last pixel of a frame. >> */ >> -static int rkisp1_pipeline_sink_walk(struct media_entity *from, >> - struct media_entity *until, >> - int (*cb)(struct media_entity *from, >> - struct media_entity *curr)) >> +static void rkisp1_cap_stream_enable(struct rkisp1_capture *cap) >> { >> - struct media_entity *entity = from; >> - struct media_pad *pad; >> - unsigned int i; >> - int ret; >> - >> - while (1) { >> - pad = NULL; >> - /* Find remote source pad */ >> - for (i = 0; i < entity->num_pads; i++) { >> - struct media_pad *spad = &entity->pads[i]; >> - >> - if (!(spad->flags & MEDIA_PAD_FL_SINK)) >> - continue; >> - pad = media_entity_remote_pad(spad); >> - if (pad && is_media_entity_v4l2_subdev(pad->entity)) >> - break; >> - } >> - if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) >> - break; >> + struct rkisp1_device *rkisp1 = cap->rkisp1; >> + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; >> - entity = pad->entity; >> - if (entity == until) >> - break; >> + cap->ops->set_data_path(cap); >> + cap->ops->config(cap); >> - ret = cb(from, entity); >> - if (ret) >> - return ret; >> + /* Setup a buffer for the next frame */ >> + spin_lock_irq(&cap->buf.lock); >> + rkisp1_set_next_buf(cap); >> + cap->ops->enable(cap); >> + /* It's safe to config ACTIVE and SHADOW regs for the >> + * first stream. While when the second is starting, do NOT >> + * force update because it also update the first one. >> + * >> + * The latter case would drop one more buf(that is 2) since >> + * there's not buf in shadow when the second FE received. This's >> + * also required because the second FE maybe corrupt especially >> + * when run at 120fps. >> + */ >> + if (!other->is_streaming) { >> + /* force cfg update */ >> + rkisp1_write(rkisp1, >> + RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); >> + rkisp1_set_next_buf(cap); >> } >> - >> - return 0; >> -} >> - >> -static int rkisp1_pipeline_disable_cb(struct media_entity *from, >> - struct media_entity *curr) >> -{ >> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); >> - >> - return v4l2_subdev_call(sd, video, s_stream, false); >> -} >> - >> -static int rkisp1_pipeline_enable_cb(struct media_entity *from, >> - struct media_entity *curr) >> -{ >> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); >> - >> - return v4l2_subdev_call(sd, video, s_stream, true); >> + spin_unlock_irq(&cap->buf.lock); >> + cap->is_streaming = true; >> } >> -static void rkisp1_stream_stop(struct rkisp1_capture *cap) >> +static void rkisp1_cap_stream_disable(struct rkisp1_capture *cap) >> { >> int ret; >> @@ -911,6 +883,81 @@ static void rkisp1_stream_stop(struct rkisp1_capture *cap) >> } >> } >> +/* >> + * rkisp1_pipeline_stream_disable - disable nodes in the pipeline >> + * >> + * Call s_stream(false) in the reverse order from >> + * rkisp1_pipeline_stream_enable() and disable the DMA engine. >> + * Should be called before media_pipeline_stop() >> + */ >> +static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) >> + __must_hold(&cap->rkisp1->stream_lock) >> +{ >> + struct rkisp1_device *rkisp1 = cap->rkisp1; >> + >> + rkisp1_cap_stream_disable(cap); >> + >> + /* >> + * If the other capture is streaming, isp and sensor nodes shouldn't >> + * be disabled, skip them. >> + */ >> + if (rkisp1->pipe.streaming_count < 2) { >> + v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >> + false); >> + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); >> + } >> + >> + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, >> + false); > > I wonder if the order of the disable matters. If it should be from the sensor up to the > capture then we should call 'rkisp1_cap_stream_disable' at the end of this > function. > >> +} >> + >> +/* >> + * rkisp1_pipeline_stream_enable - enable nodes in the pipeline >> + * >> + * Enable the DMA Engine and call s_stream(true) through the pipeline. >> + * Should be called after media_pipeline_start() >> + */ >> +static int rkisp1_pipeline_stream_enable(struct rkisp1_capture *cap) >> + __must_hold(&cap->rkisp1->stream_lock) >> +{ >> + struct rkisp1_device *rkisp1 = cap->rkisp1; >> + int ret; >> + >> + rkisp1_cap_stream_enable(cap); >> + >> + ret = v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, >> + s_stream, true); >> + if (ret) >> + return ret; > > In case of error here, shouldn't we call 'rkisp1_cap_stream_disable'? Yes, thanks for catching it :) > >> + >> + /* >> + * If the other capture is streaming, isp and sensor nodes are already >> + * enabled, skip them. >> + */ >> + if (rkisp1->pipe.streaming_count > 1) >> + return 0; >> + >> + ret = v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, true); >> + if (ret) >> + goto err_disable_rsz; >> + >> + ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >> + true); >> + if (ret) >> + goto err_disable_isp; >> + >> + return 0; >> + >> +err_disable_isp: >> + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); >> +err_disable_rsz: >> + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, >> + false); >> + rkisp1_cap_stream_disable(cap); >> + >> + return ret; >> +} >> + >> static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) >> { >> struct rkisp1_capture *cap = queue->drv_priv; >> @@ -920,13 +967,8 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) >> mutex_lock(&cap->rkisp1->stream_lock); >> - rkisp1_stream_stop(cap); >> + rkisp1_pipeline_stream_disable(cap); >> media_pipeline_stop(&node->vdev.entity); >> - ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL, >> - rkisp1_pipeline_disable_cb); >> - if (ret) >> - dev_err(rkisp1->dev, >> - "pipeline stream-off failed error:%d\n", ret); >> rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR); >> @@ -940,43 +982,6 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) >> mutex_unlock(&cap->rkisp1->stream_lock); >> } >> -/* >> - * Most of registers inside rockchip ISP1 have shadow register since >> - * they must be not be changed during processing a frame. >> - * Usually, each sub-module updates its shadow register after >> - * processing the last pixel of a frame. >> - */ >> -static void rkisp1_stream_start(struct rkisp1_capture *cap) >> -{ >> - struct rkisp1_device *rkisp1 = cap->rkisp1; >> - struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; >> - >> - cap->ops->set_data_path(cap); >> - cap->ops->config(cap); >> - >> - /* Setup a buffer for the next frame */ >> - spin_lock_irq(&cap->buf.lock); >> - rkisp1_set_next_buf(cap); >> - cap->ops->enable(cap); >> - /* It's safe to config ACTIVE and SHADOW regs for the >> - * first stream. While when the second is starting, do NOT >> - * force update because it also update the first one. >> - * >> - * The latter case would drop one more buf(that is 2) since >> - * there's not buf in shadow when the second FE received. This's >> - * also required because the second FE maybe corrupt especially >> - * when run at 120fps. >> - */ >> - if (!other->is_streaming) { >> - /* force cfg update */ >> - rkisp1_write(rkisp1, >> - RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); >> - rkisp1_set_next_buf(cap); >> - } >> - spin_unlock_irq(&cap->buf.lock); >> - cap->is_streaming = true; >> -} >> - >> static int >> rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) >> { >> @@ -1001,28 +1006,22 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) >> goto err_pipe_pm_put; >> } >> > > The previous code with the 'rkisp1_pipeline_sink_walk' only followed enabled links. > We should probably first check if the link between the resizer and the isp is enabled > and also between the isp to the sensor. and return -EPIPE if not. Very good point, yes, I'll fix this in v2. > Actually I think that the code that set the 'active_sensor' (callingrkisp1_get_remote_sensor) > should move to here since this also check if the link to the sensor is enabled. ok Thanks for your review Helen > > Thanks, > Dafna > >> - rkisp1_stream_start(cap); >> - >> - /* start sub-devices */ >> - ret = rkisp1_pipeline_sink_walk(entity, NULL, >> - rkisp1_pipeline_enable_cb); >> - if (ret) >> - goto err_stop_stream; >> - >> ret = media_pipeline_start(entity, &cap->rkisp1->pipe); >> if (ret) { >> dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret); >> - goto err_pipe_disable; >> + goto err_v4l2_pm_put; >> } >> + ret = rkisp1_pipeline_stream_enable(cap); >> + if (ret) >> + goto err_media_pipeline_stop; >> mutex_unlock(&cap->rkisp1->stream_lock); >> return 0; >> -err_pipe_disable: >> - rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb); >> -err_stop_stream: >> - rkisp1_stream_stop(cap); >> +err_media_pipeline_stop: >> + media_pipeline_stop(entity); >> +err_v4l2_pm_put: >> v4l2_pipeline_pm_put(entity); >> err_pipe_pm_put: >> pm_runtime_put(cap->rkisp1->dev); >>
Hi Tomasz. On 10/16/20 11:53 AM, Tomasz Figa wrote: > On Fri, Oct 16, 2020 at 4:28 PM Dafna Hirschfeld > <dafna.hirschfeld@collabora.com> wrote: >> >> Hi, >> >> Am 15.10.20 um 21:57 schrieb Helen Koike: >>> Allow streaming from self picture path and main picture path at the same >>> time. >>> >>> Take care for s_stream() callbacks to not be called twice. >>> When starting a stream, s_stream(true) shouldn't be called for the isp >>> and the sensor if the other stream is already enabled (since it was >>> already called). >>> When stopping a stream, s_stream(false) shouldn't be called for isp and >>> the sensor if the other stream is still enabled. >>> >>> Remove the callback function scheme for navigating through the topology, >>> simplifying the code, improving readability, while calling >>> media_pipeline_{start,stop}() in the right order. >>> >>> Remove multistreaming item from the TODO list. >>> >>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>> >>> --- >>> Hello, >>> >>> Since we didn't reach an agreement on the helpers in the core[1], I'm >>> sending this patch to fix this limitation only for rkisp1. >>> >>> [1] https://patchwork.linuxtv.org/project/linux-media/cover/20200415013044.1778572-1-helen.koike@collabora.com/ >>> >>> If we decide to add the helpers in the future, we can clean up drivers >>> even more, but I don't want to block this feature. >>> >>> Overview of the patch: >>> ====================== >>> >>> * Rename rkisp1_stream_{start,stop}() to >>> rkisp1_cap_stream_{enable,disable}() to clarify the difference between >>> other stream enable/disable functions >>> >>> * Implement rkisp1_pipeline_stream_{enable,disable}() to replace >>> rkisp1_pipeline_{enable,disable}_cb() and rkisp1_pipeline_sink_walk(), >>> which were removed. >>> >>> * Call rkisp1_cap_stream_{enable,disable}() from >>> rkisp1_pipeline_stream_{enable,disable}() for better >>> unwind handling and function name semantics. >>> >>> * Call media_pipeline_{start,stop}() in the right order. >>> >>> * Remove item from TODO list (I also reviewed the use of the >>> is_streaming var in the code and lgtm). >>> >>> This patch was tested on rockpi4 board with: >>> ============================================ >>> >>> "media-ctl" "-d" "platform:rkisp1" "-r" >>> "media-ctl" "-d" "platform:rkisp1" "-l" "'imx219 4-0010':0 -> 'rkisp1_isp':0 [1]" >>> "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_selfpath':0 [1]" >>> "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]" >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"imx219 4-0010":0 [fmt:SRGGB10_1X10/1640x1232]' >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":0 [fmt:SRGGB10_1X10/1640x1232 crop: (0,0)/1600x1200]' >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":2 [fmt:YUYV8_2X8/1600x1200 crop: (0,0)/1500x1100]' >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":1 [fmt:YUYV8_2X8/900x800]' >>> >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "width=900,height=800," >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "pixelformat=422P" >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]' >>> >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "width=900,height=800," >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "pixelformat=422P" >>> >>> sleep 1 >>> >>> time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "--stream-mmap" "--stream-count" "100" & >>> time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "--stream-mmap" "--stream-count" "100" & >>> >>> wait >>> echo "Completed" >>> >>> Thanks >>> Helen >>> --- >>> drivers/staging/media/rkisp1/TODO | 3 - >>> drivers/staging/media/rkisp1/rkisp1-capture.c | 227 +++++++++--------- >>> 2 files changed, 113 insertions(+), 117 deletions(-) >>> >>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO >>> index e7c8398fc2cef..a2dd0ad951c25 100644 >>> --- a/drivers/staging/media/rkisp1/TODO >>> +++ b/drivers/staging/media/rkisp1/TODO >>> @@ -1,9 +1,6 @@ >>> * Fix pad format size for statistics and parameters entities. >>> * Fix checkpatch errors. >>> * Add uapi docs. Remember to add documentation of how quantization is handled. >>> -* streaming paths (mainpath and selfpath) check if the other path is streaming >>> -in several places of the code, review this, specially that it doesn't seem it >>> -supports streaming from both paths at the same time. >>> >>> NOTES: >>> * All v4l2-compliance test must pass. >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> index b6f497ce3e95c..254936873c6c1 100644 >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> @@ -830,71 +830,43 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap, >>> } >>> >>> /* >>> - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb >>> - * @from: entity at which to start pipeline walk >>> - * @until: entity at which to stop pipeline walk >>> - * >>> - * Walk the entities chain starting at the pipeline video node and stop >>> - * all subdevices in the chain. >>> - * >>> - * If the until argument isn't NULL, stop the pipeline walk when reaching the >>> - * until entity. This is used to disable a partially started pipeline due to a >>> - * subdev start error. >>> + * Most of registers inside rockchip ISP1 have shadow register since >>> + * they must be not be changed during processing a frame. >>> + * Usually, each sub-module updates its shadow register after >>> + * processing the last pixel of a frame. >>> */ >>> -static int rkisp1_pipeline_sink_walk(struct media_entity *from, >>> - struct media_entity *until, >>> - int (*cb)(struct media_entity *from, >>> - struct media_entity *curr)) >>> +static void rkisp1_cap_stream_enable(struct rkisp1_capture *cap) >>> { >>> - struct media_entity *entity = from; >>> - struct media_pad *pad; >>> - unsigned int i; >>> - int ret; >>> - >>> - while (1) { >>> - pad = NULL; >>> - /* Find remote source pad */ >>> - for (i = 0; i < entity->num_pads; i++) { >>> - struct media_pad *spad = &entity->pads[i]; >>> - >>> - if (!(spad->flags & MEDIA_PAD_FL_SINK)) >>> - continue; >>> - pad = media_entity_remote_pad(spad); >>> - if (pad && is_media_entity_v4l2_subdev(pad->entity)) >>> - break; >>> - } >>> - if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) >>> - break; >>> + struct rkisp1_device *rkisp1 = cap->rkisp1; >>> + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; >>> >>> - entity = pad->entity; >>> - if (entity == until) >>> - break; >>> + cap->ops->set_data_path(cap); >>> + cap->ops->config(cap); >>> >>> - ret = cb(from, entity); >>> - if (ret) >>> - return ret; >>> + /* Setup a buffer for the next frame */ >>> + spin_lock_irq(&cap->buf.lock); >>> + rkisp1_set_next_buf(cap); >>> + cap->ops->enable(cap); >>> + /* It's safe to config ACTIVE and SHADOW regs for the >>> + * first stream. While when the second is starting, do NOT >>> + * force update because it also update the first one. >>> + * >>> + * The latter case would drop one more buf(that is 2) since >>> + * there's not buf in shadow when the second FE received. This's >>> + * also required because the second FE maybe corrupt especially >>> + * when run at 120fps. >>> + */ >>> + if (!other->is_streaming) { >>> + /* force cfg update */ >>> + rkisp1_write(rkisp1, >>> + RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); >>> + rkisp1_set_next_buf(cap); >>> } >>> - >>> - return 0; >>> -} >>> - >>> -static int rkisp1_pipeline_disable_cb(struct media_entity *from, >>> - struct media_entity *curr) >>> -{ >>> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); >>> - >>> - return v4l2_subdev_call(sd, video, s_stream, false); >>> -} >>> - >>> -static int rkisp1_pipeline_enable_cb(struct media_entity *from, >>> - struct media_entity *curr) >>> -{ >>> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); >>> - >>> - return v4l2_subdev_call(sd, video, s_stream, true); >>> + spin_unlock_irq(&cap->buf.lock); >>> + cap->is_streaming = true; >>> } >>> >>> -static void rkisp1_stream_stop(struct rkisp1_capture *cap) >>> +static void rkisp1_cap_stream_disable(struct rkisp1_capture *cap) >>> { >>> int ret; >>> >>> @@ -911,6 +883,81 @@ static void rkisp1_stream_stop(struct rkisp1_capture *cap) >>> } >>> } >>> >>> +/* >>> + * rkisp1_pipeline_stream_disable - disable nodes in the pipeline >>> + * >>> + * Call s_stream(false) in the reverse order from >>> + * rkisp1_pipeline_stream_enable() and disable the DMA engine. >>> + * Should be called before media_pipeline_stop() >>> + */ >>> +static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) >>> + __must_hold(&cap->rkisp1->stream_lock) >>> +{ >>> + struct rkisp1_device *rkisp1 = cap->rkisp1; >>> + >>> + rkisp1_cap_stream_disable(cap); >>> + >>> + /* >>> + * If the other capture is streaming, isp and sensor nodes shouldn't >>> + * be disabled, skip them. >>> + */ >>> + if (rkisp1->pipe.streaming_count < 2) { >>> + v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >>> + false); >>> + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); >>> + } >>> + >>> + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, >>> + false); >> >> I wonder if the order of the disable matters. If it should be from the sensor up to the >> capture then we should call 'rkisp1_cap_stream_disable' at the end of this >> function. > > According to our downstream code [1] and my recollection of discussion > with Rockchip, the power up order was: > - ISP > - sensor > - PHY > and power down the opposite. > > That said, the sensor drivers we had didn't implement s_power and used > runtime PM at streaming time, so I think in practice the order ended > up as: > - ISP, > - PHY, > - sensor. > > The streaming enable order [2] was: > - ISP, > - PHY, > - sensor > and disable: > - PHY, > - sensor, > - ISP. > > It's possible that the powering order probably was irrelevant and just > the streaming itself is important or actually it doesn't matter. Some > testing would be appreciated. > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/kernel/v4.4/drivers/media/platform/rockchip/isp1/dev.c;drc=4372c82fc2f0d63b4fbb5390e72464709f8a34e9;l=110 > [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/kernel/v4.4/drivers/media/platform/rockchip/isp1/dev.c;drc=4372c82fc2f0d63b4fbb5390e72464709f8a34e9;l=175 Thanks for this info, I can do some tests. Regards, Helen > > Best regards, > Tomasz >
Hello, On 10/16/20 3:11 PM, Helen Koike wrote: > > > On 10/16/20 11:28 AM, Dafna Hirschfeld wrote: >> Hi, >> >> Am 15.10.20 um 21:57 schrieb Helen Koike: >>> Allow streaming from self picture path and main picture path at the same >>> time. >>> >>> Take care for s_stream() callbacks to not be called twice. >>> When starting a stream, s_stream(true) shouldn't be called for the isp >>> and the sensor if the other stream is already enabled (since it was >>> already called). >>> When stopping a stream, s_stream(false) shouldn't be called for isp and >>> the sensor if the other stream is still enabled. >>> >>> Remove the callback function scheme for navigating through the topology, >>> simplifying the code, improving readability, while calling >>> media_pipeline_{start,stop}() in the right order. >>> >>> Remove multistreaming item from the TODO list. >>> >>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>> >>> --- >>> Hello, >>> >>> Since we didn't reach an agreement on the helpers in the core[1], I'm >>> sending this patch to fix this limitation only for rkisp1. >>> >>> [1] https://patchwork.linuxtv.org/project/linux-media/cover/20200415013044.1778572-1-helen.koike@collabora.com/ >>> >>> If we decide to add the helpers in the future, we can clean up drivers >>> even more, but I don't want to block this feature. >>> >>> Overview of the patch: >>> ====================== >>> >>> * Rename rkisp1_stream_{start,stop}() to >>> rkisp1_cap_stream_{enable,disable}() to clarify the difference between >>> other stream enable/disable functions >>> >>> * Implement rkisp1_pipeline_stream_{enable,disable}() to replace >>> rkisp1_pipeline_{enable,disable}_cb() and rkisp1_pipeline_sink_walk(), >>> which were removed. >>> >>> * Call rkisp1_cap_stream_{enable,disable}() from >>> rkisp1_pipeline_stream_{enable,disable}() for better >>> unwind handling and function name semantics. >>> >>> * Call media_pipeline_{start,stop}() in the right order. >>> >>> * Remove item from TODO list (I also reviewed the use of the >>> is_streaming var in the code and lgtm). >>> >>> This patch was tested on rockpi4 board with: >>> ============================================ >>> >>> "media-ctl" "-d" "platform:rkisp1" "-r" >>> "media-ctl" "-d" "platform:rkisp1" "-l" "'imx219 4-0010':0 -> 'rkisp1_isp':0 [1]" >>> "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_selfpath':0 [1]" >>> "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]" >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"imx219 4-0010":0 [fmt:SRGGB10_1X10/1640x1232]' >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":0 [fmt:SRGGB10_1X10/1640x1232 crop: (0,0)/1600x1200]' >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":2 [fmt:YUYV8_2X8/1600x1200 crop: (0,0)/1500x1100]' >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":1 [fmt:YUYV8_2X8/900x800]' >>> >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "width=900,height=800," >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "pixelformat=422P" >>> >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' >>> "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]' >>> >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "width=900,height=800," >>> "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "pixelformat=422P" >>> >>> sleep 1 >>> >>> time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "--stream-mmap" "--stream-count" "100" & >>> time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "--stream-mmap" "--stream-count" "100" & >>> >>> wait >>> echo "Completed" >>> >>> Thanks >>> Helen >>> --- >>> drivers/staging/media/rkisp1/TODO | 3 - >>> drivers/staging/media/rkisp1/rkisp1-capture.c | 227 +++++++++--------- >>> 2 files changed, 113 insertions(+), 117 deletions(-) >>> >>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO >>> index e7c8398fc2cef..a2dd0ad951c25 100644 >>> --- a/drivers/staging/media/rkisp1/TODO >>> +++ b/drivers/staging/media/rkisp1/TODO >>> @@ -1,9 +1,6 @@ >>> * Fix pad format size for statistics and parameters entities. >>> * Fix checkpatch errors. >>> * Add uapi docs. Remember to add documentation of how quantization is handled. >>> -* streaming paths (mainpath and selfpath) check if the other path is streaming >>> -in several places of the code, review this, specially that it doesn't seem it >>> -supports streaming from both paths at the same time. >>> NOTES: >>> * All v4l2-compliance test must pass. >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> index b6f497ce3e95c..254936873c6c1 100644 >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> @@ -830,71 +830,43 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap, >>> } >>> /* >>> - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb >>> - * @from: entity at which to start pipeline walk >>> - * @until: entity at which to stop pipeline walk >>> - * >>> - * Walk the entities chain starting at the pipeline video node and stop >>> - * all subdevices in the chain. >>> - * >>> - * If the until argument isn't NULL, stop the pipeline walk when reaching the >>> - * until entity. This is used to disable a partially started pipeline due to a >>> - * subdev start error. >>> + * Most of registers inside rockchip ISP1 have shadow register since >>> + * they must be not be changed during processing a frame. >>> + * Usually, each sub-module updates its shadow register after >>> + * processing the last pixel of a frame. >>> */ >>> -static int rkisp1_pipeline_sink_walk(struct media_entity *from, >>> - struct media_entity *until, >>> - int (*cb)(struct media_entity *from, >>> - struct media_entity *curr)) >>> +static void rkisp1_cap_stream_enable(struct rkisp1_capture *cap) >>> { >>> - struct media_entity *entity = from; >>> - struct media_pad *pad; >>> - unsigned int i; >>> - int ret; >>> - >>> - while (1) { >>> - pad = NULL; >>> - /* Find remote source pad */ >>> - for (i = 0; i < entity->num_pads; i++) { >>> - struct media_pad *spad = &entity->pads[i]; >>> - >>> - if (!(spad->flags & MEDIA_PAD_FL_SINK)) >>> - continue; >>> - pad = media_entity_remote_pad(spad); >>> - if (pad && is_media_entity_v4l2_subdev(pad->entity)) >>> - break; >>> - } >>> - if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) >>> - break; >>> + struct rkisp1_device *rkisp1 = cap->rkisp1; >>> + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; >>> - entity = pad->entity; >>> - if (entity == until) >>> - break; >>> + cap->ops->set_data_path(cap); >>> + cap->ops->config(cap); >>> - ret = cb(from, entity); >>> - if (ret) >>> - return ret; >>> + /* Setup a buffer for the next frame */ >>> + spin_lock_irq(&cap->buf.lock); >>> + rkisp1_set_next_buf(cap); >>> + cap->ops->enable(cap); >>> + /* It's safe to config ACTIVE and SHADOW regs for the >>> + * first stream. While when the second is starting, do NOT >>> + * force update because it also update the first one. >>> + * >>> + * The latter case would drop one more buf(that is 2) since >>> + * there's not buf in shadow when the second FE received. This's >>> + * also required because the second FE maybe corrupt especially >>> + * when run at 120fps. >>> + */ >>> + if (!other->is_streaming) { >>> + /* force cfg update */ >>> + rkisp1_write(rkisp1, >>> + RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); >>> + rkisp1_set_next_buf(cap); >>> } >>> - >>> - return 0; >>> -} >>> - >>> -static int rkisp1_pipeline_disable_cb(struct media_entity *from, >>> - struct media_entity *curr) >>> -{ >>> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); >>> - >>> - return v4l2_subdev_call(sd, video, s_stream, false); >>> -} >>> - >>> -static int rkisp1_pipeline_enable_cb(struct media_entity *from, >>> - struct media_entity *curr) >>> -{ >>> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); >>> - >>> - return v4l2_subdev_call(sd, video, s_stream, true); >>> + spin_unlock_irq(&cap->buf.lock); >>> + cap->is_streaming = true; >>> } >>> -static void rkisp1_stream_stop(struct rkisp1_capture *cap) >>> +static void rkisp1_cap_stream_disable(struct rkisp1_capture *cap) >>> { >>> int ret; >>> @@ -911,6 +883,81 @@ static void rkisp1_stream_stop(struct rkisp1_capture *cap) >>> } >>> } >>> +/* >>> + * rkisp1_pipeline_stream_disable - disable nodes in the pipeline >>> + * >>> + * Call s_stream(false) in the reverse order from >>> + * rkisp1_pipeline_stream_enable() and disable the DMA engine. >>> + * Should be called before media_pipeline_stop() >>> + */ >>> +static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) >>> + __must_hold(&cap->rkisp1->stream_lock) >>> +{ >>> + struct rkisp1_device *rkisp1 = cap->rkisp1; >>> + >>> + rkisp1_cap_stream_disable(cap); >>> + >>> + /* >>> + * If the other capture is streaming, isp and sensor nodes shouldn't >>> + * be disabled, skip them. >>> + */ >>> + if (rkisp1->pipe.streaming_count < 2) { >>> + v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >>> + false); >>> + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); >>> + } >>> + >>> + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, >>> + false); >> >> I wonder if the order of the disable matters. If it should be from the sensor up to the >> capture then we should call 'rkisp1_cap_stream_disable' at the end of this >> function. The only issue I saw was if I enable the sensor before the ISP. Even if the order in disabling the capture doesn't matter much, it is nice to keep the logic, thanks for this comment. >> >>> +} >>> + >>> +/* >>> + * rkisp1_pipeline_stream_enable - enable nodes in the pipeline >>> + * >>> + * Enable the DMA Engine and call s_stream(true) through the pipeline. >>> + * Should be called after media_pipeline_start() >>> + */ >>> +static int rkisp1_pipeline_stream_enable(struct rkisp1_capture *cap) >>> + __must_hold(&cap->rkisp1->stream_lock) >>> +{ >>> + struct rkisp1_device *rkisp1 = cap->rkisp1; >>> + int ret; >>> + >>> + rkisp1_cap_stream_enable(cap); >>> + >>> + ret = v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, >>> + s_stream, true); >>> + if (ret) >>> + return ret; >> >> In case of error here, shouldn't we call 'rkisp1_cap_stream_disable'? > > Yes, thanks for catching it :) > >> >>> + >>> + /* >>> + * If the other capture is streaming, isp and sensor nodes are already >>> + * enabled, skip them. >>> + */ >>> + if (rkisp1->pipe.streaming_count > 1) >>> + return 0; >>> + >>> + ret = v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, true); >>> + if (ret) >>> + goto err_disable_rsz; >>> + >>> + ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >>> + true); >>> + if (ret) >>> + goto err_disable_isp; >>> + >>> + return 0; >>> + >>> +err_disable_isp: >>> + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); >>> +err_disable_rsz: >>> + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, >>> + false); >>> + rkisp1_cap_stream_disable(cap); >>> + >>> + return ret; >>> +} >>> + >>> static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) >>> { >>> struct rkisp1_capture *cap = queue->drv_priv; >>> @@ -920,13 +967,8 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) >>> mutex_lock(&cap->rkisp1->stream_lock); >>> - rkisp1_stream_stop(cap); >>> + rkisp1_pipeline_stream_disable(cap); >>> media_pipeline_stop(&node->vdev.entity); >>> - ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL, >>> - rkisp1_pipeline_disable_cb); >>> - if (ret) >>> - dev_err(rkisp1->dev, >>> - "pipeline stream-off failed error:%d\n", ret); >>> rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR); >>> @@ -940,43 +982,6 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) >>> mutex_unlock(&cap->rkisp1->stream_lock); >>> } >>> -/* >>> - * Most of registers inside rockchip ISP1 have shadow register since >>> - * they must be not be changed during processing a frame. >>> - * Usually, each sub-module updates its shadow register after >>> - * processing the last pixel of a frame. >>> - */ >>> -static void rkisp1_stream_start(struct rkisp1_capture *cap) >>> -{ >>> - struct rkisp1_device *rkisp1 = cap->rkisp1; >>> - struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; >>> - >>> - cap->ops->set_data_path(cap); >>> - cap->ops->config(cap); >>> - >>> - /* Setup a buffer for the next frame */ >>> - spin_lock_irq(&cap->buf.lock); >>> - rkisp1_set_next_buf(cap); >>> - cap->ops->enable(cap); >>> - /* It's safe to config ACTIVE and SHADOW regs for the >>> - * first stream. While when the second is starting, do NOT >>> - * force update because it also update the first one. >>> - * >>> - * The latter case would drop one more buf(that is 2) since >>> - * there's not buf in shadow when the second FE received. This's >>> - * also required because the second FE maybe corrupt especially >>> - * when run at 120fps. >>> - */ >>> - if (!other->is_streaming) { >>> - /* force cfg update */ >>> - rkisp1_write(rkisp1, >>> - RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); >>> - rkisp1_set_next_buf(cap); >>> - } >>> - spin_unlock_irq(&cap->buf.lock); >>> - cap->is_streaming = true; >>> -} >>> - >>> static int >>> rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) >>> { >>> @@ -1001,28 +1006,22 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) >>> goto err_pipe_pm_put; >>> } >>> >> >> The previous code with the 'rkisp1_pipeline_sink_walk' only followed enabled links. >> We should probably first check if the link between the resizer and the isp is enabled >> and also between the isp to the sensor. and return -EPIPE if not. > > Very good point, yes, I'll fix this in v2. Actually this is not required, since resizer and isp sink pads use MEDIA_PAD_FL_MUST_CONNECT, media_pipeline_start() will failed with -ENOLINK if it is not connected. Thanks Helen > >> Actually I think that the code that set the 'active_sensor' (callingrkisp1_get_remote_sensor) >> should move to here since this also check if the link to the sensor is enabled. > > ok > > Thanks for your review > Helen > >> >> Thanks, >> Dafna >> >>> - rkisp1_stream_start(cap); >>> - >>> - /* start sub-devices */ >>> - ret = rkisp1_pipeline_sink_walk(entity, NULL, >>> - rkisp1_pipeline_enable_cb); >>> - if (ret) >>> - goto err_stop_stream; >>> - >>> ret = media_pipeline_start(entity, &cap->rkisp1->pipe); >>> if (ret) { >>> dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret); >>> - goto err_pipe_disable; >>> + goto err_v4l2_pm_put; >>> } >>> + ret = rkisp1_pipeline_stream_enable(cap); >>> + if (ret) >>> + goto err_media_pipeline_stop; >>> mutex_unlock(&cap->rkisp1->stream_lock); >>> return 0; >>> -err_pipe_disable: >>> - rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb); >>> -err_stop_stream: >>> - rkisp1_stream_stop(cap); >>> +err_media_pipeline_stop: >>> + media_pipeline_stop(entity); >>> +err_v4l2_pm_put: >>> v4l2_pipeline_pm_put(entity); >>> err_pipe_pm_put: >>> pm_runtime_put(cap->rkisp1->dev); >>> >
diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO index e7c8398fc2cef..a2dd0ad951c25 100644 --- a/drivers/staging/media/rkisp1/TODO +++ b/drivers/staging/media/rkisp1/TODO @@ -1,9 +1,6 @@ * Fix pad format size for statistics and parameters entities. * Fix checkpatch errors. * Add uapi docs. Remember to add documentation of how quantization is handled. -* streaming paths (mainpath and selfpath) check if the other path is streaming -in several places of the code, review this, specially that it doesn't seem it -supports streaming from both paths at the same time. NOTES: * All v4l2-compliance test must pass. diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c index b6f497ce3e95c..254936873c6c1 100644 --- a/drivers/staging/media/rkisp1/rkisp1-capture.c +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c @@ -830,71 +830,43 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture *cap, } /* - * rkisp1_pipeline_sink_walk - Walk through the pipeline and call cb - * @from: entity at which to start pipeline walk - * @until: entity at which to stop pipeline walk - * - * Walk the entities chain starting at the pipeline video node and stop - * all subdevices in the chain. - * - * If the until argument isn't NULL, stop the pipeline walk when reaching the - * until entity. This is used to disable a partially started pipeline due to a - * subdev start error. + * Most of registers inside rockchip ISP1 have shadow register since + * they must be not be changed during processing a frame. + * Usually, each sub-module updates its shadow register after + * processing the last pixel of a frame. */ -static int rkisp1_pipeline_sink_walk(struct media_entity *from, - struct media_entity *until, - int (*cb)(struct media_entity *from, - struct media_entity *curr)) +static void rkisp1_cap_stream_enable(struct rkisp1_capture *cap) { - struct media_entity *entity = from; - struct media_pad *pad; - unsigned int i; - int ret; - - while (1) { - pad = NULL; - /* Find remote source pad */ - for (i = 0; i < entity->num_pads; i++) { - struct media_pad *spad = &entity->pads[i]; - - if (!(spad->flags & MEDIA_PAD_FL_SINK)) - continue; - pad = media_entity_remote_pad(spad); - if (pad && is_media_entity_v4l2_subdev(pad->entity)) - break; - } - if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) - break; + struct rkisp1_device *rkisp1 = cap->rkisp1; + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; - entity = pad->entity; - if (entity == until) - break; + cap->ops->set_data_path(cap); + cap->ops->config(cap); - ret = cb(from, entity); - if (ret) - return ret; + /* Setup a buffer for the next frame */ + spin_lock_irq(&cap->buf.lock); + rkisp1_set_next_buf(cap); + cap->ops->enable(cap); + /* It's safe to config ACTIVE and SHADOW regs for the + * first stream. While when the second is starting, do NOT + * force update because it also update the first one. + * + * The latter case would drop one more buf(that is 2) since + * there's not buf in shadow when the second FE received. This's + * also required because the second FE maybe corrupt especially + * when run at 120fps. + */ + if (!other->is_streaming) { + /* force cfg update */ + rkisp1_write(rkisp1, + RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); + rkisp1_set_next_buf(cap); } - - return 0; -} - -static int rkisp1_pipeline_disable_cb(struct media_entity *from, - struct media_entity *curr) -{ - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); - - return v4l2_subdev_call(sd, video, s_stream, false); -} - -static int rkisp1_pipeline_enable_cb(struct media_entity *from, - struct media_entity *curr) -{ - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(curr); - - return v4l2_subdev_call(sd, video, s_stream, true); + spin_unlock_irq(&cap->buf.lock); + cap->is_streaming = true; } -static void rkisp1_stream_stop(struct rkisp1_capture *cap) +static void rkisp1_cap_stream_disable(struct rkisp1_capture *cap) { int ret; @@ -911,6 +883,81 @@ static void rkisp1_stream_stop(struct rkisp1_capture *cap) } } +/* + * rkisp1_pipeline_stream_disable - disable nodes in the pipeline + * + * Call s_stream(false) in the reverse order from + * rkisp1_pipeline_stream_enable() and disable the DMA engine. + * Should be called before media_pipeline_stop() + */ +static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) + __must_hold(&cap->rkisp1->stream_lock) +{ + struct rkisp1_device *rkisp1 = cap->rkisp1; + + rkisp1_cap_stream_disable(cap); + + /* + * If the other capture is streaming, isp and sensor nodes shouldn't + * be disabled, skip them. + */ + if (rkisp1->pipe.streaming_count < 2) { + v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, + false); + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); + } + + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, + false); +} + +/* + * rkisp1_pipeline_stream_enable - enable nodes in the pipeline + * + * Enable the DMA Engine and call s_stream(true) through the pipeline. + * Should be called after media_pipeline_start() + */ +static int rkisp1_pipeline_stream_enable(struct rkisp1_capture *cap) + __must_hold(&cap->rkisp1->stream_lock) +{ + struct rkisp1_device *rkisp1 = cap->rkisp1; + int ret; + + rkisp1_cap_stream_enable(cap); + + ret = v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, + s_stream, true); + if (ret) + return ret; + + /* + * If the other capture is streaming, isp and sensor nodes are already + * enabled, skip them. + */ + if (rkisp1->pipe.streaming_count > 1) + return 0; + + ret = v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, true); + if (ret) + goto err_disable_rsz; + + ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, + true); + if (ret) + goto err_disable_isp; + + return 0; + +err_disable_isp: + v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); +err_disable_rsz: + v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, + false); + rkisp1_cap_stream_disable(cap); + + return ret; +} + static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) { struct rkisp1_capture *cap = queue->drv_priv; @@ -920,13 +967,8 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) mutex_lock(&cap->rkisp1->stream_lock); - rkisp1_stream_stop(cap); + rkisp1_pipeline_stream_disable(cap); media_pipeline_stop(&node->vdev.entity); - ret = rkisp1_pipeline_sink_walk(&node->vdev.entity, NULL, - rkisp1_pipeline_disable_cb); - if (ret) - dev_err(rkisp1->dev, - "pipeline stream-off failed error:%d\n", ret); rkisp1_return_all_buffers(cap, VB2_BUF_STATE_ERROR); @@ -940,43 +982,6 @@ static void rkisp1_vb2_stop_streaming(struct vb2_queue *queue) mutex_unlock(&cap->rkisp1->stream_lock); } -/* - * Most of registers inside rockchip ISP1 have shadow register since - * they must be not be changed during processing a frame. - * Usually, each sub-module updates its shadow register after - * processing the last pixel of a frame. - */ -static void rkisp1_stream_start(struct rkisp1_capture *cap) -{ - struct rkisp1_device *rkisp1 = cap->rkisp1; - struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1]; - - cap->ops->set_data_path(cap); - cap->ops->config(cap); - - /* Setup a buffer for the next frame */ - spin_lock_irq(&cap->buf.lock); - rkisp1_set_next_buf(cap); - cap->ops->enable(cap); - /* It's safe to config ACTIVE and SHADOW regs for the - * first stream. While when the second is starting, do NOT - * force update because it also update the first one. - * - * The latter case would drop one more buf(that is 2) since - * there's not buf in shadow when the second FE received. This's - * also required because the second FE maybe corrupt especially - * when run at 120fps. - */ - if (!other->is_streaming) { - /* force cfg update */ - rkisp1_write(rkisp1, - RKISP1_CIF_MI_INIT_SOFT_UPD, RKISP1_CIF_MI_INIT); - rkisp1_set_next_buf(cap); - } - spin_unlock_irq(&cap->buf.lock); - cap->is_streaming = true; -} - static int rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) { @@ -1001,28 +1006,22 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count) goto err_pipe_pm_put; } - rkisp1_stream_start(cap); - - /* start sub-devices */ - ret = rkisp1_pipeline_sink_walk(entity, NULL, - rkisp1_pipeline_enable_cb); - if (ret) - goto err_stop_stream; - ret = media_pipeline_start(entity, &cap->rkisp1->pipe); if (ret) { dev_err(cap->rkisp1->dev, "start pipeline failed %d\n", ret); - goto err_pipe_disable; + goto err_v4l2_pm_put; } + ret = rkisp1_pipeline_stream_enable(cap); + if (ret) + goto err_media_pipeline_stop; mutex_unlock(&cap->rkisp1->stream_lock); return 0; -err_pipe_disable: - rkisp1_pipeline_sink_walk(entity, NULL, rkisp1_pipeline_disable_cb); -err_stop_stream: - rkisp1_stream_stop(cap); +err_media_pipeline_stop: + media_pipeline_stop(entity); +err_v4l2_pm_put: v4l2_pipeline_pm_put(entity); err_pipe_pm_put: pm_runtime_put(cap->rkisp1->dev);
Allow streaming from self picture path and main picture path at the same time. Take care for s_stream() callbacks to not be called twice. When starting a stream, s_stream(true) shouldn't be called for the isp and the sensor if the other stream is already enabled (since it was already called). When stopping a stream, s_stream(false) shouldn't be called for isp and the sensor if the other stream is still enabled. Remove the callback function scheme for navigating through the topology, simplifying the code, improving readability, while calling media_pipeline_{start,stop}() in the right order. Remove multistreaming item from the TODO list. Signed-off-by: Helen Koike <helen.koike@collabora.com> --- Hello, Since we didn't reach an agreement on the helpers in the core[1], I'm sending this patch to fix this limitation only for rkisp1. [1] https://patchwork.linuxtv.org/project/linux-media/cover/20200415013044.1778572-1-helen.koike@collabora.com/ If we decide to add the helpers in the future, we can clean up drivers even more, but I don't want to block this feature. Overview of the patch: ====================== * Rename rkisp1_stream_{start,stop}() to rkisp1_cap_stream_{enable,disable}() to clarify the difference between other stream enable/disable functions * Implement rkisp1_pipeline_stream_{enable,disable}() to replace rkisp1_pipeline_{enable,disable}_cb() and rkisp1_pipeline_sink_walk(), which were removed. * Call rkisp1_cap_stream_{enable,disable}() from rkisp1_pipeline_stream_{enable,disable}() for better unwind handling and function name semantics. * Call media_pipeline_{start,stop}() in the right order. * Remove item from TODO list (I also reviewed the use of the is_streaming var in the code and lgtm). This patch was tested on rockpi4 board with: ============================================ "media-ctl" "-d" "platform:rkisp1" "-r" "media-ctl" "-d" "platform:rkisp1" "-l" "'imx219 4-0010':0 -> 'rkisp1_isp':0 [1]" "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_selfpath':0 [1]" "media-ctl" "-d" "platform:rkisp1" "-l" "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]" "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"imx219 4-0010":0 [fmt:SRGGB10_1X10/1640x1232]' "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":0 [fmt:SRGGB10_1X10/1640x1232 crop: (0,0)/1600x1200]' "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_isp":2 [fmt:YUYV8_2X8/1600x1200 crop: (0,0)/1500x1100]' "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_selfpath":1 [fmt:YUYV8_2X8/900x800]' "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "width=900,height=800," "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "-v" "pixelformat=422P" "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":0 [fmt:YUYV8_2X8/1500x1100 crop: (300,400)/1400x1000]' "media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]' "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "width=900,height=800," "v4l2-ctl" "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "-v" "pixelformat=422P" sleep 1 time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_mainpath" "--stream-mmap" "--stream-count" "100" & time v4l2-ctl "-z" "platform:rkisp1" "-d" "rkisp1_selfpath" "--stream-mmap" "--stream-count" "100" & wait echo "Completed" Thanks Helen --- drivers/staging/media/rkisp1/TODO | 3 - drivers/staging/media/rkisp1/rkisp1-capture.c | 227 +++++++++--------- 2 files changed, 113 insertions(+), 117 deletions(-)