Hi Dafna,
On Fri, May 22, 2020 at 09:55:21AM +0200, Dafna Hirschfeld wrote:
> From: Helen Koike <helen.koike@xxxxxxxxxxxxx>
>
> Use v4l2_pipeline_stream_{enable,disable} to call .s_stream()
> subdevice callbacks through the pipeline.
> Those helpers are called only if the other capture is not streaming.
>
> If the other capture is streaming then he already did that for us
> so we call s_stream only on the resizer that is connected to the
> capture node.
>
> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> drivers/staging/media/rkisp1/rkisp1-capture.c | 104 ++++++------------
> 1 file changed, 32 insertions(+), 72 deletions(-)
>
Thank you for the patch. Please see my comments inline.
[snip]
> +static int rkisp1_s_stream_subdev(struct rkisp1_capture *cap, int enable)
> +{
> + struct rkisp1_device *rkisp1 = cap->rkisp1;
> + struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1];
> + int ret;
> +
> + /*
> + * if the other capture is already streaming then we only need to
> + * call s_stream of our reszier
> + */
> + if (other->is_streaming) {
> + struct v4l2_subdev *rsz_sd = &rkisp1->resizer_devs[cap->id].sd;
> +
> + ret = v4l2_subdev_call(rsz_sd, video, s_stream, enable);
> + if (ret && ret != -ENOIOCTLCMD)
> + dev_err(rkisp1->dev,
> + "stream %s resizer '%s' failed (%d)\n",
> + enable ? "on" : "off", rsz_sd->name, ret);
Do we need this special case? Wouldn't v4l2_pipeline_stream_*() simply
increment reference counters for the other entities?
> + } else {
> + if (enable)
> + ret = v4l2_pipeline_stream_enable(&cap->vnode.vdev);
> + else
> + ret = v4l2_pipeline_stream_disable(&cap->vnode.vdev);
I wonder if this doesn't ask for just making the helper
v4l2_pipeline_s_stream(..., int enable).
Best regards,
Tomasz
