| Message ID | Pine.LNX.4.64.0906101604420.4817@axis700.grange (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hello Guennadi, It's a very interesting patch. Actually some camera interfaces support for various image effects and I was wondering how to use them in SoC camera subsystem. But here is a question. Is it possible to make a choice with the same CID between icd and ici? I mean, if both of camera interface and camera device are supporting for same CID how can user select any of them to use? Sometimes, some image effects supported by camera interface are not good so I want to use the same effect supported by external camera ISP device. I think, it might be possible but I can't see how. Cheers, Nate On Thu, Jun 11, 2009 at 4:12 PM, Guennadi Liakhovetski<g.liakhovetski@gmx.de> wrote: > Until now soc-camera only supported client (sensor) controls. This patch > enables camera-host drivers to implement their own controls too. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- >  drivers/media/video/soc_camera.c |  24 ++++++++++++++++++++++++ >  include/media/soc_camera.h    |   4 ++++ >  2 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c > index 824c68b..8e987ca 100644 > --- a/drivers/media/video/soc_camera.c > +++ b/drivers/media/video/soc_camera.c > @@ -633,6 +633,7 @@ static int soc_camera_queryctrl(struct file *file, void *priv, >  { >     struct soc_camera_file *icf = file->private_data; >     struct soc_camera_device *icd = icf->icd; > +    struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); >     int i; > >     WARN_ON(priv != file->private_data); > @@ -640,6 +641,15 @@ static int soc_camera_queryctrl(struct file *file, void *priv, >     if (!qc->id) >         return -EINVAL; > > +    /* First check host controls */ > +    for (i = 0; i < ici->ops->num_controls; i++) > +        if (qc->id == ici->ops->controls[i].id) { > +            memcpy(qc, &(ici->ops->controls[i]), > +                sizeof(*qc)); > +            return 0; > +        } > + > +    /* Then device controls */ >     for (i = 0; i < icd->ops->num_controls; i++) >         if (qc->id == icd->ops->controls[i].id) { >             memcpy(qc, &(icd->ops->controls[i]), > @@ -656,6 +666,7 @@ static int soc_camera_g_ctrl(struct file *file, void *priv, >     struct soc_camera_file *icf = file->private_data; >     struct soc_camera_device *icd = icf->icd; >     struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); > +    int ret; > >     WARN_ON(priv != file->private_data); > > @@ -672,6 +683,12 @@ static int soc_camera_g_ctrl(struct file *file, void *priv, >         return 0; >     } > > +    if (ici->ops->get_ctrl) { > +        ret = ici->ops->get_ctrl(icd, ctrl); > +        if (ret != -ENOIOCTLCMD) > +            return ret; > +    } > + >     return v4l2_device_call_until_err(&ici->v4l2_dev, (__u32)icd, core, g_ctrl, ctrl); >  } > > @@ -681,9 +698,16 @@ static int soc_camera_s_ctrl(struct file *file, void *priv, >     struct soc_camera_file *icf = file->private_data; >     struct soc_camera_device *icd = icf->icd; >     struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); > +    int ret; > >     WARN_ON(priv != file->private_data); > > +    if (ici->ops->set_ctrl) { > +        ret = ici->ops->set_ctrl(icd, ctrl); > +        if (ret != -ENOIOCTLCMD) > +            return ret; > +    } > + >     return v4l2_device_call_until_err(&ici->v4l2_dev, (__u32)icd, core, s_ctrl, ctrl); >  } > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > index 3bc5b6b..2d116bb 100644 > --- a/include/media/soc_camera.h > +++ b/include/media/soc_camera.h > @@ -83,7 +83,11 @@ struct soc_camera_host_ops { >     int (*reqbufs)(struct soc_camera_file *, struct v4l2_requestbuffers *); >     int (*querycap)(struct soc_camera_host *, struct v4l2_capability *); >     int (*set_bus_param)(struct soc_camera_device *, __u32); > +    int (*get_ctrl)(struct soc_camera_device *, struct v4l2_control *); > +    int (*set_ctrl)(struct soc_camera_device *, struct v4l2_control *); >     unsigned int (*poll)(struct file *, poll_table *); > +    const struct v4l2_queryctrl *controls; > +    int num_controls; >  }; > >  #define SOCAM_SENSOR_INVERT_PCLK    (1 << 0) > -- > 1.6.2.4 > >
On Thu, 11 Jun 2009, Dongsoo, Nathaniel Kim wrote: > Hello Guennadi, > > It's a very interesting patch. Actually some camera interfaces support > for various image effects and I was wondering how to use them in SoC > camera subsystem. > > But here is a question. Is it possible to make a choice with the same > CID between icd and ici? I mean, if both of camera interface and > camera device are supporting for same CID how can user select any of > them to use? Sometimes, some image effects supported by camera > interface are not good so I want to use the same effect supported by > external camera ISP device. > > I think, it might be possible but I can't see how. > > @@ -681,9 +698,16 @@ static int soc_camera_s_ctrl(struct file *file, void *priv, > > Â Â Â Â struct soc_camera_file *icf = file->private_data; > > Â Â Â Â struct soc_camera_device *icd = icf->icd; > > Â Â Â Â struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); > > + Â Â Â int ret; > > > > Â Â Â Â WARN_ON(priv != file->private_data); > > > > + Â Â Â if (ici->ops->set_ctrl) { > > + Â Â Â Â Â Â Â ret = ici->ops->set_ctrl(icd, ctrl); > > + Â Â Â Â Â Â Â if (ret != -ENOIOCTLCMD) > > + Â Â Â Â Â Â Â Â Â Â Â return ret; > > + Â Â Â } > > + > > Â Â Â Â return v4l2_device_call_until_err(&ici->v4l2_dev, (__u32)icd, core, s_ctrl, ctrl); > > Â } Should be easy to see in the patch. Host's s_ctrl is called first. It can return -ENOIOCTLCMD then sensor's control will be called too. Ot the host may choose to call sensor's control itself, which, however, is discouraged. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Guennadi, So let's assume that camera interface device can process V4L2_CID_SHARPNESS and even external camera device can process that, then according to your patch both of camera interface and external camera device can be issued to process V4L2_CID_SHARPNESS which I guess will make image sharpened twice. Am I getting the patch right? If I'm getting right, it might be better to give user make a choice through platform data or some sort of variable which can make a choice between camera interface and camera device to process the CID. It could be just in aspect of manufacturer mind, we do love to make a choice between same features in different devices in easy way. So never mind if my idea is not helpful making your driver elegant :-) Cheers, Nate On Thu, Jun 11, 2009 at 9:16 PM, Guennadi Liakhovetski<g.liakhovetski@gmx.de> wrote: > On Thu, 11 Jun 2009, Dongsoo, Nathaniel Kim wrote: > >> Hello Guennadi, >> >> It's a very interesting patch. Actually some camera interfaces support >> for various image effects and I was wondering how to use them in SoC >> camera subsystem. >> >> But here is a question. Is it possible to make a choice with the same >> CID between icd and ici? I mean, if both of camera interface and >> camera device are supporting for same CID how can user select any of >> them to use? Sometimes, some image effects supported by camera >> interface are not good so I want to use the same effect supported by >> external camera ISP device. >> >> I think, it might be possible but I can't see how. > >> > @@ -681,9 +698,16 @@ static int soc_camera_s_ctrl(struct file *file, void *priv, >> > Â Â Â Â struct soc_camera_file *icf = file->private_data; >> > Â Â Â Â struct soc_camera_device *icd = icf->icd; >> > Â Â Â Â struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); >> > + Â Â Â int ret; >> > >> > Â Â Â Â WARN_ON(priv != file->private_data); >> > >> > + Â Â Â if (ici->ops->set_ctrl) { >> > + Â Â Â Â Â Â Â ret = ici->ops->set_ctrl(icd, ctrl); >> > + Â Â Â Â Â Â Â if (ret != -ENOIOCTLCMD) >> > + Â Â Â Â Â Â Â Â Â Â Â return ret; >> > + Â Â Â } >> > + >> > Â Â Â Â return v4l2_device_call_until_err(&ici->v4l2_dev, (__u32)icd, core, s_ctrl, ctrl); >> > Â } > > Should be easy to see in the patch. Host's s_ctrl is called first. It can > return -ENOIOCTLCMD then sensor's control will be called too. Ot the host > may choose to call sensor's control itself, which, however, is > discouraged. > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ >
On Fri, 12 Jun 2009, Dongsoo, Nathaniel Kim wrote: > Hello Guennadi, > > So let's assume that camera interface device can process > V4L2_CID_SHARPNESS and even external camera device can process that, > then according to your patch both of camera interface and external > camera device can be issued to process V4L2_CID_SHARPNESS which I > guess will make image sharpened twice. Am I getting the patch right? Please, do not top-post! I am sorry, is it really so difficult to understand > >> > + Â Â Â Â Â Â Â ret = ici->ops->set_ctrl(icd, ctrl); > >> > + Â Â Â Â Â Â Â if (ret != -ENOIOCTLCMD) > >> > + Â Â Â Â Â Â Â Â Â Â Â return ret; which means just one thing: the camera host (interface if you like) driver decides, whether it wants client's control to be called, in which case it has to return -ENOIOCTLCMD, or it returns any other code (0 or a negative error code), then the client will not be called. > If I'm getting right, it might be better to give user make a choice > through platform data or some sort of variable which can make a choice > between camera interface and camera device to process the CID. It > could be just in aspect of manufacturer mind, we do love to make a > choice between same features in different devices in easy way. So > never mind if my idea is not helpful making your driver elegant :-) So far it seems too much to me. Let's wait until we get a case where it really makes sense for platform code to decide who processes certain controls. I think giving the host driver the power to decide should be ok for now. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 12, 2009 at 3:30 PM, Guennadi Liakhovetski<g.liakhovetski@gmx.de> wrote: > On Fri, 12 Jun 2009, Dongsoo, Nathaniel Kim wrote: > >> Hello Guennadi, >> >> So let's assume that camera interface device can process >> V4L2_CID_SHARPNESS and even external camera device can process that, >> then according to your patch both of camera interface and external >> camera device can be issued to process V4L2_CID_SHARPNESS which I >> guess will make image sharpened twice. Am I getting the patch right? > > Please, do not top-post! Sorry for top-posting. just forgot the netiquette. > > I am sorry, is it really so difficult to understand > >> >> > + Â Â Â Â Â Â Â ret = ici->ops->set_ctrl(icd, ctrl); >> >> > + Â Â Â Â Â Â Â if (ret != -ENOIOCTLCMD) >> >> > + Â Â Â Â Â Â Â Â Â Â Â return ret; > > which means just one thing: the camera host (interface if you like) driver > decides, whether it wants client's control to be called, in which case it > has to return -ENOIOCTLCMD, or it returns any other code (0 or a negative > error code), then the client will not be called. > yes I understand what you intended. but what I wanted to tell you was with this way, user should modify the camera host driver if they want to make camera host to return -ENOIOCTLCMD. >> If I'm getting right, it might be better to give user make a choice >> through platform data or some sort of variable which can make a choice >> between camera interface and camera device to process the CID. It >> could be just in aspect of manufacturer mind, we do love to make a >> choice between same features in different devices in easy way. So >> never mind if my idea is not helpful making your driver elegant :-) > > So far it seems too much to me. Let's wait until we get a case where it > really makes sense for platform code to decide who processes certain > controls. I think giving the host driver the power to decide should be ok > for now. > I totally understand. you are already doing a great job. I won't push you. Cheers, Nate > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ >
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c index 824c68b..8e987ca 100644 --- a/drivers/media/video/soc_camera.c +++ b/drivers/media/video/soc_camera.c @@ -633,6 +633,7 @@ static int soc_camera_queryctrl(struct file *file, void *priv, { struct soc_camera_file *icf = file->private_data; struct soc_camera_device *icd = icf->icd; + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); int i; WARN_ON(priv != file->private_data); @@ -640,6 +641,15 @@ static int soc_camera_queryctrl(struct file *file, void *priv, if (!qc->id) return -EINVAL; + /* First check host controls */ + for (i = 0; i < ici->ops->num_controls; i++) + if (qc->id == ici->ops->controls[i].id) { + memcpy(qc, &(ici->ops->controls[i]), + sizeof(*qc)); + return 0; + } + + /* Then device controls */ for (i = 0; i < icd->ops->num_controls; i++) if (qc->id == icd->ops->controls[i].id) { memcpy(qc, &(icd->ops->controls[i]), @@ -656,6 +666,7 @@ static int soc_camera_g_ctrl(struct file *file, void *priv, struct soc_camera_file *icf = file->private_data; struct soc_camera_device *icd = icf->icd; struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + int ret; WARN_ON(priv != file->private_data); @@ -672,6 +683,12 @@ static int soc_camera_g_ctrl(struct file *file, void *priv, return 0; } + if (ici->ops->get_ctrl) { + ret = ici->ops->get_ctrl(icd, ctrl); + if (ret != -ENOIOCTLCMD) + return ret; + } + return v4l2_device_call_until_err(&ici->v4l2_dev, (__u32)icd, core, g_ctrl, ctrl); } @@ -681,9 +698,16 @@ static int soc_camera_s_ctrl(struct file *file, void *priv, struct soc_camera_file *icf = file->private_data; struct soc_camera_device *icd = icf->icd; struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + int ret; WARN_ON(priv != file->private_data); + if (ici->ops->set_ctrl) { + ret = ici->ops->set_ctrl(icd, ctrl); + if (ret != -ENOIOCTLCMD) + return ret; + } + return v4l2_device_call_until_err(&ici->v4l2_dev, (__u32)icd, core, s_ctrl, ctrl); } diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h index 3bc5b6b..2d116bb 100644 --- a/include/media/soc_camera.h +++ b/include/media/soc_camera.h @@ -83,7 +83,11 @@ struct soc_camera_host_ops { int (*reqbufs)(struct soc_camera_file *, struct v4l2_requestbuffers *); int (*querycap)(struct soc_camera_host *, struct v4l2_capability *); int (*set_bus_param)(struct soc_camera_device *, __u32); + int (*get_ctrl)(struct soc_camera_device *, struct v4l2_control *); + int (*set_ctrl)(struct soc_camera_device *, struct v4l2_control *); unsigned int (*poll)(struct file *, poll_table *); + const struct v4l2_queryctrl *controls; + int num_controls; }; #define SOCAM_SENSOR_INVERT_PCLK (1 << 0)
Until now soc-camera only supported client (sensor) controls. This patch enables camera-host drivers to implement their own controls too. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/media/video/soc_camera.c | 24 ++++++++++++++++++++++++ include/media/soc_camera.h | 4 ++++ 2 files changed, 28 insertions(+), 0 deletions(-)