Re: [PATCH v5 27/35] omap3isp: Introduce isp_video_check_external_subdevs()
Hi Sakari,
Thanks for the patch.
On Tuesday 06 March 2012 18:33:08 Sakari Ailus wrote:
isp_video_check_external_subdevs() will retrieve external subdev's
bits-per-pixel and pixel rate for the use of other ISP subdevs at streamon
time. isp_video_check_external_subdevs() is called after pipeline
validation.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/ispvideo.c | 75
1 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispvideo.c
b/drivers/media/video/omap3isp/ispvideo.c index 4bc9cca..ef5c770 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -934,6 +934,77 @@ isp_video_dqbuf(struct file *file, void *fh, struct
v4l2_buffer *b) file-f_flags O_NONBLOCK);
}
+static int isp_video_check_external_subdevs(struct isp_pipeline *pipe)
+{
+ struct isp_device *isp =
+ container_of(pipe, struct isp_video, pipe)-isp;
Any reason not to pass isp_device * from the caller to this function ?
+ struct media_entity *ents[] = {
+ isp-isp_csi2a.subdev.entity,
+ isp-isp_csi2c.subdev.entity,
+ isp-isp_ccp2.subdev.entity,
+ isp-isp_ccdc.subdev.entity
+ };
+ struct media_pad *source_pad;
+ struct media_entity *source = NULL;
+ struct media_entity *sink;
+ struct v4l2_subdev_format fmt;
+ struct v4l2_ext_controls ctrls;
+ struct v4l2_ext_control ctrl;
+ int i;
i is allowed to be unsigned in this driver as well ;-)
+ int ret = 0;
+
+ for (i = 0; i ARRAY_SIZE(ents); i++) {
+ /* Is the entity part of the pipeline? */
+ if (!(pipe-entities (1 ents[i]-id)))
+ continue;
+
+ /* ISP entities have always sink pad == 0. Find source. */
+ source_pad = media_entity_remote_source(ents[i]-pads[0]);
+
Don't you usually avoid blank lines between a variable assignment and checking
it for an error condition ?
+ if (source_pad == NULL)
+ continue;
+
+ source = source_pad-entity;
+ sink = ents[i];
+ break;
+ }
+
+ if (!source || media_entity_type(source) != MEDIA_ENT_T_V4L2_SUBDEV)
+ return 0;
+
+ pipe-external = media_entity_to_v4l2_subdev(source);
+
+ fmt.pad = source_pad-index;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
+pad, get_fmt, NULL, fmt);
+ BUG_ON(ret 0);
That's a bit harsh. I'd rather return an error.
+
+ pipe-external_bpp = omap3isp_video_format_info(
+ fmt.format.code)-bpp;
Maybe you could teachs emacs that 78 characters fit in a 80-columns line ? :-)
+
+ memset(ctrls, 0, sizeof(ctrls));
+ memset(ctrl, 0, sizeof(ctrl));
+
+ ctrl.id = V4L2_CID_PIXEL_RATE;
+
+ ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
You can leave ctrl_class to 0.
+ ctrls.count = 1;
+ ctrls.controls = ctrl;
+
+ ret = v4l2_g_ext_ctrls(pipe-external-ctrl_handler, ctrls);
+ if (ret 0) {
+ dev_warn(isp-dev,
+ no pixel rate control in subdev %s\n,
No need to split this either.
+ pipe-external-name);
+ return ret;
+ }
+
+ pipe-external_rate = ctrl.value64;
+
+ return 0;
+}
+
/*
* Stream management
*
@@ -1010,6 +1081,10 @@ isp_video_streamon(struct file *file, void *fh, enum
v4l2_buf_type type) while ((entity = media_entity_graph_walk_next(graph)))
pipe-entities |= 1 entity-id;
+ ret = isp_video_check_external_subdevs(pipe);
+ if (ret 0)
+ goto err_check_format;
+
/* Verify that the currently configured format matches the output of
* the connected subdev.
*/
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 27/35] omap3isp: Introduce isp_video_check_external_subdevs()
Hi Laurent,
Thanks for the comments!
On Wed, Mar 07, 2012 at 11:43:31AM +0100, Laurent Pinchart wrote:
On Tuesday 06 March 2012 18:33:08 Sakari Ailus wrote:
isp_video_check_external_subdevs() will retrieve external subdev's
bits-per-pixel and pixel rate for the use of other ISP subdevs at streamon
time. isp_video_check_external_subdevs() is called after pipeline
validation.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/ispvideo.c | 75
1 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispvideo.c
b/drivers/media/video/omap3isp/ispvideo.c index 4bc9cca..ef5c770 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -934,6 +934,77 @@ isp_video_dqbuf(struct file *file, void *fh, struct
v4l2_buffer *b) file-f_flags O_NONBLOCK);
}
+static int isp_video_check_external_subdevs(struct isp_pipeline *pipe)
+{
+ struct isp_device *isp =
+ container_of(pipe, struct isp_video, pipe)-isp;
Any reason not to pass isp_device * from the caller to this function ?
I didn't simply because it was unnecessary. Should I? pipe is needed in
any case.
+ struct media_entity *ents[] = {
+ isp-isp_csi2a.subdev.entity,
+ isp-isp_csi2c.subdev.entity,
+ isp-isp_ccp2.subdev.entity,
+ isp-isp_ccdc.subdev.entity
+ };
+ struct media_pad *source_pad;
+ struct media_entity *source = NULL;
+ struct media_entity *sink;
+ struct v4l2_subdev_format fmt;
+ struct v4l2_ext_controls ctrls;
+ struct v4l2_ext_control ctrl;
+ int i;
i is allowed to be unsigned in this driver as well ;-)
unsigned... we meet again!
+ int ret = 0;
+
+ for (i = 0; i ARRAY_SIZE(ents); i++) {
+ /* Is the entity part of the pipeline? */
+ if (!(pipe-entities (1 ents[i]-id)))
+ continue;
+
+ /* ISP entities have always sink pad == 0. Find source. */
+ source_pad = media_entity_remote_source(ents[i]-pads[0]);
+
Don't you usually avoid blank lines between a variable assignment and
checking
it for an error condition ?
We do. Fixed.
+ if (source_pad == NULL)
+ continue;
+
+ source = source_pad-entity;
+ sink = ents[i];
+ break;
+ }
+
+ if (!source || media_entity_type(source) != MEDIA_ENT_T_V4L2_SUBDEV)
+ return 0;
+
+ pipe-external = media_entity_to_v4l2_subdev(source);
+
+ fmt.pad = source_pad-index;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
+ pad, get_fmt, NULL, fmt);
+ BUG_ON(ret 0);
That's a bit harsh. I'd rather return an error.
This is a driver BUG(). At the very least it's WARN() and return an error...
I think I'll do dev_warn(message) and return the error.
I actually add the same for the case where source is NULL --- that's also a
driver bug.
+
+ pipe-external_bpp = omap3isp_video_format_info(
+ fmt.format.code)-bpp;
Maybe you could teachs emacs that 78 characters fit in a 80-columns line ? :-)
That's 79, not 78. And I need to scroll to the end of the line you wrote to
see it after it has been prefixed with . :-D
I think I've been renaming things and as a result it did fit on a single
line.
+
+ memset(ctrls, 0, sizeof(ctrls));
+ memset(ctrl, 0, sizeof(ctrl));
+
+ ctrl.id = V4L2_CID_PIXEL_RATE;
+
+ ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
You can leave ctrl_class to 0.
Fixed.
+ ctrls.count = 1;
+ ctrls.controls = ctrl;
+
+ ret = v4l2_g_ext_ctrls(pipe-external-ctrl_handler, ctrls);
+ if (ret 0) {
+ dev_warn(isp-dev,
+no pixel rate control in subdev %s\n,
No need to split this either.
Fixed.
Kind regards,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 27/35] omap3isp: Introduce isp_video_check_external_subdevs()
Hi Sakari,
On Wednesday 07 March 2012 19:49:46 Sakari Ailus wrote:
On Wed, Mar 07, 2012 at 11:43:31AM +0100, Laurent Pinchart wrote:
On Tuesday 06 March 2012 18:33:08 Sakari Ailus wrote:
isp_video_check_external_subdevs() will retrieve external subdev's
bits-per-pixel and pixel rate for the use of other ISP subdevs at
streamon
time. isp_video_check_external_subdevs() is called after pipeline
validation.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/ispvideo.c | 75
1 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispvideo.c
b/drivers/media/video/omap3isp/ispvideo.c index 4bc9cca..ef5c770 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -934,6 +934,77 @@ isp_video_dqbuf(struct file *file, void *fh, struct
v4l2_buffer *b) file-f_flags O_NONBLOCK);
}
+static int isp_video_check_external_subdevs(struct isp_pipeline *pipe)
+{
+ struct isp_device *isp =
+ container_of(pipe, struct isp_video, pipe)-isp;
Any reason not to pass isp_device * from the caller to this function ?
I didn't simply because it was unnecessary. Should I? pipe is needed in
any case.
It will look simpler (in my opinion), and will probably generate less code, so
I think you should.
+ struct media_entity *ents[] = {
+ isp-isp_csi2a.subdev.entity,
+ isp-isp_csi2c.subdev.entity,
+ isp-isp_ccp2.subdev.entity,
+ isp-isp_ccdc.subdev.entity
+ };
+ struct media_pad *source_pad;
+ struct media_entity *source = NULL;
+ struct media_entity *sink;
+ struct v4l2_subdev_format fmt;
+ struct v4l2_ext_controls ctrls;
+ struct v4l2_ext_control ctrl;
+ int i;
i is allowed to be unsigned in this driver as well ;-)
unsigned... we meet again!
+ int ret = 0;
+
+ for (i = 0; i ARRAY_SIZE(ents); i++) {
+ /* Is the entity part of the pipeline? */
+ if (!(pipe-entities (1 ents[i]-id)))
+ continue;
+
+ /* ISP entities have always sink pad == 0. Find source. */
+ source_pad = media_entity_remote_source(ents[i]-pads[0]);
+
Don't you usually avoid blank lines between a variable assignment and
checking it for an error condition ?
We do. Fixed.
+ if (source_pad == NULL)
+ continue;
+
+ source = source_pad-entity;
+ sink = ents[i];
+ break;
+ }
+
+ if (!source || media_entity_type(source) != MEDIA_ENT_T_V4L2_SUBDEV)
+ return 0;
+
+ pipe-external = media_entity_to_v4l2_subdev(source);
+
+ fmt.pad = source_pad-index;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
+pad, get_fmt, NULL, fmt);
+ BUG_ON(ret 0);
That's a bit harsh. I'd rather return an error.
This is a driver BUG(). At the very least it's WARN() and return an error...
I think I'll do dev_warn(message) and return the error.
dev_warn + return error sounds good.
I actually add the same for the case where source is NULL --- that's also a
driver bug.
+
+ pipe-external_bpp = omap3isp_video_format_info(
+ fmt.format.code)-bpp;
Maybe you could teachs emacs that 78 characters fit in a 80-columns line ?
:-)
That's 79, not 78. And I need to scroll to the end of the line you wrote to
see it after it has been prefixed with . :-D
I think I've been renaming things and as a result it did fit on a single
line.
+
+ memset(ctrls, 0, sizeof(ctrls));
+ memset(ctrl, 0, sizeof(ctrl));
+
+ ctrl.id = V4L2_CID_PIXEL_RATE;
+
+ ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
You can leave ctrl_class to 0.
Fixed.
+ ctrls.count = 1;
+ ctrls.controls = ctrl;
+
+ ret = v4l2_g_ext_ctrls(pipe-external-ctrl_handler, ctrls);
+ if (ret 0) {
+ dev_warn(isp-dev,
+ no pixel rate control in subdev %s\n,
No need to split this either.
Fixed.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 27/35] omap3isp: Introduce isp_video_check_external_subdevs()
Hi Laurent,
Laurent Pinchart wrote:
On Wednesday 07 March 2012 19:49:46 Sakari Ailus wrote:
On Wed, Mar 07, 2012 at 11:43:31AM +0100, Laurent Pinchart wrote:
On Tuesday 06 March 2012 18:33:08 Sakari Ailus wrote:
isp_video_check_external_subdevs() will retrieve external subdev's
bits-per-pixel and pixel rate for the use of other ISP subdevs at
streamon
time. isp_video_check_external_subdevs() is called after pipeline
validation.
Signed-off-by: Sakari Ailussakari.ai...@iki.fi
---
drivers/media/video/omap3isp/ispvideo.c | 75
1 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispvideo.c
b/drivers/media/video/omap3isp/ispvideo.c index 4bc9cca..ef5c770 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -934,6 +934,77 @@ isp_video_dqbuf(struct file *file, void *fh, struct
v4l2_buffer *b) file-f_flags O_NONBLOCK);
}
+static int isp_video_check_external_subdevs(struct isp_pipeline *pipe)
+{
+ struct isp_device *isp =
+ container_of(pipe, struct isp_video, pipe)-isp;
Any reason not to pass isp_device * from the caller to this function ?
I didn't simply because it was unnecessary. Should I? pipe is needed in
any case.
It will look simpler (in my opinion), and will probably generate less code, so
I think you should.
I'll change that for a new version tomorrow.
--
Sakari Ailus
sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 27/35] omap3isp: Introduce isp_video_check_external_subdevs()
isp_video_check_external_subdevs() will retrieve external subdev's
bits-per-pixel and pixel rate for the use of other ISP subdevs at streamon
time. isp_video_check_external_subdevs() is called after pipeline
validation.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/ispvideo.c | 75 +++
1 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispvideo.c
b/drivers/media/video/omap3isp/ispvideo.c
index 4bc9cca..ef5c770 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -934,6 +934,77 @@ isp_video_dqbuf(struct file *file, void *fh, struct
v4l2_buffer *b)
file-f_flags O_NONBLOCK);
}
+static int isp_video_check_external_subdevs(struct isp_pipeline *pipe)
+{
+ struct isp_device *isp =
+ container_of(pipe, struct isp_video, pipe)-isp;
+ struct media_entity *ents[] = {
+ isp-isp_csi2a.subdev.entity,
+ isp-isp_csi2c.subdev.entity,
+ isp-isp_ccp2.subdev.entity,
+ isp-isp_ccdc.subdev.entity
+ };
+ struct media_pad *source_pad;
+ struct media_entity *source = NULL;
+ struct media_entity *sink;
+ struct v4l2_subdev_format fmt;
+ struct v4l2_ext_controls ctrls;
+ struct v4l2_ext_control ctrl;
+ int i;
+ int ret = 0;
+
+ for (i = 0; i ARRAY_SIZE(ents); i++) {
+ /* Is the entity part of the pipeline? */
+ if (!(pipe-entities (1 ents[i]-id)))
+ continue;
+
+ /* ISP entities have always sink pad == 0. Find source. */
+ source_pad = media_entity_remote_source(ents[i]-pads[0]);
+
+ if (source_pad == NULL)
+ continue;
+
+ source = source_pad-entity;
+ sink = ents[i];
+ break;
+ }
+
+ if (!source || media_entity_type(source) != MEDIA_ENT_T_V4L2_SUBDEV)
+ return 0;
+
+ pipe-external = media_entity_to_v4l2_subdev(source);
+
+ fmt.pad = source_pad-index;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
+ pad, get_fmt, NULL, fmt);
+ BUG_ON(ret 0);
+
+ pipe-external_bpp = omap3isp_video_format_info(
+ fmt.format.code)-bpp;
+
+ memset(ctrls, 0, sizeof(ctrls));
+ memset(ctrl, 0, sizeof(ctrl));
+
+ ctrl.id = V4L2_CID_PIXEL_RATE;
+
+ ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+ ctrls.count = 1;
+ ctrls.controls = ctrl;
+
+ ret = v4l2_g_ext_ctrls(pipe-external-ctrl_handler, ctrls);
+ if (ret 0) {
+ dev_warn(isp-dev,
+no pixel rate control in subdev %s\n,
+pipe-external-name);
+ return ret;
+ }
+
+ pipe-external_rate = ctrl.value64;
+
+ return 0;
+}
+
/*
* Stream management
*
@@ -1010,6 +1081,10 @@ isp_video_streamon(struct file *file, void *fh, enum
v4l2_buf_type type)
while ((entity = media_entity_graph_walk_next(graph)))
pipe-entities |= 1 entity-id;
+ ret = isp_video_check_external_subdevs(pipe);
+ if (ret 0)
+ goto err_check_format;
+
/* Verify that the currently configured format matches the output of
* the connected subdev.
*/
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
