* [PATCH 0/2] omap3isp: Control streaming on directly connected subdevs only @ 2019-08-12 8:32 Sakari Ailus 2019-08-12 8:32 ` [PATCH 1/2] omap3isp: Set device on omap3isp subdevs Sakari Ailus 2019-08-12 8:32 ` [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs Sakari Ailus 0 siblings, 2 replies; 5+ messages in thread From: Sakari Ailus @ 2019-08-12 8:32 UTC (permalink / raw) To: linux-media; +Cc: laurent.pinchart 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] omap3isp: Set device on omap3isp subdevs 2019-08-12 8:32 [PATCH 0/2] omap3isp: Control streaming on directly connected subdevs only Sakari Ailus @ 2019-08-12 8:32 ` Sakari Ailus 2019-08-12 22:39 ` Laurent Pinchart 2019-08-12 8:32 ` [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs Sakari Ailus 1 sibling, 1 reply; 5+ messages in thread From: Sakari Ailus @ 2019-08-12 8:32 UTC (permalink / raw) To: linux-media; +Cc: laurent.pinchart 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] omap3isp: Set device on omap3isp subdevs 2019-08-12 8:32 ` [PATCH 1/2] omap3isp: Set device on omap3isp subdevs Sakari Ailus @ 2019-08-12 22:39 ` Laurent Pinchart 0 siblings, 0 replies; 5+ messages in thread From: Laurent Pinchart @ 2019-08-12 22:39 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs 2019-08-12 8:32 [PATCH 0/2] omap3isp: Control streaming on directly connected subdevs only Sakari Ailus 2019-08-12 8:32 ` [PATCH 1/2] omap3isp: Set device on omap3isp subdevs Sakari Ailus @ 2019-08-12 8:32 ` Sakari Ailus 2019-08-12 22:40 ` Laurent Pinchart 1 sibling, 1 reply; 5+ messages in thread From: Sakari Ailus @ 2019-08-12 8:32 UTC (permalink / raw) To: linux-media; +Cc: laurent.pinchart 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs 2019-08-12 8:32 ` [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs Sakari Ailus @ 2019-08-12 22:40 ` Laurent Pinchart 0 siblings, 0 replies; 5+ messages in thread From: Laurent Pinchart @ 2019-08-12 22:40 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-12 22:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-12 8:32 [PATCH 0/2] omap3isp: Control streaming on directly connected subdevs only Sakari Ailus 2019-08-12 8:32 ` [PATCH 1/2] omap3isp: Set device on omap3isp subdevs Sakari Ailus 2019-08-12 22:39 ` Laurent Pinchart 2019-08-12 8:32 ` [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs Sakari Ailus 2019-08-12 22:40 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).