Re: [RFC 1/4] omap3isp: Implement validate_pipeline
Hi Laurent,
Thanks for the review!!!
On Thu, Dec 15, 2011 at 11:18:53AM +0100, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thanks for the patch.
>
> On Thursday 15 December 2011 10:50:32 Sakari Ailus wrote:
> > Validate pipeline of any external entity connected to the ISP driver.
> > The validation of the pipeline for the part that involves links inside the
> > domain of another driver must be done by that very driver.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/video/omap3isp/ispvideo.c | 12
> > 1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/media/video/omap3isp/ispvideo.c
> > b/drivers/media/video/omap3isp/ispvideo.c index f229057..17bc03c 100644
> > --- a/drivers/media/video/omap3isp/ispvideo.c
> > +++ b/drivers/media/video/omap3isp/ispvideo.c
> > @@ -355,6 +355,18 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
> > return -EPIPE;
> >
> > + if (subdev->host_priv) {
> > + /*
> > +* host_priv != NULL: this is a sensor. Issue
> > +* validate_pipeline. We're at our end of the
> > +* pipeline so we quit now.
> > +*/
> > + ret = v4l2_subdev_call(subdev, pad, validate_pipeline);
> > + if (IS_ERR_VALUE(ret))
>
> Is the validate pipeline operation expected to return a value different than
> 0
> on success ? If not if (ret < 0) should do.
>
> Although there's another issue. Not all sensors will implement the
> validate_pipeline operation, so you shouldn't return an error if ret == -
> ENOIOCTLCMD.
>
> I will comment on the validate_pipeline approach itself in the "On
> controlling
> sensors" mail thread.
Good point. I'll fix this; same for your comment on the third patch.
--
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: [RFC 1/4] omap3isp: Implement validate_pipeline
Hi Sakari,
Thanks for the patch.
On Thursday 15 December 2011 10:50:32 Sakari Ailus wrote:
> Validate pipeline of any external entity connected to the ISP driver.
> The validation of the pipeline for the part that involves links inside the
> domain of another driver must be done by that very driver.
>
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/video/omap3isp/ispvideo.c | 12
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index f229057..17bc03c 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -355,6 +355,18 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
> return -EPIPE;
>
> + if (subdev->host_priv) {
> + /*
> + * host_priv != NULL: this is a sensor. Issue
> + * validate_pipeline. We're at our end of the
> + * pipeline so we quit now.
> + */
> + ret = v4l2_subdev_call(subdev, pad, validate_pipeline);
> + if (IS_ERR_VALUE(ret))
Is the validate pipeline operation expected to return a value different than 0
on success ? If not if (ret < 0) should do.
Although there's another issue. Not all sensors will implement the
validate_pipeline operation, so you shouldn't return an error if ret == -
ENOIOCTLCMD.
I will comment on the validate_pipeline approach itself in the "On controlling
sensors" mail thread.
> + return -EPIPE;
> + break;
> + }
> +
> if (shifter_link) {
> unsigned int parallel_shift = 0;
> if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
--
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
[RFC 1/4] omap3isp: Implement validate_pipeline
Validate pipeline of any external entity connected to the ISP driver.
The validation of the pipeline for the part that involves links inside the
domain of another driver must be done by that very driver.
Signed-off-by: Sakari Ailus
---
drivers/media/video/omap3isp/ispvideo.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispvideo.c
b/drivers/media/video/omap3isp/ispvideo.c
index f229057..17bc03c 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -355,6 +355,18 @@ static int isp_video_validate_pipeline(struct isp_pipeline
*pipe)
fmt_source.format.height != fmt_sink.format.height)
return -EPIPE;
+ if (subdev->host_priv) {
+ /*
+* host_priv != NULL: this is a sensor. Issue
+* validate_pipeline. We're at our end of the
+* pipeline so we quit now.
+*/
+ ret = v4l2_subdev_call(subdev, pad, validate_pipeline);
+ if (IS_ERR_VALUE(ret))
+ return -EPIPE;
+ break;
+ }
+
if (shifter_link) {
unsigned int parallel_shift = 0;
if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
--
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
