| Message ID | 1348822255-30875-2-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Anatolij On Fri, 28 Sep 2012, Anatolij Gustschin wrote: > With mpc52xx_csi interface I2C subdev streamon after vb2_streamon() > doesn't work due to mpc52xx sensor interface glue-logic restrictions. > > Since mpc5200 doesn't have a camera sensor interface, the sensor on > O2D cameras is connected to the LocalPlus bus by special glue-logic. > While sensor read-out the sensor will be clocked with the bus chip- > select signal as the sensor clock so that the read-out is synchronous > with the data accesses on the bus. Therefore, I2C access can't be > performed when the glue-logic is setup for sensor read-out. > > mpc52xx_csi driver for O2D cameras performs I2C subdev streamon in > its start_streaming() operation and then disables the I2C bus access > by configuring the interface glue-logic for sensor read-out. > For above-mentioned reasons we disable this subdev call here. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > drivers/media/platform/soc_camera/soc_camera.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c > index f6b1c1f..64e0abb 100644 > --- a/drivers/media/platform/soc_camera/soc_camera.c > +++ b/drivers/media/platform/soc_camera/soc_camera.c > @@ -809,7 +809,7 @@ static int soc_camera_streamon(struct file *file, void *priv, > { > struct soc_camera_device *icd = file->private_data; > struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > - struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > + struct v4l2_subdev __maybe_unused *sd = soc_camera_to_subdev(icd); > int ret; > > WARN_ON(priv != file->private_data); > @@ -826,8 +826,11 @@ static int soc_camera_streamon(struct file *file, void *priv, > else > ret = vb2_streamon(&icd->vb2_vidq, i); > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) No, we're not adding any preprocessor or run-time hardware dependencies to soc-camera or to any other generic code. I have no idea what those "IFM O2D" cameras are. If it's their common feature, that they cannot take any further I2C commands, while streaming, their drivers have to do that themselves. And not only for stream-on, there are many operations, that are allowed during streaming. Thanks Guennadi > if (!ret) > v4l2_subdev_call(sd, video, s_stream, 1); > +#endif > > return ret; > } > -- > 1.7.1 > --- 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
Hi Guennadi, On Fri, 28 Sep 2012 13:26:03 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: ... > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) > > No, we're not adding any preprocessor or run-time hardware dependencies to > soc-camera or to any other generic code. I have no idea what those "IFM > O2D" cameras are. If it's their common feature, that they cannot take any > further I2C commands, while streaming, their drivers have to do that > themselves. I'm not sure I understand you. To do what themselves? Thanks, Anatolij -- 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
Hi Anatolij > > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > > > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) > > > > No, we're not adding any preprocessor or run-time hardware dependencies to > > soc-camera or to any other generic code. I have no idea what those "IFM > > O2D" cameras are. If it's their common feature, that they cannot take any > > further I2C commands, while streaming, their drivers have to do that > > themselves. > > I'm not sure I understand you. To do what themselves? They - subdevice drivers of such IFM O2D cameras - should take care to avoid any I2C commands during a running read-out. Neither the bridge driver nor the framework core can or should know these details. This is just a generic call to a subdevice's .s_stream() method. What the driver does in it is totally its own business. Nobody says, that you have to issue I2C commands in it. 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
Hi Guennadi, On Wed, 3 Oct 2012 00:09:29 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Hi Anatolij > > > > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > > > > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) > > > > > > No, we're not adding any preprocessor or run-time hardware dependencies to > > > soc-camera or to any other generic code. I have no idea what those "IFM > > > O2D" cameras are. If it's their common feature, that they cannot take any > > > further I2C commands, while streaming, their drivers have to do that > > > themselves. > > > > I'm not sure I understand you. To do what themselves? > > They - subdevice drivers of such IFM O2D cameras - should take care to avoid > any I2C commands during a running read-out. Neither the bridge driver nor > the framework core can or should know these details. This is just a generic > call to a subdevice's .s_stream() method. What the driver does in it is > totally its own business. Nobody says, that you have to issue I2C commands > in it. The fact that many I2C commands are permitted during streaming doesn't necessary mean that an application must use them during streaming. Our camera application is aware of I2C bus access limitations during steaming and after streamon it doesn't use configurations resulting in I2C accesses to the sensor. But I'm not arguing for the ifdef here. We are using mt9v022 subdevice driver and I'd really avoid creating new custom subdevice driver, duplicating the mt9v022 driver functionality. This custom duplicated driver also won't be accepted in mainline, I think. I was thinking about a proposal for adding I2C bus access limitation awareness to the mt9v022 subdev driver. In our case we enable the snapshot mode by calling subdevice's .s_stream() in the .start_streaming() of the capture driver (that means as part of vb2_streamon() in soc_camera_streamon()) and then configure the logic for read-out there. Here is the relevant part from soc_camera_streamon(): if (ici->ops->init_videobuf) ret = videobuf_streamon(&icd->vb_vidq); else ret = vb2_streamon(&icd->vb2_vidq, i); After that I2C access is not possible until .stop_streaming() in the capture driver. After enabling streaming in .s_stream() the subdevice driver will have to filter (optionaly) further I2C accesses and return success code for them so that soc_camera_streamon() finaly succeeds. Currently the return code of s_stream v4l2_subdev_call() is not checked: if (!ret) v4l2_subdev_call(sd, video, s_stream, 1); But for the case it will be fixed, we have to return success code in s_stream, at least. Otherwise VIDIOC_STREAMON will fail. Will you accept adding optional I2C access filtering to the mt9v022 subdev driver? Thanks, Anatolij -- 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
Hi Anatolij On Fri, 5 Oct 2012, Anatolij Gustschin wrote: > Hi Guennadi, > > On Wed, 3 Oct 2012 00:09:29 +0200 (CEST) > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > > Hi Anatolij > > > > > > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > > > > > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) > > > > > > > > No, we're not adding any preprocessor or run-time hardware dependencies to > > > > soc-camera or to any other generic code. I have no idea what those "IFM > > > > O2D" cameras are. If it's their common feature, that they cannot take any > > > > further I2C commands, while streaming, their drivers have to do that > > > > themselves. > > > > > > I'm not sure I understand you. To do what themselves? > > > > They - subdevice drivers of such IFM O2D cameras - should take care to avoid > > any I2C commands during a running read-out. Neither the bridge driver nor > > the framework core can or should know these details. This is just a generic > > call to a subdevice's .s_stream() method. What the driver does in it is > > totally its own business. Nobody says, that you have to issue I2C commands > > in it. Unfortunately you still haven't explained what "IFM O2D" cameras are and why they have that I2C command restriction. Or - looking at the keyboard - is O2D just a typo and it should have been I2C? > The fact that many I2C commands are permitted during streaming doesn't > necessary mean that an application must use them during streaming. Our > camera application is aware of I2C bus access limitations during steaming > and after streamon it doesn't use configurations resulting in I2C accesses > to the sensor. But I'm not arguing for the ifdef here. But that's the only thing, that your patch is doing. So, this patch can be dropped? > We are using mt9v022 > subdevice driver and I'd really avoid creating new custom subdevice driver, > duplicating the mt9v022 driver functionality. This custom duplicated driver > also won't be accepted in mainline, I think. Correct. > I was thinking about a proposal > for adding I2C bus access limitation awareness to the mt9v022 subdev driver. I think I begin to understand. This is exactly why you need the snapshot mode in mt9v022, right? And in your camera host driver you issue a "stream-off" command on stream-on to switch into the snapshot mode, then you have to suppress the stream-on in soc-camera core. If I understand correctly, then this is broken. Your host driver will stop streaming on all sensor drivers and the ifdef will suppress the streamon from the soc-camera core, so, your host driver will only work on this specific board, where you have to use the snapshot mode on mt9v022. The proper solution, as I already suggested before, would be to implement a snapshot mode support. In that case the mt9v022 driver will be aware, that it has to work in the snapshot mode and will not switch over into the streaming mode. This topic has been raised several times on the mailing list, but until now nobody had a real need for a snapshot mode. The easiest way to do this would be to add a camera class control, however, I suspect, this is a much more complex topic, see, e.g. this recent thread: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/54079 Maybe you could review that thread and reply to it with your requirements? Thanks Guennadi > In our case we enable the snapshot mode by calling subdevice's .s_stream() > in the .start_streaming() of the capture driver (that means as part of > vb2_streamon() in soc_camera_streamon()) and then configure the logic > for read-out there. Here is the relevant part from soc_camera_streamon(): > > if (ici->ops->init_videobuf) > ret = videobuf_streamon(&icd->vb_vidq); > else > ret = vb2_streamon(&icd->vb2_vidq, i); > > After that I2C access is not possible until .stop_streaming() in the capture > driver. After enabling streaming in .s_stream() the subdevice driver will have > to filter (optionaly) further I2C accesses and return success code for them so > that soc_camera_streamon() finaly succeeds. Currently the return code of > s_stream v4l2_subdev_call() is not checked: > > if (!ret) > v4l2_subdev_call(sd, video, s_stream, 1); > > But for the case it will be fixed, we have to return success code in s_stream, > at least. Otherwise VIDIOC_STREAMON will fail. > > Will you accept adding optional I2C access filtering to the mt9v022 subdev > driver? > > Thanks, > Anatolij > --- 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, 5 Oct 2012 16:31:58 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Hi Anatolij > > On Fri, 5 Oct 2012, Anatolij Gustschin wrote: > > > Hi Guennadi, > > > > On Wed, 3 Oct 2012 00:09:29 +0200 (CEST) > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > > > > Hi Anatolij > > > > > > > > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > > > > > > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) > > > > > > > > > > No, we're not adding any preprocessor or run-time hardware dependencies to > > > > > soc-camera or to any other generic code. I have no idea what those "IFM > > > > > O2D" cameras are. If it's their common feature, that they cannot take any > > > > > further I2C commands, while streaming, their drivers have to do that > > > > > themselves. > > > > > > > > I'm not sure I understand you. To do what themselves? > > > > > > They - subdevice drivers of such IFM O2D cameras - should take care to avoid > > > any I2C commands during a running read-out. Neither the bridge driver nor > > > the framework core can or should know these details. This is just a generic > > > call to a subdevice's .s_stream() method. What the driver does in it is > > > totally its own business. Nobody says, that you have to issue I2C commands > > > in it. > > Unfortunately you still haven't explained what "IFM O2D" cameras are and > why they have that I2C command restriction. This is not true. I did. And I did it even _before_ anyone has asked me to explain it. Please read the commit log of this patch again. > > The fact that many I2C commands are permitted during streaming doesn't > > necessary mean that an application must use them during streaming. Our > > camera application is aware of I2C bus access limitations during steaming > > and after streamon it doesn't use configurations resulting in I2C accesses > > to the sensor. But I'm not arguing for the ifdef here. > > But that's the only thing, that your patch is doing. So, this patch can be > dropped? Only if there will be another possibility to isolate I2C access after streamon. Otherwise the capturing won't work. > > We are using mt9v022 > > subdevice driver and I'd really avoid creating new custom subdevice driver, > > duplicating the mt9v022 driver functionality. This custom duplicated driver > > also won't be accepted in mainline, I think. > > Correct. > > > I was thinking about a proposal > > for adding I2C bus access limitation awareness to the mt9v022 subdev driver. > > I think I begin to understand. This is exactly why you need the snapshot > mode in mt9v022, right? No! Not only. There are many different requirements for industrial camera applications. I.e. one requirement is to trigger a single frame on some external event and to capture exactly this triggered frame. Streaming mode is not suitable here. > And in your camera host driver you issue a > "stream-off" command on stream-on to switch into the snapshot mode, then > you have to suppress the stream-on in soc-camera core. If I understand > correctly, then this is broken. Your host driver will stop streaming on > all sensor drivers and the ifdef will suppress the streamon from the > soc-camera core, so, your host driver will only work on this specific > board, where you have to use the snapshot mode on mt9v022. What is wrong with this? Nothing is broken at all. This host driver is written for this specific board family and is not used on other boards. It cannot be used on other boards at all since these do not provide needed sensor glue logic. Even if other sensor drivers should ever be used on this board (most probably it will never be the case), they have to be used in snapshot mode anyway. > The proper solution, as I already suggested before, would be to implement > a snapshot mode support. In that case the mt9v022 driver will be aware, > that it has to work in the snapshot mode and will not switch over into the > streaming mode. > > This topic has been raised several times on the mailing list, but until > now nobody had a real need for a snapshot mode. The easiest way to do this > would be to add a camera class control, however, I suspect, this is a much > more complex topic, see, e.g. this recent thread: > > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/54079 > > Maybe you could review that thread and reply to it with your requirements? I did already read this thread partially. Unfortunately I do not have time for it now, maybe later. Thanks, Anatolij -- 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, 5 Oct 2012, Anatolij Gustschin wrote: > On Fri, 5 Oct 2012 16:31:58 +0200 (CEST) > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > > Hi Anatolij > > > > On Fri, 5 Oct 2012, Anatolij Gustschin wrote: > > > > > Hi Guennadi, > > > > > > On Wed, 3 Oct 2012 00:09:29 +0200 (CEST) > > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > > > > > > Hi Anatolij > > > > > > > > > > > +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ > > > > > > > + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) > > > > > > > > > > > > No, we're not adding any preprocessor or run-time hardware dependencies to > > > > > > soc-camera or to any other generic code. I have no idea what those "IFM > > > > > > O2D" cameras are. If it's their common feature, that they cannot take any > > > > > > further I2C commands, while streaming, their drivers have to do that > > > > > > themselves. > > > > > > > > > > I'm not sure I understand you. To do what themselves? > > > > > > > > They - subdevice drivers of such IFM O2D cameras - should take care to avoid > > > > any I2C commands during a running read-out. Neither the bridge driver nor > > > > the framework core can or should know these details. This is just a generic > > > > call to a subdevice's .s_stream() method. What the driver does in it is > > > > totally its own business. Nobody says, that you have to issue I2C commands > > > > in it. > > > > Unfortunately you still haven't explained what "IFM O2D" cameras are and > > why they have that I2C command restriction. > > This is not true. I did. And I did it even _before_ anyone has asked me to > explain it. Please read the commit log of this patch again. I did. It tells about the specific implementation, that you worked with, but I have no idea, whether that's a definition of an "O2D camera" or just your specific case. So, is an O2D camera a sensor, like mt9v022 + "special glue logic" to connect it to the LocalPlus bus? > > > The fact that many I2C commands are permitted during streaming doesn't > > > necessary mean that an application must use them during streaming. Our > > > camera application is aware of I2C bus access limitations during steaming > > > and after streamon it doesn't use configurations resulting in I2C accesses > > > to the sensor. But I'm not arguing for the ifdef here. > > > > But that's the only thing, that your patch is doing. So, this patch can be > > dropped? > > Only if there will be another possibility to isolate I2C access after > streamon. Otherwise the capturing won't work. > > > > We are using mt9v022 > > > subdevice driver and I'd really avoid creating new custom subdevice driver, > > > duplicating the mt9v022 driver functionality. This custom duplicated driver > > > also won't be accepted in mainline, I think. > > > > Correct. > > > > > I was thinking about a proposal > > > for adding I2C bus access limitation awareness to the mt9v022 subdev driver. > > > > I think I begin to understand. This is exactly why you need the snapshot > > mode in mt9v022, right? > > No! Not only. There are many different requirements for industrial camera > applications. I.e. one requirement is to trigger a single frame on some > external event and to capture exactly this triggered frame. Streaming mode > is not suitable here. I understand this, but your specific case is this your LocalPlus bus + glue, which can only operate the attached sensor in the snapshot mode, triggering each signle frame, using a chip-select pin. But it doesn't matter indeed in this case. Important is, that we have a valid use case and we have to make it work. Let's think how this snapshot mode is different from the "normal" streaming mode. The obvious difference is the sensor (mt9v022) configuration. So, our "switch to snapshot mode" command must have a subdevice operation or it has to be a control. Are there any more differences? Does the host have to know, that the snapshot mode has been activated? In master mode the regularity, at which frames will be arriving can change, but do hosts care? Most anyway work asynchronously, listening to sync signals. However, in slave mode the host will have to be synchronised with the trigger signal. I don't know, whether we have any active slave-mode set ups, i.e. whether we should care about those. In either case, this should be a proper configuration option, so far I don't see why a control should not suffice. > > And in your camera host driver you issue a > > "stream-off" command on stream-on to switch into the snapshot mode, then > > you have to suppress the stream-on in soc-camera core. If I understand > > correctly, then this is broken. Your host driver will stop streaming on > > all sensor drivers and the ifdef will suppress the streamon from the > > soc-camera core, so, your host driver will only work on this specific > > board, where you have to use the snapshot mode on mt9v022. > > What is wrong with this? Nothing is broken at all. This host driver is > written for this specific board family and is not used on other boards. > It cannot be used on other boards at all since these do not provide > needed sensor glue logic. Even if other sensor drivers should ever be > used on this board (most probably it will never be the case), they have > to be used in snapshot mode anyway. Hm, ok, if you're willing to maintain a driver in the mainline, that is only used on 1 family of boards - no problem with me. Still, I don't want it to look like a hack, e.g. calling streamoff in place of a streamon. So, we still need to extend the mt9v022 driver to explicitly support the snapshot mode, in which it will not switch to the streaming mode on streamon. And I think this should be a run-time operation, because eventually it can be used to switch between preview and taking single externally-triggered snapshots. Thanks Guennadi > > The proper solution, as I already suggested before, would be to implement > > a snapshot mode support. In that case the mt9v022 driver will be aware, > > that it has to work in the snapshot mode and will not switch over into the > > streaming mode. > > > > This topic has been raised several times on the mailing list, but until > > now nobody had a real need for a snapshot mode. The easiest way to do this > > would be to add a camera class control, however, I suspect, this is a much > > more complex topic, see, e.g. this recent thread: > > > > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/54079 > > > > Maybe you could review that thread and reply to it with your requirements? > > I did already read this thread partially. Unfortunately I do not have time > for it now, maybe later. > > Thanks, > Anatolij --- 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
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index f6b1c1f..64e0abb 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -809,7 +809,7 @@ static int soc_camera_streamon(struct file *file, void *priv, { struct soc_camera_device *icd = file->private_data; struct soc_camera_host *ici = to_soc_camera_host(icd->parent); - struct v4l2_subdev *sd = soc_camera_to_subdev(icd); + struct v4l2_subdev __maybe_unused *sd = soc_camera_to_subdev(icd); int ret; WARN_ON(priv != file->private_data); @@ -826,8 +826,11 @@ static int soc_camera_streamon(struct file *file, void *priv, else ret = vb2_streamon(&icd->vb2_vidq, i); +#if !defined(CONFIG_VIDEO_MPC52xx_CSI) && \ + !defined(CONFIG_VIDEO_MPC52xx_CSI_MODULE) if (!ret) v4l2_subdev_call(sd, video, s_stream, 1); +#endif return ret; }
With mpc52xx_csi interface I2C subdev streamon after vb2_streamon() doesn't work due to mpc52xx sensor interface glue-logic restrictions. Since mpc5200 doesn't have a camera sensor interface, the sensor on O2D cameras is connected to the LocalPlus bus by special glue-logic. While sensor read-out the sensor will be clocked with the bus chip- select signal as the sensor clock so that the read-out is synchronous with the data accesses on the bus. Therefore, I2C access can't be performed when the glue-logic is setup for sensor read-out. mpc52xx_csi driver for O2D cameras performs I2C subdev streamon in its start_streaming() operation and then disables the I2C bus access by configuring the interface glue-logic for sensor read-out. For above-mentioned reasons we disable this subdev call here. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- drivers/media/platform/soc_camera/soc_camera.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)