Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

[Anatolij Gustschin]

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

Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

[Guennadi Liakhovetski]

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/

Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

[Anatolij Gustschin]

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

Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

[Guennadi Liakhovetski]

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/

Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

[Anatolij Gustschin]

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

Re: [PATCH 2/2] V4L: soc_camera: disable I2C subdev streamon for mpc52xx_csi

[Guennadi Liakhovetski]

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/