omap3isp: Control streaming on directly connected subdevs only
cover omap3isp: Control streaming on directly connected subdevs only
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(+)
Signature · 1 line
2.20.1
PATCH 1/2 omap3isp: Set device on omap3isp subdevs
The omap3isp driver registered subdevs without the dev field being set. Do that now.
Signed-off-by Sakari Ailus <[email protected]>
--- 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(+)
drivers/media/platform/omap3isp/ispccdc.c +1 -0
@@ -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;
drivers/media/platform/omap3isp/ispccp2.c +1 -0
@@ -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;
drivers/media/platform/omap3isp/ispcsi2.c +1 -0
@@ -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;
drivers/media/platform/omap3isp/isppreview.c +1 -0
@@ -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;
drivers/media/platform/omap3isp/ispresizer.c +1 -0
@@ -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;
drivers/media/platform/omap3isp/ispstat.c +2 -0
@@ -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); }
Signature · 1 line
2.20.1
Re: [PATCH 1/2] omap3isp: Set device on omap3isp subdevs
Hi Sakari,
Thank you for the patch.
Quoted 4 lines · Sakari Ailus
The omap3isp driver registered subdevs without the dev field being set. Do that now.
Signed-off-by Sakari Ailus <[email protected]>
The change looks fine, so
Reviewed-by Laurent Pinchart <[email protected]>
What are the implications of not setting the dev field ?
Quoted 85 lines
--- 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(+)drivers/media/platform/omap3isp/ispccdc.c +1
-0@@ -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;drivers/media/platform/omap3isp/ispccp2.c +1
-0@@ -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;drivers/media/platform/omap3isp/ispcsi2.c +1
-0@@ -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;drivers/media/platform/omap3isp/isppreview.c +1
-0@@ -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;drivers/media/platform/omap3isp/ispresizer.c +1
-0@@ -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;drivers/media/platform/omap3isp/ispstat.c +2
-0@@ -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); }Signature · 1 line
2.20.1
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 2/2 omap3isp: Don't set streaming state on random subdevs
In reply to Sakari AilusThe 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 <[email protected]>
--- drivers/media/platform/omap3isp/isp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/media/platform/omap3isp/isp.c +8 -0
@@ -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;
Signature · 1 line
2.20.1
Re: [PATCH 2/2] omap3isp: Don't set streaming state on random subdevs
Hi Sakari,
Thank you for the patch.
Quoted 5 lines · Sakari Ailus
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 <[email protected]>
Reviewed-by Laurent Pinchart <[email protected]>
Quoted 30 lines
--- drivers/media/platform/omap3isp/isp.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/media/platform/omap3isp/isp.c +8
-0@@ -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;
Signature · 3 lines
Regards, Laurent Pinchart