Hi folks, The omap3isp driver controlled the streaming state of all subdevs in the pipeline, including those further up in the pipeline. Should there be more than two subdevs (or drivers) there, this would cause s_stream op to be called multiple times (and possibly at the wrong time). Fix it. Sakari Ailus (2): omap3isp: Set device on omap3isp subdevs omap3isp: Don't set streaming state on random subdevs drivers/media/platform/omap3isp/isp.c | 8 ++++++++ drivers/media/platform/omap3isp/ispccdc.c | 1 + drivers/media/platform/omap3isp/ispccp2.c | 1 + drivers/media/platform/omap3isp/ispcsi2.c | 1 + drivers/media/platform/omap3isp/isppreview.c | 1 + drivers/media/platform/omap3isp/ispresizer.c | 1 + drivers/media/platform/omap3isp/ispstat.c | 2 ++ 7 files changed, 15 insertions(+) -- 2.20.1
The omap3isp driver registered subdevs without the dev field being set. Do that now. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/platform/omap3isp/ispccdc.c | 1 + drivers/media/platform/omap3isp/ispccp2.c | 1 + drivers/media/platform/omap3isp/ispcsi2.c | 1 + drivers/media/platform/omap3isp/isppreview.c | 1 + drivers/media/platform/omap3isp/ispresizer.c | 1 + drivers/media/platform/omap3isp/ispstat.c | 2 ++ 6 files changed, 7 insertions(+) diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c index 261ad1175f98..9297281402f5 100644 --- a/drivers/media/platform/omap3isp/ispccdc.c +++ b/drivers/media/platform/omap3isp/ispccdc.c @@ -2605,6 +2605,7 @@ int omap3isp_ccdc_register_entities(struct isp_ccdc_device *ccdc, int ret; /* Register the subdev and video node. */ + ccdc->subdev.dev = vdev->mdev->dev; ret = v4l2_device_register_subdev(vdev, &ccdc->subdev); if (ret < 0) goto error; diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c index 2dea423ffc0e..3cf9a32c1d9e 100644 --- a/drivers/media/platform/omap3isp/ispccp2.c +++ b/drivers/media/platform/omap3isp/ispccp2.c @@ -1034,6 +1034,7 @@ int omap3isp_ccp2_register_entities(struct isp_ccp2_device *ccp2, int ret; /* Register the subdev and video nodes. */ + ccp2->subdev.dev = vdev->mdev->dev; ret = v4l2_device_register_subdev(vdev, &ccp2->subdev); if (ret < 0) goto error; diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c index da66ea65be5d..9977702d57ff 100644 --- a/drivers/media/platform/omap3isp/ispcsi2.c +++ b/drivers/media/platform/omap3isp/ispcsi2.c @@ -1201,6 +1201,7 @@ int omap3isp_csi2_register_entities(struct isp_csi2_device *csi2, int ret; /* Register the subdev and video nodes. */ + csi2->subdev.dev = vdev->mdev->dev; ret = v4l2_device_register_subdev(vdev, &csi2->subdev); if (ret < 0) goto error; diff --git a/drivers/media/platform/omap3isp/isppreview.c b/drivers/media/platform/omap3isp/isppreview.c index 6ea6aeafd751..94fc9cf01436 100644 --- a/drivers/media/platform/omap3isp/isppreview.c +++ b/drivers/media/platform/omap3isp/isppreview.c @@ -2228,6 +2228,7 @@ int omap3isp_preview_register_entities(struct isp_prev_device *prev, int ret; /* Register the subdev and video nodes. */ + prev->subdev.dev = vdev->mdev->dev; ret = v4l2_device_register_subdev(vdev, &prev->subdev); if (ret < 0) goto error; diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c index b281cae036b3..d5cce8b8af97 100644 --- a/drivers/media/platform/omap3isp/ispresizer.c +++ b/drivers/media/platform/omap3isp/ispresizer.c @@ -1684,6 +1684,7 @@ int omap3isp_resizer_register_entities(struct isp_res_device *res, int ret; /* Register the subdev and video nodes. */ + res->subdev.dev = vdev->mdev->dev; ret = v4l2_device_register_subdev(vdev, &res->subdev); if (ret < 0) goto error; diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index 46a42c5dc1cc..bb1578d6eaf6 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -1029,6 +1029,8 @@ void omap3isp_stat_unregister_entities(struct ispstat *stat) int omap3isp_stat_register_entities(struct ispstat *stat, struct v4l2_device *vdev) { + stat->subdev.dev = vdev->mdev->dev; + return v4l2_device_register_subdev(vdev, &stat->subdev); } -- 2.20.1
The streaming state should be set to the first upstream sub-device only, not everywhere, for a sub-device driver itself knows how to best control the streaming state of its own upstream sub-devices. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/platform/omap3isp/isp.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index 008933beebe0..52533cdafb47 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -722,6 +722,10 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe, s_stream, mode); pipe->do_propagation = true; } + + /* Stop at the first external sub-device. */ + if (subdev->dev != isp->dev) + break; } return 0; @@ -836,6 +840,10 @@ static int isp_pipeline_disable(struct isp_pipeline *pipe) &subdev->entity); failure = -ETIMEDOUT; } + + /* Stop at the first external sub-device. */ + if (subdev->dev != isp->dev) + break; } return failure; -- 2.20.1
Hi Sakari, Thank you for the patch. On Mon, Aug 12, 2019 at 11:32:26AM +0300, Sakari Ailus wrote: > The omap3isp driver registered subdevs without the dev field being set. Do > that now. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> The change looks fine, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> What are the implications of not setting the dev field ? > --- > drivers/media/platform/omap3isp/ispccdc.c | 1 + > drivers/media/platform/omap3isp/ispccp2.c | 1 + > drivers/media/platform/omap3isp/ispcsi2.c | 1 + > drivers/media/platform/omap3isp/isppreview.c | 1 + > drivers/media/platform/omap3isp/ispresizer.c | 1 + > drivers/media/platform/omap3isp/ispstat.c | 2 ++ > 6 files changed, 7 insertions(+) > > diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c > index 261ad1175f98..9297281402f5 100644 > --- a/drivers/media/platform/omap3isp/ispccdc.c > +++ b/drivers/media/platform/omap3isp/ispccdc.c > @@ -2605,6 +2605,7 @@ int omap3isp_ccdc_register_entities(struct isp_ccdc_device *ccdc, > int ret; > > /* Register the subdev and video node. */ > + ccdc->subdev.dev = vdev->mdev->dev; > ret = v4l2_device_register_subdev(vdev, &ccdc->subdev); > if (ret < 0) > goto error; > diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c > index 2dea423ffc0e..3cf9a32c1d9e 100644 > --- a/drivers/media/platform/omap3isp/ispccp2.c > +++ b/drivers/media/platform/omap3isp/ispccp2.c > @@ -1034,6 +1034,7 @@ int omap3isp_ccp2_register_entities(struct isp_ccp2_device *ccp2, > int ret; > > /* Register the subdev and video nodes. */ > + ccp2->subdev.dev = vdev->mdev->dev; > ret = v4l2_device_register_subdev(vdev, &ccp2->subdev); > if (ret < 0) > goto error; > diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c > index da66ea65be5d..9977702d57ff 100644 > --- a/drivers/media/platform/omap3isp/ispcsi2.c > +++ b/drivers/media/platform/omap3isp/ispcsi2.c > @@ -1201,6 +1201,7 @@ int omap3isp_csi2_register_entities(struct isp_csi2_device *csi2, > int ret; > > /* Register the subdev and video nodes. */ > + csi2->subdev.dev = vdev->mdev->dev; > ret = v4l2_device_register_subdev(vdev, &csi2->subdev); > if (ret < 0) > goto error; > diff --git a/drivers/media/platform/omap3isp/isppreview.c b/drivers/media/platform/omap3isp/isppreview.c > index 6ea6aeafd751..94fc9cf01436 100644 > --- a/drivers/media/platform/omap3isp/isppreview.c > +++ b/drivers/media/platform/omap3isp/isppreview.c > @@ -2228,6 +2228,7 @@ int omap3isp_preview_register_entities(struct isp_prev_device *prev, > int ret; > > /* Register the subdev and video nodes. */ > + prev->subdev.dev = vdev->mdev->dev; > ret = v4l2_device_register_subdev(vdev, &prev->subdev); > if (ret < 0) > goto error; > diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c > index b281cae036b3..d5cce8b8af97 100644 > --- a/drivers/media/platform/omap3isp/ispresizer.c > +++ b/drivers/media/platform/omap3isp/ispresizer.c > @@ -1684,6 +1684,7 @@ int omap3isp_resizer_register_entities(struct isp_res_device *res, > int ret; > > /* Register the subdev and video nodes. */ > + res->subdev.dev = vdev->mdev->dev; > ret = v4l2_device_register_subdev(vdev, &res->subdev); > if (ret < 0) > goto error; > diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c > index 46a42c5dc1cc..bb1578d6eaf6 100644 > --- a/drivers/media/platform/omap3isp/ispstat.c > +++ b/drivers/media/platform/omap3isp/ispstat.c > @@ -1029,6 +1029,8 @@ void omap3isp_stat_unregister_entities(struct ispstat *stat) > int omap3isp_stat_register_entities(struct ispstat *stat, > struct v4l2_device *vdev) > { > + stat->subdev.dev = vdev->mdev->dev; > + > return v4l2_device_register_subdev(vdev, &stat->subdev); > } > > -- > 2.20.1 > -- Regards, Laurent Pinchart
Hi Sakari, Thank you for the patch. On Mon, Aug 12, 2019 at 11:32:27AM +0300, Sakari Ailus wrote: > The streaming state should be set to the first upstream sub-device only, > not everywhere, for a sub-device driver itself knows how to best control > the streaming state of its own upstream sub-devices. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/omap3isp/isp.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c > index 008933beebe0..52533cdafb47 100644 > --- a/drivers/media/platform/omap3isp/isp.c > +++ b/drivers/media/platform/omap3isp/isp.c > @@ -722,6 +722,10 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe, > s_stream, mode); > pipe->do_propagation = true; > } > + > + /* Stop at the first external sub-device. */ > + if (subdev->dev != isp->dev) > + break; > } > > return 0; > @@ -836,6 +840,10 @@ static int isp_pipeline_disable(struct isp_pipeline *pipe) > &subdev->entity); > failure = -ETIMEDOUT; > } > + > + /* Stop at the first external sub-device. */ > + if (subdev->dev != isp->dev) > + break; > } > > return failure; -- Regards, Laurent Pinchart