Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi Hans, On Saturday 26 February 2011 13:50:12 Hans Verkuil wrote: > On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote: > > Guennadi Liakhovetski wrote: > > > On Wed, 23 Feb 2011, Hans Verkuil wrote: > > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > > >>> Clock values are often being rounded at runtime and do not always > > >>> reflect exactly the numbers fixed at compile time. And negotiation > > >>> could help to obtain exact values at both sensor and host side. > > >> > > >> The only static data I am concerned about are those that affect signal > > >> integrity. After thinking carefully about this I realized that there > > >> is really only one setting that is relevant to that: the sampling > > >> edge. The polarities do not matter in this. > > > > > > Ok, this is much better! I'm still not perfectly happy having to punish > > > all just for the sake of a couple of broken boards, but I can certainly > > > much better live with this, than with having to hard-code each and > > > every bit. Thanks, Hans! > > > > How much punishing would actually take place without autonegotiation? > > How many boards do we have in total? I counted around 26 of > > soc_camera_link declarations under arch/. Are there more? > > > > An example of hardware which does care about clock polarity is the > > N8[01]0. The parallel clock polarity is inverted since this actually > > does improve reliability. In an ideal hardware this likely wouldn't > > happen but sometimes the hardware is not exactly ideal. Both the sensor > > and the camera block support non-inverted and inverted clock signal. > > > > So at the very least it should be possible to provide this information > > in the board code even if both ends share multiple common values for > > parameters. > > > > There have been many comments on the dangers of the autonegotiation and > > I share those concerns. One of my main concerns is that it creates an > > unnecessary dependency from all the boards to the negotiation code, the > > behaviour of which may not change. > > OK, let me summarize this and if there are no objections then Stan can > start implementing this. > > 1) We need two subdev ops: one reports the bus config capabilities and one > that sets it up. Note that these ops should be core ops since this > functionality is relevant for both sensors and video receive/transmit > devices. Could you elaborate on this ? Stan's original proposal is to report the subdev configuration so that the host can configure itself at streamon time. Why do we need an operation to setup the subdev ? > 2) The clock sampling edge and polarity should not be negotiated but must > be set from board code for both subdevs and host. In the future this might > even require a callback with the clock frequency as argument. > > 3) We probably need a utility function that given the host and subdev > capabilities will return the required subdev/host settings. > > 4) soc-camera should switch to these new ops. > > Of course, we also need MIPI support in this API. The same considerations > apply to MIPI as to the parallel bus: settings that depend on the hardware > board design should come from board code, others can be negotiated. Since > I know next to nothing about MIPI I will leave that to the experts... > > One thing I am not sure about is if we want separate ops for parallel bus > and MIPI, or if we merge them. I am leaning towards separate ops as I > think that might be easier to implement. -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On 02/26/2011 01:50 PM, Hans Verkuil wrote: > On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote: >> Hi Guennadi and others, >> >> Apologies for the late reply... >> >> Guennadi Liakhovetski wrote: >>> On Wed, 23 Feb 2011, Hans Verkuil wrote: >>> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > Clock values are often being rounded at runtime and do not always reflect > exactly > the numbers fixed at compile time. And negotiation could help to obtain > exact > values at both sensor and host side. The only static data I am concerned about are those that affect signal integrity. After thinking carefully about this I realized that there is really only one setting that is relevant to that: the sampling edge. The polarities do not matter in this. >>> >>> Ok, this is much better! I'm still not perfectly happy having to punish >>> all just for the sake of a couple of broken boards, but I can certainly >>> much better live with this, than with having to hard-code each and every >>> bit. Thanks, Hans! >> >> How much punishing would actually take place without autonegotiation? >> How many boards do we have in total? I counted around 26 of >> soc_camera_link declarations under arch/. Are there more? >> >> An example of hardware which does care about clock polarity is the >> N8[01]0. The parallel clock polarity is inverted since this actually >> does improve reliability. In an ideal hardware this likely wouldn't >> happen but sometimes the hardware is not exactly ideal. Both the sensor >> and the camera block support non-inverted and inverted clock signal. >> >> So at the very least it should be possible to provide this information >> in the board code even if both ends share multiple common values for >> parameters. >> >> There have been many comments on the dangers of the autonegotiation and >> I share those concerns. One of my main concerns is that it creates an >> unnecessary dependency from all the boards to the negotiation code, the >> behaviour of which may not change. > > OK, let me summarize this and if there are no objections then Stan can start > implementing this. > > 1) We need two subdev ops: one reports the bus config capabilities and one > that > sets it up. Note that these ops should be core ops since this functionality is > relevant for both sensors and video receive/transmit devices. Sounds reasonable. In case of MIPI-CSI receiver as a separate subdev I assume it would allow to retrieve settings from sensor subdev and apply them to MIPI-CSI receiver. > > 2) The clock sampling edge and polarity should not be negotiated but must be > set > from board code for both subdevs and host. In the future this might even > require > a callback with the clock frequency as argument. > > 3) We probably need a utility function that given the host and subdev > capabilities > will return the required subdev/host settings. > > 4) soc-camera should switch to these new ops. > > Of course, we also need MIPI support in this API. The same considerations > apply to > MIPI as to the parallel bus: settings that depend on the hardware board design > should come from board code, others can be negotiated. Since I know next to > nothing > about MIPI I will leave that to the experts... > > One thing I am not sure about is if we want separate ops for parallel bus and > MIPI, > or if we merge them. I am leaning towards separate ops as I think that might > be > easier to implement. I suppose it wouldn't hurt to have same, parametrized ops for both parallel and serial bus. Just like in the original Stan's RFC. > > Regards, > > Hans > -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
On Saturday, February 26, 2011 14:14:29 Guennadi Liakhovetski wrote: > On Sat, 26 Feb 2011, Hans Verkuil wrote: > > > On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote: > > > Hi Guennadi and others, > > > > > > Apologies for the late reply... > > > > > > Guennadi Liakhovetski wrote: > > > > On Wed, 23 Feb 2011, Hans Verkuil wrote: > > > > > > > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > > > >>> Clock values are often being rounded at runtime and do not always > > > >>> reflect exactly > > > >>> the numbers fixed at compile time. And negotiation could help to > > > >>> obtain exact > > > >>> values at both sensor and host side. > > > >> > > > >> The only static data I am concerned about are those that affect signal > > > >> integrity. > > > >> After thinking carefully about this I realized that there is really > > > >> only one > > > >> setting that is relevant to that: the sampling edge. The polarities do > > > >> not > > > >> matter in this. > > > > > > > > Ok, this is much better! I'm still not perfectly happy having to punish > > > > all just for the sake of a couple of broken boards, but I can certainly > > > > much better live with this, than with having to hard-code each and > > > > every > > > > bit. Thanks, Hans! > > > > > > How much punishing would actually take place without autonegotiation? > > > How many boards do we have in total? I counted around 26 of > > > soc_camera_link declarations under arch/. Are there more? > > > > > > An example of hardware which does care about clock polarity is the > > > N8[01]0. The parallel clock polarity is inverted since this actually > > > does improve reliability. In an ideal hardware this likely wouldn't > > > happen but sometimes the hardware is not exactly ideal. Both the sensor > > > and the camera block support non-inverted and inverted clock signal. > > > > > > So at the very least it should be possible to provide this information > > > in the board code even if both ends share multiple common values for > > > parameters. > > > > > > There have been many comments on the dangers of the autonegotiation and > > > I share those concerns. One of my main concerns is that it creates an > > > unnecessary dependency from all the boards to the negotiation code, the > > > behaviour of which may not change. > > Sorry, didn't want to comment on this... But to me this sounds like a void > argument... Yes, there are _many_ inter-dependencies in the kernel, and if > you break code something will stop working... What's new about it??? But > no, I do not want to continue this discussion endlessly... > > > OK, let me summarize this and if there are no objections then Stan can start > > implementing this. > > > > 1) We need two subdev ops: one reports the bus config capabilities and one > > that > > sets it up. Note that these ops should be core ops since this functionality > > is > > relevant for both sensors and video receive/transmit devices. > > > > 2) The clock sampling edge and polarity should not be negotiated but must > > be set > > from board code for both subdevs and host. In the future this might even > > require > > a callback with the clock frequency as argument. > > > > 3) We probably need a utility function that given the host and subdev > > capabilities > > will return the required subdev/host settings. > > > > 4) soc-camera should switch to these new ops. > > ...remains only to find, who will do this;) > > So, I'm in minority here, if we don't count all those X systems, > successfully using soc-camera with its evil auto-negotiation. If you just > decide to do this and push the changes - sure, there's nothing I can do > against this. But if you decide to postpone a final decision on this until > we meet personally and will not have to circulate the same arguments 100 > times - just because the delay is shorter - maybe we can find a solution, > that will keep everyone happy. No, I am no longer willing to postpone this. Sorry. Discussing this in a brainstorm meeting or whatever won't bring us any closer. We did that in the last Helsinki meeting already. Heck, we've gone over this for a year and a half now, if not more. The arguments haven't changed in all that time. Enough is enough. Let Stan implement the new subdev core ops (Stan, please confirm that you can work on this!), then we can use it in soc-camera for everything except the clock. The final step will be to remove the clock negotiation from soc-camera. By the time we are ready for that final step we'll see who can do this. It's several months in the future anyway. Regards, Hans > > Of course, we also need MIPI support in this API. The same considerations > > apply to > > MIPI as to the parallel bus: settings that depend on the hardware board > > design > > should come from board code, others can be negotiated. Since I know next to > > nothing > > about MIPI I will leave that to the experts... > > > > One thin
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Sat, 26 Feb 2011, Hans Verkuil wrote: > On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote: > > Hi Guennadi and others, > > > > Apologies for the late reply... > > > > Guennadi Liakhovetski wrote: > > > On Wed, 23 Feb 2011, Hans Verkuil wrote: > > > > > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > > >>> Clock values are often being rounded at runtime and do not always > > >>> reflect exactly > > >>> the numbers fixed at compile time. And negotiation could help to obtain > > >>> exact > > >>> values at both sensor and host side. > > >> > > >> The only static data I am concerned about are those that affect signal > > >> integrity. > > >> After thinking carefully about this I realized that there is really only > > >> one > > >> setting that is relevant to that: the sampling edge. The polarities do > > >> not > > >> matter in this. > > > > > > Ok, this is much better! I'm still not perfectly happy having to punish > > > all just for the sake of a couple of broken boards, but I can certainly > > > much better live with this, than with having to hard-code each and every > > > bit. Thanks, Hans! > > > > How much punishing would actually take place without autonegotiation? > > How many boards do we have in total? I counted around 26 of > > soc_camera_link declarations under arch/. Are there more? > > > > An example of hardware which does care about clock polarity is the > > N8[01]0. The parallel clock polarity is inverted since this actually > > does improve reliability. In an ideal hardware this likely wouldn't > > happen but sometimes the hardware is not exactly ideal. Both the sensor > > and the camera block support non-inverted and inverted clock signal. > > > > So at the very least it should be possible to provide this information > > in the board code even if both ends share multiple common values for > > parameters. > > > > There have been many comments on the dangers of the autonegotiation and > > I share those concerns. One of my main concerns is that it creates an > > unnecessary dependency from all the boards to the negotiation code, the > > behaviour of which may not change. Sorry, didn't want to comment on this... But to me this sounds like a void argument... Yes, there are _many_ inter-dependencies in the kernel, and if you break code something will stop working... What's new about it??? But no, I do not want to continue this discussion endlessly... > OK, let me summarize this and if there are no objections then Stan can start > implementing this. > > 1) We need two subdev ops: one reports the bus config capabilities and one > that > sets it up. Note that these ops should be core ops since this functionality is > relevant for both sensors and video receive/transmit devices. > > 2) The clock sampling edge and polarity should not be negotiated but must be > set > from board code for both subdevs and host. In the future this might even > require > a callback with the clock frequency as argument. > > 3) We probably need a utility function that given the host and subdev > capabilities > will return the required subdev/host settings. > > 4) soc-camera should switch to these new ops. ...remains only to find, who will do this;) So, I'm in minority here, if we don't count all those X systems, successfully using soc-camera with its evil auto-negotiation. If you just decide to do this and push the changes - sure, there's nothing I can do against this. But if you decide to postpone a final decision on this until we meet personally and will not have to circulate the same arguments 100 times - just because the delay is shorter - maybe we can find a solution, that will keep everyone happy. > Of course, we also need MIPI support in this API. The same considerations > apply to > MIPI as to the parallel bus: settings that depend on the hardware board design > should come from board code, others can be negotiated. Since I know next to > nothing > about MIPI I will leave that to the experts... > > One thing I am not sure about is if we want separate ops for parallel bus and > MIPI, > or if we merge them. I am leaning towards separate ops as I think that might > be > easier to implement. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote: > Hi Guennadi and others, > > Apologies for the late reply... > > Guennadi Liakhovetski wrote: > > On Wed, 23 Feb 2011, Hans Verkuil wrote: > > > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > >>> Clock values are often being rounded at runtime and do not always reflect > >>> exactly > >>> the numbers fixed at compile time. And negotiation could help to obtain > >>> exact > >>> values at both sensor and host side. > >> > >> The only static data I am concerned about are those that affect signal > >> integrity. > >> After thinking carefully about this I realized that there is really only > >> one > >> setting that is relevant to that: the sampling edge. The polarities do not > >> matter in this. > > > > Ok, this is much better! I'm still not perfectly happy having to punish > > all just for the sake of a couple of broken boards, but I can certainly > > much better live with this, than with having to hard-code each and every > > bit. Thanks, Hans! > > How much punishing would actually take place without autonegotiation? > How many boards do we have in total? I counted around 26 of > soc_camera_link declarations under arch/. Are there more? > > An example of hardware which does care about clock polarity is the > N8[01]0. The parallel clock polarity is inverted since this actually > does improve reliability. In an ideal hardware this likely wouldn't > happen but sometimes the hardware is not exactly ideal. Both the sensor > and the camera block support non-inverted and inverted clock signal. > > So at the very least it should be possible to provide this information > in the board code even if both ends share multiple common values for > parameters. > > There have been many comments on the dangers of the autonegotiation and > I share those concerns. One of my main concerns is that it creates an > unnecessary dependency from all the boards to the negotiation code, the > behaviour of which may not change. OK, let me summarize this and if there are no objections then Stan can start implementing this. 1) We need two subdev ops: one reports the bus config capabilities and one that sets it up. Note that these ops should be core ops since this functionality is relevant for both sensors and video receive/transmit devices. 2) The clock sampling edge and polarity should not be negotiated but must be set from board code for both subdevs and host. In the future this might even require a callback with the clock frequency as argument. 3) We probably need a utility function that given the host and subdev capabilities will return the required subdev/host settings. 4) soc-camera should switch to these new ops. Of course, we also need MIPI support in this API. The same considerations apply to MIPI as to the parallel bus: settings that depend on the hardware board design should come from board code, others can be negotiated. Since I know next to nothing about MIPI I will leave that to the experts... One thing I am not sure about is if we want separate ops for parallel bus and MIPI, or if we merge them. I am leaning towards separate ops as I think that might be easier to implement. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi Guennadi and others, Apologies for the late reply... Guennadi Liakhovetski wrote: > On Wed, 23 Feb 2011, Hans Verkuil wrote: > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: >>> Clock values are often being rounded at runtime and do not always reflect >>> exactly >>> the numbers fixed at compile time. And negotiation could help to obtain >>> exact >>> values at both sensor and host side. >> >> The only static data I am concerned about are those that affect signal >> integrity. >> After thinking carefully about this I realized that there is really only one >> setting that is relevant to that: the sampling edge. The polarities do not >> matter in this. > > Ok, this is much better! I'm still not perfectly happy having to punish > all just for the sake of a couple of broken boards, but I can certainly > much better live with this, than with having to hard-code each and every > bit. Thanks, Hans! How much punishing would actually take place without autonegotiation? How many boards do we have in total? I counted around 26 of soc_camera_link declarations under arch/. Are there more? An example of hardware which does care about clock polarity is the N8[01]0. The parallel clock polarity is inverted since this actually does improve reliability. In an ideal hardware this likely wouldn't happen but sometimes the hardware is not exactly ideal. Both the sensor and the camera block support non-inverted and inverted clock signal. So at the very least it should be possible to provide this information in the board code even if both ends share multiple common values for parameters. There have been many comments on the dangers of the autonegotiation and I share those concerns. One of my main concerns is that it creates an unnecessary dependency from all the boards to the negotiation code, the behaviour of which may not change. Regards, -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi, >> Sorry, I accept different opinions, and in the end only one of the two >> possibilities will be implemented, and either way it'll all work in the >> end, but, I don't buy either of these arguments. > >> Complexity - the code is >> already there, it is working, it is simple, it has not broken since it has >> been implemented. I had it hard-coded in the beginning and I went over to >> negotiation and never regretted it. > > First of all, it seems that this discussion is heavily parallel i/f > oriented, and soc_camera focused, and it's just not like that. > yes, it seems that is correct. My patch just get back on host side some sensor dynamic parameters and it doesn't pretending for any negotiation. > Now, _just_ for soc_camera framework, yeah... it works and it's there, but > still not providing a solution for other v4l2_subdev users (like Media > Controller). > I already start looking into soc_camera code, but I'm so confused. :( > Complexity comes only when trying to make this truly generic, and avoid > fragmentation of solutions (1 for soc, 1 for MC), plus adding support for > serial (MIPI) interfaces > > Now, also, the patch originally proposed by Stan doesn't actually deal with > putting polarities as part of the interface parameters, which is something > you're currently negotiating in soc_camera framework, again, just for > parallel interfaces. > > Now, just for the sake of clarifying my understanding, I guess what you're > saying is to make sensor driver expose all possible polarities and physical > details configurable, and make the platform data limit the actual options due > to the physical layout. > > For example, if in my board A, I have: > > - OV5640 sensor driver, which supports both Parallel and CSI2 > Interfaces (with up to 2 datalanes) > - Rx subdev (or host) driver(s) which support Parallel, CSI2-A and > CSI2-B interfaces (with 2 and 1 datalanes respectively). > If the sensor is physically connected as parallel and serial the board code should dictates the preferred interface, IMO. Or ... > I should specify in my boardfile integration details, such as the > sensor is actually wired to the CSI2-B I/f, so make the sensor > negotiate with the other side of the bus and enable CSI2 i/f with > given details, like just use 1 datalane, and match datalane > position/polarity. > > Am I understanding right? > >> Hardware damage - if this were the case, I'd probably be surrounded only >> by bricks. How configuring a wrong hsync polarity can damage your >> hardware? > > Ok, I'll regret my statement on this one. I guess I was a bit too dramatic > to point out consequences of HW mismatches. Nevermind this. > :) > Regards, > Sergio > >> 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best regards, Stan -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi Guennadi, > -Original Message- > From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de] > Sent: Wednesday, February 23, 2011 10:46 AM > To: Aguirre, Sergio > Cc: Laurent Pinchart; Hans Verkuil; Sylwester Nawrocki; Stan; linux- > me...@vger.kernel.org > Subject: RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms > > On Wed, 23 Feb 2011, Aguirre, Sergio wrote: > > > Hi, > > > > > -Original Message- > > > From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] > > > Sent: Wednesday, February 23, 2011 10:15 AM > > > To: Hans Verkuil > > > Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester > > > Nawrocki; Stan; linux-media@vger.kernel.org > > > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation > g_interface_parms > > > > > > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote: > > > > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote: > > > > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > > > > > > > > > > > > > > > > The only static data I am concerned about are those that > affect > > > > > > > > signal integrity. > > > > > > > > > > > > > > > > After thinking carefully about this I realized that there is > > > really > > > > > > > > only one setting that is relevant to that: the sampling > edge. > > > The > > > > > > > > polarities do not matter in this. > > > > > > > > > > > > I respectfully disagree. > > > > > > > > > > So do I. Sampling edge is related to polarities, so you need to > take > > > both > > > > > into account. > > > > > > > > When you switch polarity for data/field/hsync/vsync signals on a > simple > > > bus > > > > you just invert whether a 1 bit is output as high or low voltage. So > you > > > > just change the meaning of the bit. This does not matter for signal > > > > integrity, since you obviously have to be able to sample both low > and > > > high > > > > voltages. It is *when* you sample that can have a major effect. > > > > > > When you switch the polarity you will likely have to sample on the > > > opposite > > > edge. If, for signal integrity reasons, you can only sample on a given > > > edge, > > > you will want to use a fixed polarity and not negotiate it. > > > > I guess this should be reason enough to decide this in platform data in > the > > board file. > > > > > > > > Given the very small number of parameters that are negotiated by soc- > > > camera at > > > the moment, I'm very much in favour of hardcoding all of them in > platform > > > data > > > and just adding a g_interface_parms subdev operation. > > > > I'll second that. > > > > Negotiating all this adds unnecessary complexity, and just makes the > code > > More prone to errors, and even probably causing hardware damage due to > > misconfiguration. It's better to keep this static and make the board > config > > fully conscious of it. > > Sorry, I accept different opinions, and in the end only one of the two > possibilities will be implemented, and either way it'll all work in the > end, but, I don't buy either of these arguments. > Complexity - the code is > already there, it is working, it is simple, it has not broken since it has > been implemented. I had it hard-coded in the beginning and I went over to > negotiation and never regretted it. First of all, it seems that this discussion is heavily parallel i/f oriented, and soc_camera focused, and it's just not like that. Now, _just_ for soc_camera framework, yeah... it works and it's there, but still not providing a solution for other v4l2_subdev users (like Media Controller). Complexity comes only when trying to make this truly generic, and avoid fragmentation of solutions (1 for soc, 1 for MC), plus adding support for serial (MIPI) interfaces Now, also, the patch originally proposed by Stan doesn't actually deal with putting polarities as part of the interface parameters, which is something you're currently negotiating in soc_camera framework, again, just for parallel interfaces. Now, just for the sake of clarifying my understanding, I guess what you're saying is to make sensor driver expose all possible polarities and physical details conf
RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wed, 23 Feb 2011, Aguirre, Sergio wrote: > Hi, > > > -Original Message- > > From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] > > Sent: Wednesday, February 23, 2011 10:15 AM > > To: Hans Verkuil > > Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester > > Nawrocki; Stan; linux-media@vger.kernel.org > > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms > > > > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote: > > > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote: > > > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > > > > > > > > > > > > > The only static data I am concerned about are those that affect > > > > > > > signal integrity. > > > > > > > > > > > > > > After thinking carefully about this I realized that there is > > really > > > > > > > only one setting that is relevant to that: the sampling edge. > > The > > > > > > > polarities do not matter in this. > > > > > > > > > > I respectfully disagree. > > > > > > > > So do I. Sampling edge is related to polarities, so you need to take > > both > > > > into account. > > > > > > When you switch polarity for data/field/hsync/vsync signals on a simple > > bus > > > you just invert whether a 1 bit is output as high or low voltage. So you > > > just change the meaning of the bit. This does not matter for signal > > > integrity, since you obviously have to be able to sample both low and > > high > > > voltages. It is *when* you sample that can have a major effect. > > > > When you switch the polarity you will likely have to sample on the > > opposite > > edge. If, for signal integrity reasons, you can only sample on a given > > edge, > > you will want to use a fixed polarity and not negotiate it. > > I guess this should be reason enough to decide this in platform data in the > board file. > > > > > Given the very small number of parameters that are negotiated by soc- > > camera at > > the moment, I'm very much in favour of hardcoding all of them in platform > > data > > and just adding a g_interface_parms subdev operation. > > I'll second that. > > Negotiating all this adds unnecessary complexity, and just makes the code > More prone to errors, and even probably causing hardware damage due to > misconfiguration. It's better to keep this static and make the board config > fully conscious of it. Sorry, I accept different opinions, and in the end only one of the two possibilities will be implemented, and either way it'll all work in the end, but, I don't buy either of these arguments. Complexity - the code is already there, it is working, it is simple, it has not broken since it has been implemented. I had it hard-coded in the beginning and I went over to negotiation and never regretted it. Hardware damage - if this were the case, I'd probably be surrounded only by bricks. How configuring a wrong hsync polarity can damage your hardware? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wed, 23 Feb 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Wednesday 23 February 2011 10:31:21 Guennadi Liakhovetski wrote: > > [snip] > > > Currently soc-camera auto-configures the following parameters: > > > > hsync polarity > > vsync polarity > > data polarity > > Data polarity ? Are there sensors that can invert the data polarity ? Yes. Regards Guennadi > > > master / slave mode > > data bus width > > pixel clock polarity > > -- > Regards, > > Laurent Pinchart > --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi Guennadi, On Wednesday 23 February 2011 10:31:21 Guennadi Liakhovetski wrote: [snip] > Currently soc-camera auto-configures the following parameters: > > hsync polarity > vsync polarity > data polarity Data polarity ? Are there sensors that can invert the data polarity ? > master / slave mode > data bus width > pixel clock polarity -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday 23 February 2011 17:28:39 Hans Verkuil wrote: > On Wednesday, February 23, 2011 17:14:41 Laurent Pinchart wrote: > > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote: [snip] > > > When you switch polarity for data/field/hsync/vsync signals on a simple > > > bus you just invert whether a 1 bit is output as high or low voltage. So > > > you just change the meaning of the bit. This does not matter for > > > signal integrity, since you obviously have to be able to sample both > > > low and high voltages. It is *when* you sample that can have a major > > > effect. > > > > When you switch the polarity you will likely have to sample on the > > opposite edge. If, for signal integrity reasons, you can only sample on > > a given edge, you will want to use a fixed polarity and not negotiate > > it. > > You are confusing clock polarity (which is relevant) with data, field, > hsync and vsync polarities which just invert the meaning of those pins. I was talking about clock polarity. Sorry for the confusion. > I wish I had a whiteboard to draw it, much easier to explain that way. > > > Given the very small number of parameters that are negotiated by > > soc-camera at the moment, I'm very much in favour of hardcoding all of > > them in platform data and just adding a g_interface_parms subdev > > operation. > > Just for the record: I have no problem with hardcoding these polarities. > After all, that was my original idea. But they can be negotiated as well. -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday, February 23, 2011 17:14:41 Laurent Pinchart wrote: > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote: > > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote: > > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > > > > > > > > > > The only static data I am concerned about are those that affect > > > > > > signal integrity. > > > > > > > > > > > > After thinking carefully about this I realized that there is really > > > > > > only one setting that is relevant to that: the sampling edge. The > > > > > > polarities do not matter in this. > > > > > > > > I respectfully disagree. > > > > > > So do I. Sampling edge is related to polarities, so you need to take both > > > into account. > > > > When you switch polarity for data/field/hsync/vsync signals on a simple bus > > you just invert whether a 1 bit is output as high or low voltage. So you > > just change the meaning of the bit. This does not matter for signal > > integrity, since you obviously have to be able to sample both low and high > > voltages. It is *when* you sample that can have a major effect. > > When you switch the polarity you will likely have to sample on the opposite > edge. If, for signal integrity reasons, you can only sample on a given edge, > you will want to use a fixed polarity and not negotiate it. You are confusing clock polarity (which is relevant) with data, field, hsync and vsync polarities which just invert the meaning of those pins. I wish I had a whiteboard to draw it, much easier to explain that way. > > Given the very small number of parameters that are negotiated by soc-camera at > the moment, I'm very much in favour of hardcoding all of them in platform data > and just adding a g_interface_parms subdev operation. Just for the record: I have no problem with hardcoding these polarities. After all, that was my original idea. But they can be negotiated as well. Regards, Hans > > > This might be different for differential clocks. I have no experience with > > this, so I can't say anything sensible about that. > > -- > 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi, > -Original Message- > From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] > Sent: Wednesday, February 23, 2011 10:15 AM > To: Hans Verkuil > Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester > Nawrocki; Stan; linux-media@vger.kernel.org > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms > > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote: > > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote: > > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > > > > > > > > > > The only static data I am concerned about are those that affect > > > > > > signal integrity. > > > > > > > > > > > > After thinking carefully about this I realized that there is > really > > > > > > only one setting that is relevant to that: the sampling edge. > The > > > > > > polarities do not matter in this. > > > > > > > > I respectfully disagree. > > > > > > So do I. Sampling edge is related to polarities, so you need to take > both > > > into account. > > > > When you switch polarity for data/field/hsync/vsync signals on a simple > bus > > you just invert whether a 1 bit is output as high or low voltage. So you > > just change the meaning of the bit. This does not matter for signal > > integrity, since you obviously have to be able to sample both low and > high > > voltages. It is *when* you sample that can have a major effect. > > When you switch the polarity you will likely have to sample on the > opposite > edge. If, for signal integrity reasons, you can only sample on a given > edge, > you will want to use a fixed polarity and not negotiate it. I guess this should be reason enough to decide this in platform data in the board file. > > Given the very small number of parameters that are negotiated by soc- > camera at > the moment, I'm very much in favour of hardcoding all of them in platform > data > and just adding a g_interface_parms subdev operation. I'll second that. Negotiating all this adds unnecessary complexity, and just makes the code More prone to errors, and even probably causing hardware damage due to misconfiguration. It's better to keep this static and make the board config fully conscious of it. Regards, Sergio > > > This might be different for differential clocks. I have no experience > with > > this, so I can't say anything sensible about that. > > -- > 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote: > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote: > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > > > > > > > The only static data I am concerned about are those that affect > > > > > signal integrity. > > > > > > > > > > After thinking carefully about this I realized that there is really > > > > > only one setting that is relevant to that: the sampling edge. The > > > > > polarities do not matter in this. > > > > > > I respectfully disagree. > > > > So do I. Sampling edge is related to polarities, so you need to take both > > into account. > > When you switch polarity for data/field/hsync/vsync signals on a simple bus > you just invert whether a 1 bit is output as high or low voltage. So you > just change the meaning of the bit. This does not matter for signal > integrity, since you obviously have to be able to sample both low and high > voltages. It is *when* you sample that can have a major effect. When you switch the polarity you will likely have to sample on the opposite edge. If, for signal integrity reasons, you can only sample on a given edge, you will want to use a fixed polarity and not negotiate it. Given the very small number of parameters that are negotiated by soc-camera at the moment, I'm very much in favour of hardcoding all of them in platform data and just adding a g_interface_parms subdev operation. > This might be different for differential clocks. I have no experience with > this, so I can't say anything sensible about that. -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote: > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > > > > The only static data I am concerned about are those that affect signal > > > > integrity. > > > > > > > After thinking carefully about this I realized that there is really > > > > only one setting that is relevant to that: the sampling edge. The > > > > polarities do not matter in this. > > > > I respectfully disagree. > > So do I. Sampling edge is related to polarities, so you need to take both into > account. When you switch polarity for data/field/hsync/vsync signals on a simple bus you just invert whether a 1 bit is output as high or low voltage. So you just change the meaning of the bit. This does not matter for signal integrity, since you obviously have to be able to sample both low and high voltages. It is *when* you sample that can have a major effect. This might be different for differential clocks. I have no experience with this, so I can't say anything sensible about that. Regards, Hans > > > AFAIK, There is not such thing as sampling edge configuration for MIPI > > Receivers, and the polarities DO matter, since it's a differential > > signal. > > > > > Ok, this is much better! I'm still not perfectly happy having to punish > > > all just for the sake of a couple of broken boards, but I can certainly > > > much better live with this, than with having to hard-code each and every > > > bit. Thanks, Hans! > > > > > > So, I think, we can proceed with this, let's see the code now, shall > > > we?;) > > > > > > Currently soc-camera auto-configures the following parameters: > > > > > > hsync polarity > > > vsync polarity > > > data polarity > > > master / slave mode > > What do you mean by master/slave mode ? > > > > data bus width > > The data bus width can already be configured through the media bus format. Do > we need to set it explicitly ? > > > > pixel clock polarity > > > > > > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and > > > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). > > > Removing the pixclk polarity, the rest we can use as a basis for a new > > > subdev-based implementation. > > > > Don't we need to move this out from soc_camera and make it available in > > v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI > > support) > > > > That way both SoC_Camera, and Media Controller frameworks can use that. > > -- > 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 > -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wed, 23 Feb 2011, Laurent Pinchart wrote: > > > Currently soc-camera auto-configures the following parameters: > > > > > > hsync polarity > > > vsync polarity > > > data polarity > > > master / slave mode > > What do you mean by master/slave mode ? Many datasheets define a slave mode, in which the sensor is receiving the sync signals and the pixel clock from the host and is only driving the data lanes. > > > data bus width > > The data bus width can already be configured through the media bus format. Do > we need to set it explicitly ? Maybe we'd have to think about it more, but I think, we need it: Bus width, specified by media bus formats tells you how the data can be sampled on the bus. Whereas the above parameter tells you, how the devices are physically connected. With one and the same physical connection you can get several data formats, e.g., by using shifters, like on omap3. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: > > > > > The only static data I am concerned about are those that affect signal > > > integrity. > > > > > After thinking carefully about this I realized that there is really > > > only one setting that is relevant to that: the sampling edge. The > > > polarities do not matter in this. > > I respectfully disagree. So do I. Sampling edge is related to polarities, so you need to take both into account. > AFAIK, There is not such thing as sampling edge configuration for MIPI > Receivers, and the polarities DO matter, since it's a differential > signal. > > > Ok, this is much better! I'm still not perfectly happy having to punish > > all just for the sake of a couple of broken boards, but I can certainly > > much better live with this, than with having to hard-code each and every > > bit. Thanks, Hans! > > > > So, I think, we can proceed with this, let's see the code now, shall > > we?;) > > > > Currently soc-camera auto-configures the following parameters: > > > > hsync polarity > > vsync polarity > > data polarity > > master / slave mode What do you mean by master/slave mode ? > > data bus width The data bus width can already be configured through the media bus format. Do we need to set it explicitly ? > > pixel clock polarity > > > > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and > > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). > > Removing the pixclk polarity, the rest we can use as a basis for a new > > subdev-based implementation. > > Don't we need to move this out from soc_camera and make it available in > v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI > support) > > That way both SoC_Camera, and Media Controller frameworks can use that. -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
> -Original Message- > From: Hans Verkuil [mailto:hansv...@cisco.com] > Sent: Wednesday, February 23, 2011 3:32 AM > To: Hans Verkuil > Cc: Sylwester Nawrocki; Guennadi Liakhovetski; Stan; linux- > me...@vger.kernel.org; Laurent Pinchart; Aguirre, Sergio > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms > > On Wednesday, February 23, 2011 09:10:42 Hans Verkuil wrote: > > Unfortunately, if a subdev is set to 'sample at rising edge', then that > does > > not necessarily mean that the host should sample at the same edge. > Depending > > on the clock line routing and the integrity of the clock signal the host > may > > actually have to sample on the other edge. And yes, I've seen this. > > It might be useful to give some background information regarding the > sampling > edge problems. > > There are two main reasons why the sampling edge can be hardware > dependent. > The first is if the data lines go through an amplifier or something > similar > that will slightly delay the data lines compared to the clock signal. This > can > shift the edge at which you have to sample. > > Actually, this may even be dependent on the clock frequency. I have not > seen > that in real life yet, but it might happen. This will complicate things > even > more since in that case you need to make a callback function in the board > code > that determines the sampling edge based on the clock frequency. I think we > can > ignore that for now, but we do need to keep it in mind. > > The other is the waveform of the clock. For relatively low frequencies > this > will resemble a symmetrical square wave. But for higher frequencies this > more > resembles the bottom waveform in this picture: > > http://myweb.msoe.edu/williamstm/Images/Divider2.jpg > > This is asymmetric so depending on the slopes the sampling edge can make > quite > a difference. > > The higher the clock frequency, the more asymmetric the waveform will > look. I think this a very specific HW problem (Low slew rate), and it's something That should be fixed in HW, or avoided in SW after proper experimentation is done. One example on how to fix this is described in this document: http://www.actel.com/documents/SchmittTrigger_AN.pdf Regards, Sergio > > Regards, > > Hans -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wed, 23 Feb 2011, Hans Verkuil wrote: > On Wednesday, February 23, 2011 15:06:49 Aguirre, Sergio wrote: > > Guennadi and Hans, > > > > > > > > > > The only static data I am concerned about are those that affect signal > > > integrity. > > > > After thinking carefully about this I realized that there is really only > > > one > > > > setting that is relevant to that: the sampling edge. The polarities do > > > not > > > > matter in this. > > > > I respectfully disagree. > > > > AFAIK, There is not such thing as sampling edge configuration for MIPI > > Receivers, and the polarities DO matter, since it's a differential > > signal. > > The polarities do not matter for a standard parallel bus. I cannot speak for > MIPI or CSI busses as I have no experience there. So if you say that > polarities matter for MIPI, then for MIPI those should be specified > statically > as well. Do I misunderstand? I interpreted Hans' proposal as: clock edge sensitivity is critical mainly because of high frequency, at which the signal integrity is harder to maintain, and therefore we cannot rely on automagic. Whereas sync signals are much lower frequency, and therefore any breakage would be easier to detect. I don't otherwise understand what "polarities do not matter" mean - of course they do. What am I missing? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday, February 23, 2011 15:06:49 Aguirre, Sergio wrote: > Guennadi and Hans, > > > > > > The only static data I am concerned about are those that affect signal > > integrity. > > > After thinking carefully about this I realized that there is really only > > one > > > setting that is relevant to that: the sampling edge. The polarities do > > not > > > matter in this. > > I respectfully disagree. > > AFAIK, There is not such thing as sampling edge configuration for MIPI > Receivers, and the polarities DO matter, since it's a differential > signal. The polarities do not matter for a standard parallel bus. I cannot speak for MIPI or CSI busses as I have no experience there. So if you say that polarities matter for MIPI, then for MIPI those should be specified statically as well. > > > > > Ok, this is much better! I'm still not perfectly happy having to punish > > all just for the sake of a couple of broken boards, but I can certainly > > much better live with this, than with having to hard-code each and every > > bit. Thanks, Hans! > > > > So, I think, we can proceed with this, let's see the code now, shall we?;) > > > > Currently soc-camera auto-configures the following parameters: > > > > hsync polarity > > vsync polarity > > data polarity > > master / slave mode > > data bus width > > pixel clock polarity > > > > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and > > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). > > Removing the pixclk polarity, the rest we can use as a basis for a new > > subdev-based implementation. > > Don't we need to move this out from soc_camera and make it available in > v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI > support) > > That way both SoC_Camera, and Media Controller frameworks can use that. I believe that is the plan, yes. Regards, Hans > > Regards, > Sergio > > > > > Thanks > > Guennadi > > > > > Unfortunately, if a subdev is set to 'sample at rising edge', then that > > does > > > not necessarily mean that the host should sample at the same edge. > > Depending > > > on the clock line routing and the integrity of the clock signal the host > > may > > > actually have to sample on the other edge. And yes, I've seen this. > > > > > > Anyway, this has been discussed to death already. I am very much opposed > > to > > > negotiating the sampling edge. During the Helsinki meeting in June last > > year > > > we decided to do this via platform data (see section 7 in the meeting > > > minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab). > > > > > > I will formally NACK attempts to negotiate this. Mauro is of course free > > to > > > override me. > > > > > > Something simple like this for subdev platform_data might be enough: > > > > > > struct v4l2_bus_config { > > > /* 0 - sample at falling edge, 1 - sample at rising edge */ > > > unsigned edge_pclock:1; > > > /* 0 - host should use the same sampling edge, 1 - host should > > use the > > >other sampling edge */ > > > unsigned host_invert_edge_pclock:1; > > > }; > > > > > > The host can query the bus configuration and the subdev will return: > > > > > > edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock; > > > > > > We might want to add bits as well to describe whether polarities are > > inverted. > > > > > > This old RFC gives a good overview of the possible polarities: > > > > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html > > > > > > Regards, > > > > > > Hans > > > > > > > I personally like the Stanimir's proposal as the parameters to be > > negotiated > > > > are pretty dynamic. Only the number of lanes could be problematic as > > not all > > > > lanes might be routed across different boards. Perhaps we should > > consider specifying > > > > an AUTO value for some negotiated parameters. Such as in case of an > > attribute that > > > > need to be fixed on some boards or can be fully negotiated on others, > > a fixed > > > > value or "auto" could be respectively set up in the host's > > platform_data. This could > > > > be used to override some parameters in the host driver if needed. > > > > > > > > IMHO, as long as we negotiate only dynamic parameters there should be > > no special > > > > issues. > > > > > > > > Regards, > > > > Sylwester > > > > > > > > > about this if it wasn't for the fact that soc-camera doesn't do this > > but instead > > > > > negotiates it. Obviously, it isn't a pleasant prospect having to > > change all that. > > > > > > > > > > Normally this would be enough of an argument for me to just > > negotiate it. The > > > > > reason that I don't want this in this particular case is that I know > > from > > > > > personal experience that incorrect settings can be extremely hard to > > find. > > > > > > > > > > I also think that there is a reasonable chance that such bugs can > > happen. Take > > > > > a scen
RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Guennadi and Hans, > > The only static data I am concerned about are those that affect signal > integrity. > > After thinking carefully about this I realized that there is really only > one > > setting that is relevant to that: the sampling edge. The polarities do > not > > matter in this. I respectfully disagree. AFAIK, There is not such thing as sampling edge configuration for MIPI Receivers, and the polarities DO matter, since it's a differential signal. > > Ok, this is much better! I'm still not perfectly happy having to punish > all just for the sake of a couple of broken boards, but I can certainly > much better live with this, than with having to hard-code each and every > bit. Thanks, Hans! > > So, I think, we can proceed with this, let's see the code now, shall we?;) > > Currently soc-camera auto-configures the following parameters: > > hsync polarity > vsync polarity > data polarity > master / slave mode > data bus width > pixel clock polarity > > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). > Removing the pixclk polarity, the rest we can use as a basis for a new > subdev-based implementation. Don't we need to move this out from soc_camera and make it available in v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI support) That way both SoC_Camera, and Media Controller frameworks can use that. Regards, Sergio > > Thanks > Guennadi > > > Unfortunately, if a subdev is set to 'sample at rising edge', then that > does > > not necessarily mean that the host should sample at the same edge. > Depending > > on the clock line routing and the integrity of the clock signal the host > may > > actually have to sample on the other edge. And yes, I've seen this. > > > > Anyway, this has been discussed to death already. I am very much opposed > to > > negotiating the sampling edge. During the Helsinki meeting in June last > year > > we decided to do this via platform data (see section 7 in the meeting > > minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab). > > > > I will formally NACK attempts to negotiate this. Mauro is of course free > to > > override me. > > > > Something simple like this for subdev platform_data might be enough: > > > > struct v4l2_bus_config { > > /* 0 - sample at falling edge, 1 - sample at rising edge */ > > unsigned edge_pclock:1; > > /* 0 - host should use the same sampling edge, 1 - host should > use the > >other sampling edge */ > > unsigned host_invert_edge_pclock:1; > > }; > > > > The host can query the bus configuration and the subdev will return: > > > > edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock; > > > > We might want to add bits as well to describe whether polarities are > inverted. > > > > This old RFC gives a good overview of the possible polarities: > > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html > > > > Regards, > > > > Hans > > > > > I personally like the Stanimir's proposal as the parameters to be > negotiated > > > are pretty dynamic. Only the number of lanes could be problematic as > not all > > > lanes might be routed across different boards. Perhaps we should > consider specifying > > > an AUTO value for some negotiated parameters. Such as in case of an > attribute that > > > need to be fixed on some boards or can be fully negotiated on others, > a fixed > > > value or "auto" could be respectively set up in the host's > platform_data. This could > > > be used to override some parameters in the host driver if needed. > > > > > > IMHO, as long as we negotiate only dynamic parameters there should be > no special > > > issues. > > > > > > Regards, > > > Sylwester > > > > > > > about this if it wasn't for the fact that soc-camera doesn't do this > but instead > > > > negotiates it. Obviously, it isn't a pleasant prospect having to > change all that. > > > > > > > > Normally this would be enough of an argument for me to just > negotiate it. The > > > > reason that I don't want this in this particular case is that I know > from > > > > personal experience that incorrect settings can be extremely hard to > find. > > > > > > > > I also think that there is a reasonable chance that such bugs can > happen. Take > > > > a scenario like this: someone writes a new host driver. Initially > there is only > > > > support for positive polarity and detection on the rising edge, > because that's > > > > what the current board on which the driver was developed supports. > This is quite > > > > typical for an initial version of a driver. > > > > > > > > Later someone adds support for negative polarity and falling edge. > Suddenly the > > > > polarity negotiation on the previous board results in negative > instead of positive > > > > which was never tested. Now that board starts producing pixel errors > every so > > > > often. And yes, this type of ha
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tuesday 22 February 2011 18:08:40 Guennadi Liakhovetski wrote: > On Tue, 22 Feb 2011, Hans Verkuil wrote: [snip] > > I also think that there is a reasonable chance that such bugs can happen. > > Take a scenario like this: someone writes a new host driver. Initially > > there is only support for positive polarity and detection on the rising > > edge, because that's what the current board on which the driver was > > developed supports. This is quite typical for an initial version of a > > driver. > > > > Later someone adds support for negative polarity and falling edge. > > Suddenly the polarity negotiation on the previous board results in > > negative instead of positive which was never tested. Now that board > > starts producing pixel errors every so often. And yes, this type of > > hardware problems do happen as I know from painful experience. > > > > Problems like this are next to impossible to debug without the aid of an > > oscilloscope, so this isn't like most other bugs that are relatively easy > > to debug. > > Well, this is pretty simple to debug with the help of git bisect, as long > as patches are sufficiently clean and properly broken down into single > topics. It won't be that easy, as the problems might not be easily reproduceable. Changing the auto-negotiation code will require testing all boards that use it, which is something that can't be done by the person submitting the patch. We will get hard to detect (and debug) breakages. > > It is so much easier just to avoid this by putting it in platform data. > > It's simple, unambiguous and above all, unchanging. > > As I said, this all boils down to who does patches and who accepts them > for mainlibe. -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi Sergio, On Tuesday 22 February 2011 15:01:57 Aguirre, Sergio wrote: > On Tuesday, February 22, 2011 7:33 AM Hans Verkuil wrote: > > On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote: > > > On Tue, 22 Feb 2011, Stanimir Varbanov wrote: > > > > This RFC patch adds a new subdev sensor operation named > > > > g_interface_parms. > > > > > > > > It is planned as a not mandatory operation and it is driver's > > > > developer decision to use it or not. > > > > > > > > Please share your opinions and ideas. > > > > Stanimir, thanks for the RFC. I think it is time that we create a good > > solution for this. This is currently the last remaining issue preventing > > soc-camera subdevs from being used generally. (Control handling is also > > still special, but this is being worked on.) > > > > > Yes, I like the idea in principle (/me pulling his bullet-proof vest > > > on), :-) : > > > as some of you might guess, because I feel it's going away from the > > > idea, that I've been hard pressed to accept of hard-coding the media-bus > > > configuration and in the direction of direct communication of > > > bus-parameters between the (sub-)devices, e.g., a camera host and a > > > camera device in soc-camera terminology. > > > > > > But before reviewing the patch as such, I'd like to discuss the > > > strategy, that we want to pursue here - what exactly do we want to > > > hard-code and what we want to configure dynamically? As explained > > > before, my preference would be to only specify the absolute minimum in > > > the platform data, i.e., parameters that either are ambiguous or special > > > for this platform. So, once again, my approach to configure interface > > > parameters like signal polarities and edge sensitivity is: > > > > > > 1. if at least one side has a fixed value of the specific parameter, > > > usually no need to specify it in platform data. Example: sensor only > > > supports HSYNC active high, host supports both, normally "high" should > > > be selected. > > > > > > 2. as above, but there's an inverter on the board in the signal path. > > > The "invert" parameter must be specified in the platform data and the > > > host will configure itself to "low" and send "high" confirmed to the > > > sensor. > > > > > > 3. both are configurable. In this case the platform data has to > > > specify, which polarity shall be used. > > > > > > This is simple, it is implemented, it has worked that way with no > > > problem for several years now. > > > > > > The configuration procedure in this case looks like: > > > > > > 1. host requests supported interface configurations from the client > > > (sensor) > > > > > > 2. host matches returned parameters against platform data and its own > > > capabilities > > > > > > 3. if no suitable configuration possible - error out > > > > > > 4. the single possible configuration is identified and sent to the > > > sensor back for its configuration > > > > > > This way we need one more method: s_interface_parms. > > > > > > Shortly talking to Laurent earlier today privately, he mentioned, that > > > one of the reasons for this move is to support dynamic bus > > > reconfiguration, e.g., the number of used CSI lanes. The same is useful > > > for parallel interfaces. E.g., I had to hack the omap3spi driver to > > > capture only 8 (parallel) data lanes from the sensor, connected with all > > > its 10 lanes to get a format, easily supported by user-space > > > applications. Ideally you don't want to change anything in the code for > > > this. If the user is requesting the 10-bit format, all 10 lanes are > > > used, if only 8 - the interface is reconfigured accordingly. > > > > I have no problems with dynamic bus reconfiguration as such. So if the > > host driver wants to do lane reconfiguration, then that's fine by me. > > > > When it comes to signal integrity (polarity, rising/falling edge), then I > > remain convinced that this should be set via platform data. This is not > > something that should be negotiated since this depends not only on the > > sensor and host devices, but also on the routing of the lines between them > > on the actual board, how much noise there is on those lines, the quality > > of the clock signal, etc. Not really an issue with PAL/NTSC type signals, > > but when you get to 1080p60 and up, then such things become much more > > important. > > > > So these settings should not be negotiated, but set explicitly. > > > > It actually doesn't have to be done through platform data (although that > > makes the most sense), as long as it is explicitly set based on board- > > specific data. I agree with Hans here. I don't think negotiation is a good idea. > My 2 cents here is that I think this consists in 2 parts, and should be > divided properly. > > 1. You know required # of lanes & clockspeed after you had set the format. > For example: > > - VGA @ 30fps > DDR Clk: 330 MHz > Number of Datalanes: 1 > > - VGA @ 60fps > DDR C
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi Guennadi, On Tuesday 22 February 2011 12:40:32 Guennadi Liakhovetski wrote: > On Tue, 22 Feb 2011, Stanimir Varbanov wrote: > > This RFC patch adds a new subdev sensor operation named > > g_interface_parms. It is planned as a not mandatory operation and it is > > driver's developer decision to use it or not. > > > > Please share your opinions and ideas. [snip] (answering this in another e-mail) > Shortly talking to Laurent earlier today privately, he mentioned, that one > of the reasons for this move is to support dynamic bus reconfiguration, > e.g., the number of used CSI lanes. The same is useful for parallel > interfaces. E.g., I had to hack the omap3spi driver to capture only 8 > (parallel) data lanes from the sensor, connected with all its 10 lanes to > get a format, easily supported by user-space applications. Ideally you > don't want to change anything in the code for this. If the user is > requesting the 10-bit format, all 10 lanes are used, if only 8 - the > interface is reconfigured accordingly. This should indeed be supported out-of-the-box. I'm waiting for Hans' opinion on this, but the idea is that the user should configure a 10-bit format at the sensor output and an 8-bit format at the CCDC input to capture 8-bit data. -- 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: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wed, 23 Feb 2011, Hans Verkuil wrote: > On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > > Hi everybody, > > > > On 02/22/2011 06:00 PM, Hans Verkuil wrote: > > > On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote: > > >> On Tue, 22 Feb 2011, Stan wrote: > > >> > > >>> In principle I agree with this bus negotiation. > > >>> > > >>> - So. let's start thinking how this could be fit to the subdev sensor > > >>> operations. > > >> > > >> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to > > >> start designing interfaces;) > > >> > > >>> - howto isolate your current work into some common place and reuse it, > > >>> even on platform part. > > >>> - and is it possible. > > >>> > > >>> The discussion becomes very emotional and this is not a good adviser :) > > >> > > >> No, no emotions at least on this side:) But it's also not technical, > > >> unfortunately. I'm prepared to discuss technical benefits or drawbacks of > > >> each of these approaches, but these arguments - can we trust programmers > > >> or can we not? or will anyone at some time in the future break it or not? > > >> Sorry, I am not a psychologist:) Personally, I would _exclusively_ > > >> consider technical arguments. Of course, things like "clean and simple > > >> APIs," "proper separation / layering" etc. are also important, but even > > >> they already can become difficult to discuss and are already on the > > >> border > > >> between technical issues and personal preferences... So, don't know, in > > >> the end, I think, it will just come down to who is making decisions and > > >> who is implementing them:) I just expressed my opinion, we don't have to > > >> agree, eventually, the maintainer will decide whether to apply patches or > > >> not:) > > > > > > In my view at least it *is* a technical argument. It makes perfect sense > > > to > > > me from a technical point of view to put static, board-specific > > > configuration > > > in platform_data. I don't think there would have been much, if any, > > > discussion > > > > We should not be forgetting that there often will be two or more sets > > of platform_data. For sensor, MIPI interface, for the host interface > > driver.. > > By negotiating setups we could avoid situations when corresponding > > parameters > > are not matched. That is not so meaningful benefit though. > > > > Clock values are often being rounded at runtime and do not always reflect > > exactly > > the numbers fixed at compile time. And negotiation could help to obtain > > exact > > values at both sensor and host side. > > The only static data I am concerned about are those that affect signal > integrity. > After thinking carefully about this I realized that there is really only one > setting that is relevant to that: the sampling edge. The polarities do not > matter in this. Ok, this is much better! I'm still not perfectly happy having to punish all just for the sake of a couple of broken boards, but I can certainly much better live with this, than with having to hard-code each and every bit. Thanks, Hans! So, I think, we can proceed with this, let's see the code now, shall we?;) Currently soc-camera auto-configures the following parameters: hsync polarity vsync polarity data polarity master / slave mode data bus width pixel clock polarity (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). Removing the pixclk polarity, the rest we can use as a basis for a new subdev-based implementation. Thanks Guennadi > Unfortunately, if a subdev is set to 'sample at rising edge', then that does > not necessarily mean that the host should sample at the same edge. Depending > on the clock line routing and the integrity of the clock signal the host may > actually have to sample on the other edge. And yes, I've seen this. > > Anyway, this has been discussed to death already. I am very much opposed to > negotiating the sampling edge. During the Helsinki meeting in June last year > we decided to do this via platform data (see section 7 in the meeting > minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab). > > I will formally NACK attempts to negotiate this. Mauro is of course free to > override me. > > Something simple like this for subdev platform_data might be enough: > > struct v4l2_bus_config { > /* 0 - sample at falling edge, 1 - sample at rising edge */ > unsigned edge_pclock:1; > /* 0 - host should use the same sampling edge, 1 - host should use the >other sampling edge */ > unsigned host_invert_edge_pclock:1; > }; > > The host can query the bus configuration and the subdev will return: > > edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock; > > We might want to add bits as well to describe whether polarities are inverted. > > This old RFC gives a good overview of the possible
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Wednesday, February 23, 2011 09:10:42 Hans Verkuil wrote: > Unfortunately, if a subdev is set to 'sample at rising edge', then that does > not necessarily mean that the host should sample at the same edge. Depending > on the clock line routing and the integrity of the clock signal the host may > actually have to sample on the other edge. And yes, I've seen this. It might be useful to give some background information regarding the sampling edge problems. There are two main reasons why the sampling edge can be hardware dependent. The first is if the data lines go through an amplifier or something similar that will slightly delay the data lines compared to the clock signal. This can shift the edge at which you have to sample. Actually, this may even be dependent on the clock frequency. I have not seen that in real life yet, but it might happen. This will complicate things even more since in that case you need to make a callback function in the board code that determines the sampling edge based on the clock frequency. I think we can ignore that for now, but we do need to keep it in mind. The other is the waveform of the clock. For relatively low frequencies this will resemble a symmetrical square wave. But for higher frequencies this more resembles the bottom waveform in this picture: http://myweb.msoe.edu/williamstm/Images/Divider2.jpg This is asymmetric so depending on the slopes the sampling edge can make quite a difference. The higher the clock frequency, the more asymmetric the waveform will look. Regards, Hans -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote: > Hi everybody, > > On 02/22/2011 06:00 PM, Hans Verkuil wrote: > > On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote: > >> On Tue, 22 Feb 2011, Stan wrote: > >> > >>> In principle I agree with this bus negotiation. > >>> > >>> - So. let's start thinking how this could be fit to the subdev sensor > >>> operations. > >> > >> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to > >> start designing interfaces;) > >> > >>> - howto isolate your current work into some common place and reuse it, > >>> even on platform part. > >>> - and is it possible. > >>> > >>> The discussion becomes very emotional and this is not a good adviser :) > >> > >> No, no emotions at least on this side:) But it's also not technical, > >> unfortunately. I'm prepared to discuss technical benefits or drawbacks of > >> each of these approaches, but these arguments - can we trust programmers > >> or can we not? or will anyone at some time in the future break it or not? > >> Sorry, I am not a psychologist:) Personally, I would _exclusively_ > >> consider technical arguments. Of course, things like "clean and simple > >> APIs," "proper separation / layering" etc. are also important, but even > >> they already can become difficult to discuss and are already on the border > >> between technical issues and personal preferences... So, don't know, in > >> the end, I think, it will just come down to who is making decisions and > >> who is implementing them:) I just expressed my opinion, we don't have to > >> agree, eventually, the maintainer will decide whether to apply patches or > >> not:) > > > > In my view at least it *is* a technical argument. It makes perfect sense to > > me from a technical point of view to put static, board-specific > > configuration > > in platform_data. I don't think there would have been much, if any, > > discussion > > We should not be forgetting that there often will be two or more sets > of platform_data. For sensor, MIPI interface, for the host interface driver.. > By negotiating setups we could avoid situations when corresponding parameters > are not matched. That is not so meaningful benefit though. > > Clock values are often being rounded at runtime and do not always reflect > exactly > the numbers fixed at compile time. And negotiation could help to obtain exact > values at both sensor and host side. The only static data I am concerned about are those that affect signal integrity. After thinking carefully about this I realized that there is really only one setting that is relevant to that: the sampling edge. The polarities do not matter in this. Unfortunately, if a subdev is set to 'sample at rising edge', then that does not necessarily mean that the host should sample at the same edge. Depending on the clock line routing and the integrity of the clock signal the host may actually have to sample on the other edge. And yes, I've seen this. Anyway, this has been discussed to death already. I am very much opposed to negotiating the sampling edge. During the Helsinki meeting in June last year we decided to do this via platform data (see section 7 in the meeting minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab). I will formally NACK attempts to negotiate this. Mauro is of course free to override me. Something simple like this for subdev platform_data might be enough: struct v4l2_bus_config { /* 0 - sample at falling edge, 1 - sample at rising edge */ unsigned edge_pclock:1; /* 0 - host should use the same sampling edge, 1 - host should use the other sampling edge */ unsigned host_invert_edge_pclock:1; }; The host can query the bus configuration and the subdev will return: edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock; We might want to add bits as well to describe whether polarities are inverted. This old RFC gives a good overview of the possible polarities: http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html Regards, Hans > I personally like the Stanimir's proposal as the parameters to be negotiated > are pretty dynamic. Only the number of lanes could be problematic as not all > lanes might be routed across different boards. Perhaps we should consider > specifying > an AUTO value for some negotiated parameters. Such as in case of an attribute > that > need to be fixed on some boards or can be fully negotiated on others, a fixed > value or "auto" could be respectively set up in the host's platform_data. > This could > be used to override some parameters in the host driver if needed. > > IMHO, as long as we negotiate only dynamic parameters there should be no > special > issues. > > Regards, > Sylwester > > > about this if it wasn't for the fact that soc-camera doesn't do this but > > instead > > negotiates it. Obviously, it isn't a pleasant prospect having to change all >
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi everybody, On 02/22/2011 06:00 PM, Hans Verkuil wrote: > On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote: >> On Tue, 22 Feb 2011, Stan wrote: >> >>> In principle I agree with this bus negotiation. >>> >>> - So. let's start thinking how this could be fit to the subdev sensor >>> operations. >> >> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to >> start designing interfaces;) >> >>> - howto isolate your current work into some common place and reuse it, >>> even on platform part. >>> - and is it possible. >>> >>> The discussion becomes very emotional and this is not a good adviser :) >> >> No, no emotions at least on this side:) But it's also not technical, >> unfortunately. I'm prepared to discuss technical benefits or drawbacks of >> each of these approaches, but these arguments - can we trust programmers >> or can we not? or will anyone at some time in the future break it or not? >> Sorry, I am not a psychologist:) Personally, I would _exclusively_ >> consider technical arguments. Of course, things like "clean and simple >> APIs," "proper separation / layering" etc. are also important, but even >> they already can become difficult to discuss and are already on the border >> between technical issues and personal preferences... So, don't know, in >> the end, I think, it will just come down to who is making decisions and >> who is implementing them:) I just expressed my opinion, we don't have to >> agree, eventually, the maintainer will decide whether to apply patches or >> not:) > > In my view at least it *is* a technical argument. It makes perfect sense to > me from a technical point of view to put static, board-specific configuration > in platform_data. I don't think there would have been much, if any, discussion We should not be forgetting that there often will be two or more sets of platform_data. For sensor, MIPI interface, for the host interface driver.. By negotiating setups we could avoid situations when corresponding parameters are not matched. That is not so meaningful benefit though. Clock values are often being rounded at runtime and do not always reflect exactly the numbers fixed at compile time. And negotiation could help to obtain exact values at both sensor and host side. I personally like the Stanimir's proposal as the parameters to be negotiated are pretty dynamic. Only the number of lanes could be problematic as not all lanes might be routed across different boards. Perhaps we should consider specifying an AUTO value for some negotiated parameters. Such as in case of an attribute that need to be fixed on some boards or can be fully negotiated on others, a fixed value or "auto" could be respectively set up in the host's platform_data. This could be used to override some parameters in the host driver if needed. IMHO, as long as we negotiate only dynamic parameters there should be no special issues. Regards, Sylwester > about this if it wasn't for the fact that soc-camera doesn't do this but > instead > negotiates it. Obviously, it isn't a pleasant prospect having to change all > that. > > Normally this would be enough of an argument for me to just negotiate it. The > reason that I don't want this in this particular case is that I know from > personal experience that incorrect settings can be extremely hard to find. > > I also think that there is a reasonable chance that such bugs can happen. Take > a scenario like this: someone writes a new host driver. Initially there is > only > support for positive polarity and detection on the rising edge, because that's > what the current board on which the driver was developed supports. This is > quite > typical for an initial version of a driver. > > Later someone adds support for negative polarity and falling edge. Suddenly > the > polarity negotiation on the previous board results in negative instead of > positive > which was never tested. Now that board starts producing pixel errors every so > often. And yes, this type of hardware problems do happen as I know from > painful > experience. > > Problems like this are next to impossible to debug without the aid of an > oscilloscope, so this isn't like most other bugs that are relatively easy to > debug. > > It is so much easier just to avoid this by putting it in platform data. It's > simple, unambiguous and above all, unchanging. > > Regards, > > Hans > >> >> 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 majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tue, 22 Feb 2011, Hans Verkuil wrote: > On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote: > > On Tue, 22 Feb 2011, Stan wrote: > > > > > In principle I agree with this bus negotiation. > > > > > > - So. let's start thinking how this could be fit to the subdev sensor > > > operations. > > > > Well, I'm afraid not everyone is convinced yet, so, it is a bit early to > > start designing interfaces;) > > > > > - howto isolate your current work into some common place and reuse it, > > > even on platform part. > > > - and is it possible. > > > > > > The discussion becomes very emotional and this is not a good adviser :) > > > > No, no emotions at least on this side:) But it's also not technical, > > unfortunately. I'm prepared to discuss technical benefits or drawbacks of > > each of these approaches, but these arguments - can we trust programmers > > or can we not? or will anyone at some time in the future break it or not? > > Sorry, I am not a psychologist:) Personally, I would _exclusively_ > > consider technical arguments. Of course, things like "clean and simple > > APIs," "proper separation / layering" etc. are also important, but even > > they already can become difficult to discuss and are already on the border > > between technical issues and personal preferences... So, don't know, in > > the end, I think, it will just come down to who is making decisions and > > who is implementing them:) I just expressed my opinion, we don't have to > > agree, eventually, the maintainer will decide whether to apply patches or > > not:) > > In my view at least it *is* a technical argument. It makes perfect sense to > me from a technical point of view to put static, board-specific configuration > in platform_data. I don't think there would have been much, if any, discussion > about this if it wasn't for the fact that soc-camera doesn't do this but > instead > negotiates it. Obviously, it isn't a pleasant prospect having to change all > that. > > Normally this would be enough of an argument for me to just negotiate it. The > reason that I don't want this in this particular case is that I know from > personal experience that incorrect settings can be extremely hard to find. > > I also think that there is a reasonable chance that such bugs can happen. Take > a scenario like this: someone writes a new host driver. Initially there is > only > support for positive polarity and detection on the rising edge, because that's > what the current board on which the driver was developed supports. This is > quite > typical for an initial version of a driver. > > Later someone adds support for negative polarity and falling edge. Suddenly > the > polarity negotiation on the previous board results in negative instead of > positive > which was never tested. Now that board starts producing pixel errors every so > often. And yes, this type of hardware problems do happen as I know from > painful > experience. > > Problems like this are next to impossible to debug without the aid of an > oscilloscope, so this isn't like most other bugs that are relatively easy to > debug. Well, this is pretty simple to debug with the help of git bisect, as long as patches are sufficiently clean and properly broken down into single topics. > It is so much easier just to avoid this by putting it in platform data. It's > simple, unambiguous and above all, unchanging. As I said, this all boils down to who does patches and who accepts them for mainlibe. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote: > On Tue, 22 Feb 2011, Stan wrote: > > > In principle I agree with this bus negotiation. > > > > - So. let's start thinking how this could be fit to the subdev sensor > > operations. > > Well, I'm afraid not everyone is convinced yet, so, it is a bit early to > start designing interfaces;) > > > - howto isolate your current work into some common place and reuse it, > > even on platform part. > > - and is it possible. > > > > The discussion becomes very emotional and this is not a good adviser :) > > No, no emotions at least on this side:) But it's also not technical, > unfortunately. I'm prepared to discuss technical benefits or drawbacks of > each of these approaches, but these arguments - can we trust programmers > or can we not? or will anyone at some time in the future break it or not? > Sorry, I am not a psychologist:) Personally, I would _exclusively_ > consider technical arguments. Of course, things like "clean and simple > APIs," "proper separation / layering" etc. are also important, but even > they already can become difficult to discuss and are already on the border > between technical issues and personal preferences... So, don't know, in > the end, I think, it will just come down to who is making decisions and > who is implementing them:) I just expressed my opinion, we don't have to > agree, eventually, the maintainer will decide whether to apply patches or > not:) In my view at least it *is* a technical argument. It makes perfect sense to me from a technical point of view to put static, board-specific configuration in platform_data. I don't think there would have been much, if any, discussion about this if it wasn't for the fact that soc-camera doesn't do this but instead negotiates it. Obviously, it isn't a pleasant prospect having to change all that. Normally this would be enough of an argument for me to just negotiate it. The reason that I don't want this in this particular case is that I know from personal experience that incorrect settings can be extremely hard to find. I also think that there is a reasonable chance that such bugs can happen. Take a scenario like this: someone writes a new host driver. Initially there is only support for positive polarity and detection on the rising edge, because that's what the current board on which the driver was developed supports. This is quite typical for an initial version of a driver. Later someone adds support for negative polarity and falling edge. Suddenly the polarity negotiation on the previous board results in negative instead of positive which was never tested. Now that board starts producing pixel errors every so often. And yes, this type of hardware problems do happen as I know from painful experience. Problems like this are next to impossible to debug without the aid of an oscilloscope, so this isn't like most other bugs that are relatively easy to debug. It is so much easier just to avoid this by putting it in platform data. It's simple, unambiguous and above all, unchanging. Regards, Hans > > 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tue, 22 Feb 2011, Stan wrote: > In principle I agree with this bus negotiation. > > - So. let's start thinking how this could be fit to the subdev sensor > operations. Well, I'm afraid not everyone is convinced yet, so, it is a bit early to start designing interfaces;) > - howto isolate your current work into some common place and reuse it, > even on platform part. > - and is it possible. > > The discussion becomes very emotional and this is not a good adviser :) No, no emotions at least on this side:) But it's also not technical, unfortunately. I'm prepared to discuss technical benefits or drawbacks of each of these approaches, but these arguments - can we trust programmers or can we not? or will anyone at some time in the future break it or not? Sorry, I am not a psychologist:) Personally, I would _exclusively_ consider technical arguments. Of course, things like "clean and simple APIs," "proper separation / layering" etc. are also important, but even they already can become difficult to discuss and are already on the border between technical issues and personal preferences... So, don't know, in the end, I think, it will just come down to who is making decisions and who is implementing them:) I just expressed my opinion, we don't have to agree, eventually, the maintainer will decide whether to apply patches or not:) 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi, Guennadi Liakhovetski wrote: > On Tue, 22 Feb 2011, Hans Verkuil wrote: > >> On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote: >>> On Tue, 22 Feb 2011, Stanimir Varbanov wrote: >>> This RFC patch adds a new subdev sensor operation named g_interface_parms. It is planned as a not mandatory operation and it is driver's developer decision to use it or not. Please share your opinions and ideas. >> Stanimir, thanks for the RFC. I think it is time that we create a good >> solution for this. This is currently the last remaining issue preventing soc- >> camera subdevs from being used generally. (Control handling is also still >> special, but this is being worked on.) >> >>> Yes, I like the idea in principle (/me pulling his bullet-proof vest on), >> :-) >> >>> as some of you might guess, because I feel it's going away from the idea, >>> that I've been hard pressed to accept of hard-coding the media-bus >>> configuration and in the direction of direct communication of >>> bus-parameters between the (sub-)devices, e.g., a camera host and a camera >>> device in soc-camera terminology. >>> >>> But before reviewing the patch as such, I'd like to discuss the strategy, >>> that we want to pursue here - what exactly do we want to hard-code and >>> what we want to configure dynamically? As explained before, my preference >>> would be to only specify the absolute minimum in the platform data, i.e., >>> parameters that either are ambiguous or special for this platform. So, >>> once again, my approach to configure interface parameters like signal >>> polarities and edge sensitivity is: >>> >>> 1. if at least one side has a fixed value of the specific parameter, >>> usually no need to specify it in platform data. Example: sensor only >>> supports HSYNC active high, host supports both, normally "high" should be >>> selected. >>> >>> 2. as above, but there's an inverter on the board in the signal path. The >>> "invert" parameter must be specified in the platform data and the host >>> will configure itself to "low" and send "high" confirmed to the sensor. >>> >>> 3. both are configurable. In this case the platform data has to specify, >>> which polarity shall be used. >>> >>> This is simple, it is implemented, it has worked that way with no problem >>> for several years now. >>> >>> The configuration procedure in this case looks like: >>> >>> 1. host requests supported interface configurations from the client >>> (sensor) >>> >>> 2. host matches returned parameters against platform data and its own >>> capabilities >>> >>> 3. if no suitable configuration possible - error out >>> >>> 4. the single possible configuration is identified and sent to the sensor >>> back for its configuration >>> >>> This way we need one more method: s_interface_parms. >>> >>> Shortly talking to Laurent earlier today privately, he mentioned, that one >>> of the reasons for this move is to support dynamic bus reconfiguration, >>> e.g., the number of used CSI lanes. The same is useful for parallel >>> interfaces. E.g., I had to hack the omap3spi driver to capture only 8 >>> (parallel) data lanes from the sensor, connected with all its 10 lanes to >>> get a format, easily supported by user-space applications. Ideally you >>> don't want to change anything in the code for this. If the user is >>> requesting the 10-bit format, all 10 lanes are used, if only 8 - the >>> interface is reconfigured accordingly. >> I have no problems with dynamic bus reconfiguration as such. So if the host >> driver wants to do lane reconfiguration, then that's fine by me. >> >> When it comes to signal integrity (polarity, rising/falling edge), then I >> remain convinced that this should be set via platform data. This is not >> something that should be negotiated since this depends not only on the >> sensor >> and host devices, but also on the routing of the lines between them on the >> actual board, how much noise there is on those lines, the quality of the >> clock >> signal, etc. Not really an issue with PAL/NTSC type signals, but when you >> get >> to 1080p60 and up, then such things become much more important. > I think this could be satisfied by Guennadi's approach because he use the platform data with preference. > I understand this, but my point is: forcing this parameters in the > platform data doesn't give you any _practical_ enhancements, only > _psychological_, meaning, that you think, that if these parameters are > compulsory, programmers, writing board integration code, will be forced to > think, what values to configure. Whereas if this is not compulsory, > programmers will hope on automagic and things will break. So, this is > purely psychological. And that's the whole question - fo we trust > programmers, that they will anyway take care to set correct parameters, or > do we not trust them and therefore want to punish everyone because of > them. Besides, I'm pretty
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tue, 22 Feb 2011, Hans Verkuil wrote: > Secondly, if we rely on negotiations, then someone at some time might change > things and suddenly the negotiation gives different results which may not > work > on some boards. And such bugs can be extremely hard to track down. So that is Sorry, there's always a chance, that someone breaks something. Always when changing central algorithms you have to take care, that behaviour doesn't change on existing configurations. Nothing new there. > why I don't want to rely on negotiations of these settings. People are free > to > copy and paste, though. I assume (and hope) that they will test before > sending > a patch, so if it works with the copy-and-pasted settings, then that's good > enough for me. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tuesday, February 22, 2011 15:11:49 Guennadi Liakhovetski wrote: > On Tue, 22 Feb 2011, Hans Verkuil wrote: > > I have no problems with dynamic bus reconfiguration as such. So if the host > > driver wants to do lane reconfiguration, then that's fine by me. > > > > When it comes to signal integrity (polarity, rising/falling edge), then I > > remain convinced that this should be set via platform data. This is not > > something that should be negotiated since this depends not only on the sensor > > and host devices, but also on the routing of the lines between them on the > > actual board, how much noise there is on those lines, the quality of the clock > > signal, etc. Not really an issue with PAL/NTSC type signals, but when you get > > to 1080p60 and up, then such things become much more important. > > I understand this, but my point is: forcing this parameters in the > platform data doesn't give you any _practical_ enhancements, only > _psychological_, meaning, that you think, that if these parameters are > compulsory, programmers, writing board integration code, will be forced to > think, what values to configure. Whereas if this is not compulsory, > programmers will hope on automagic and things will break. So, this is > purely psychological. And that's the whole question - fo we trust > programmers, that they will anyway take care to set correct parameters, or > do we not trust them and therefore want to punish everyone because of > them. Besides, I'm pretty convinced, that even if those parameters will be > compulsory, most programmers will anyway just copy-paste them from > "similar" set ups... First of all, I don't trust myself, let alone someone else :-) So forcing people to think about it is a good thing in my mind. Secondly, if we rely on negotiations, then someone at some time might change things and suddenly the negotiation gives different results which may not work on some boards. And such bugs can be extremely hard to track down. So that is why I don't want to rely on negotiations of these settings. People are free to copy and paste, though. I assume (and hope) that they will test before sending a patch, so if it works with the copy-and-pasted settings, then that's good enough for me. Regards, Hans > > Thanks > Guennadi > > > So these settings should not be negotiated, but set explicitly. > > > > It actually doesn't have to be done through platform data (although that makes > > the most sense), as long as it is explicitly set based on board-specific data. > > > > Regards, > > > > Hans > > > > > > > > Thanks > > > Guennadi > > > > > > > --- > > > > It tries to create a common API for getting the sensor interface type > > > > - serial or parallel, modes and interface clocks. The interface clocks > > > > then are used in the host side to calculate it's configuration, check > > > > that the clocks are not beyond host limitations etc. > > > > > > > > "phy_rate" in serial interface (CSI DDR clk) is used to calculate > > > > the CSI2 PHY receiver timing parameters: ths_settle, ths_term, > > > > clk_settle and clk_term. > > > > > > > > As the "phy_rate" depends on current sensor mode (configuration of the > > > > sensor's PLL and internal clock domains) it can be treated as dynamic > > > > parameter and can vary (could be different for viewfinder and still > > > > capture), in this context g_interface_parms should be called after > > > > s_fmt. > > > > > > > > "pix_clk" for parallel interface reflects the current sensor pixel > > > > clock. With this clock the image data is clocked out of the sensor. > > > > > > > > "pix_clk" for serial interface reflects the current sensor pixel > > > > clock at which image date is read from sensor matrix. > > > > > > > > "lanes" for serial interface reflects the number of PHY lanes used from > > > > the sensor to output image data. This should be known from the host > > > > side before the streaming is started. For some sensor modes it's > > > > enough to use one lane, for bigger resolutions two lanes and more > > > > are used. > > > > > > > > "channel" for serial interface is also needed from host side to > > > > configure it's PHY receiver at particular virtual channel. > > > > > > > > --- > > > > Some background and inspiration. > > > > > > > > - Currently in the OMAP3 ISP driver we use a set of platform data > > > > callbacks to provide the above parameters and this comes to very > > > > complicated platform code, driver implementation and unneeded > > > > sensor driver <-> host driver dependences. > > > > > > > > - In the present time we seeing growing count of sensor drivers and > > > > host (bridge) drivers but without standard API's for communication. > > > > Currently the subdev sensor operations have only one operation - > > > > g_skip_top_lines. > > > > > > > > Stanimir Varbanov (1): > > > > v4l: Introduce sensor operation for getting interface configuration > > > > > > > >
RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tue, 22 Feb 2011, Aguirre, Sergio wrote: > For example, at least OMAP3 & 4 has the following pin pairs: > > CSI2_DX0, CSI2_DY0 > CSI2_DX1, CSI2_DY1 > CSI2_DX2, CSI2_DY2 > CSI2_DX3, CSI2_DY3 > CSI2_DX4, CSI2_DY4 > > So, what you do is that, you can control where do you want the clock, > where do you want each datalane pair, and also the pin polarity > (X: +, Y: -, or viceversa). And this is something that is static. > THIS I think should go in the host driver's platform data. I think, these are two different things: pin roles - yes, they are SoC-specific and, probably, hard-wired. But once you've assigned roles, you have to configure them - roles, functions, not pins. And that configuration is no longer SoC specific, at least some of the parameters are common to all such set ups - polarities and edges. So, you can use the same set of parameters for them on different platforms. And yes - you have to be able to configure them dynamically. Consider two sensors switching to the same host by means of some board logic. So, at least there have to be multiple parameter sets to use, depending on the connection topology. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tue, 22 Feb 2011, Hans Verkuil wrote: > On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote: > > On Tue, 22 Feb 2011, Stanimir Varbanov wrote: > > > > > This RFC patch adds a new subdev sensor operation named g_interface_parms. > > > It is planned as a not mandatory operation and it is driver's developer > > > decision to use it or not. > > > > > > Please share your opinions and ideas. > > Stanimir, thanks for the RFC. I think it is time that we create a good > solution for this. This is currently the last remaining issue preventing soc- > camera subdevs from being used generally. (Control handling is also still > special, but this is being worked on.) > > > Yes, I like the idea in principle (/me pulling his bullet-proof vest on), > > :-) > > > as some of you might guess, because I feel it's going away from the idea, > > that I've been hard pressed to accept of hard-coding the media-bus > > configuration and in the direction of direct communication of > > bus-parameters between the (sub-)devices, e.g., a camera host and a camera > > device in soc-camera terminology. > > > > But before reviewing the patch as such, I'd like to discuss the strategy, > > that we want to pursue here - what exactly do we want to hard-code and > > what we want to configure dynamically? As explained before, my preference > > would be to only specify the absolute minimum in the platform data, i.e., > > parameters that either are ambiguous or special for this platform. So, > > once again, my approach to configure interface parameters like signal > > polarities and edge sensitivity is: > > > > 1. if at least one side has a fixed value of the specific parameter, > > usually no need to specify it in platform data. Example: sensor only > > supports HSYNC active high, host supports both, normally "high" should be > > selected. > > > > 2. as above, but there's an inverter on the board in the signal path. The > > "invert" parameter must be specified in the platform data and the host > > will configure itself to "low" and send "high" confirmed to the sensor. > > > > 3. both are configurable. In this case the platform data has to specify, > > which polarity shall be used. > > > > This is simple, it is implemented, it has worked that way with no problem > > for several years now. > > > > The configuration procedure in this case looks like: > > > > 1. host requests supported interface configurations from the client > > (sensor) > > > > 2. host matches returned parameters against platform data and its own > > capabilities > > > > 3. if no suitable configuration possible - error out > > > > 4. the single possible configuration is identified and sent to the sensor > > back for its configuration > > > > This way we need one more method: s_interface_parms. > > > > Shortly talking to Laurent earlier today privately, he mentioned, that one > > of the reasons for this move is to support dynamic bus reconfiguration, > > e.g., the number of used CSI lanes. The same is useful for parallel > > interfaces. E.g., I had to hack the omap3spi driver to capture only 8 > > (parallel) data lanes from the sensor, connected with all its 10 lanes to > > get a format, easily supported by user-space applications. Ideally you > > don't want to change anything in the code for this. If the user is > > requesting the 10-bit format, all 10 lanes are used, if only 8 - the > > interface is reconfigured accordingly. > > I have no problems with dynamic bus reconfiguration as such. So if the host > driver wants to do lane reconfiguration, then that's fine by me. > > When it comes to signal integrity (polarity, rising/falling edge), then I > remain convinced that this should be set via platform data. This is not > something that should be negotiated since this depends not only on the sensor > and host devices, but also on the routing of the lines between them on the > actual board, how much noise there is on those lines, the quality of the > clock > signal, etc. Not really an issue with PAL/NTSC type signals, but when you get > to 1080p60 and up, then such things become much more important. I understand this, but my point is: forcing this parameters in the platform data doesn't give you any _practical_ enhancements, only _psychological_, meaning, that you think, that if these parameters are compulsory, programmers, writing board integration code, will be forced to think, what values to configure. Whereas if this is not compulsory, programmers will hope on automagic and things will break. So, this is purely psychological. And that's the whole question - fo we trust programmers, that they will anyway take care to set correct parameters, or do we not trust them and therefore want to punish everyone because of them. Besides, I'm pretty convinced, that even if those parameters will be compulsory, most programmers will anyway just copy-paste them from "similar" set ups... Thanks Guennadi > So t
RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
Hi, > -Original Message- > From: Hans Verkuil [mailto:hansv...@cisco.com] > Sent: Tuesday, February 22, 2011 7:33 AM > To: Guennadi Liakhovetski > Cc: Stanimir Varbanov; linux-media@vger.kernel.org; > laurent.pinch...@ideasonboard.com; Aguirre, Sergio > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms > > On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote: > > On Tue, 22 Feb 2011, Stanimir Varbanov wrote: > > > > > This RFC patch adds a new subdev sensor operation named > g_interface_parms. > > > It is planned as a not mandatory operation and it is driver's > developer > > > decision to use it or not. > > > > > > Please share your opinions and ideas. > > Stanimir, thanks for the RFC. I think it is time that we create a good > solution for this. This is currently the last remaining issue preventing > soc- > camera subdevs from being used generally. (Control handling is also still > special, but this is being worked on.) > > > Yes, I like the idea in principle (/me pulling his bullet-proof vest > on), > > :-) > > > as some of you might guess, because I feel it's going away from the > idea, > > that I've been hard pressed to accept of hard-coding the media-bus > > configuration and in the direction of direct communication of > > bus-parameters between the (sub-)devices, e.g., a camera host and a > camera > > device in soc-camera terminology. > > > > But before reviewing the patch as such, I'd like to discuss the > strategy, > > that we want to pursue here - what exactly do we want to hard-code and > > what we want to configure dynamically? As explained before, my > preference > > would be to only specify the absolute minimum in the platform data, > i.e., > > parameters that either are ambiguous or special for this platform. So, > > once again, my approach to configure interface parameters like signal > > polarities and edge sensitivity is: > > > > 1. if at least one side has a fixed value of the specific parameter, > > usually no need to specify it in platform data. Example: sensor only > > supports HSYNC active high, host supports both, normally "high" should > be > > selected. > > > > 2. as above, but there's an inverter on the board in the signal path. > The > > "invert" parameter must be specified in the platform data and the host > > will configure itself to "low" and send "high" confirmed to the sensor. > > > > 3. both are configurable. In this case the platform data has to specify, > > which polarity shall be used. > > > > This is simple, it is implemented, it has worked that way with no > problem > > for several years now. > > > > The configuration procedure in this case looks like: > > > > 1. host requests supported interface configurations from the client > > (sensor) > > > > 2. host matches returned parameters against platform data and its own > > capabilities > > > > 3. if no suitable configuration possible - error out > > > > 4. the single possible configuration is identified and sent to the > sensor > > back for its configuration > > > > This way we need one more method: s_interface_parms. > > > > Shortly talking to Laurent earlier today privately, he mentioned, that > one > > of the reasons for this move is to support dynamic bus reconfiguration, > > e.g., the number of used CSI lanes. The same is useful for parallel > > interfaces. E.g., I had to hack the omap3spi driver to capture only 8 > > (parallel) data lanes from the sensor, connected with all its 10 lanes > to > > get a format, easily supported by user-space applications. Ideally you > > don't want to change anything in the code for this. If the user is > > requesting the 10-bit format, all 10 lanes are used, if only 8 - the > > interface is reconfigured accordingly. > > I have no problems with dynamic bus reconfiguration as such. So if the > host > driver wants to do lane reconfiguration, then that's fine by me. > > When it comes to signal integrity (polarity, rising/falling edge), then I > remain convinced that this should be set via platform data. This is not > something that should be negotiated since this depends not only on the > sensor > and host devices, but also on the routing of the lines between them on the > actual board, how much noise there is on those lines, the quality of the > clock > signal, etc. Not really an issue with PAL/NTSC type signals, but when you > get >
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote: > On Tue, 22 Feb 2011, Stanimir Varbanov wrote: > > > This RFC patch adds a new subdev sensor operation named g_interface_parms. > > It is planned as a not mandatory operation and it is driver's developer > > decision to use it or not. > > > > Please share your opinions and ideas. Stanimir, thanks for the RFC. I think it is time that we create a good solution for this. This is currently the last remaining issue preventing soc- camera subdevs from being used generally. (Control handling is also still special, but this is being worked on.) > Yes, I like the idea in principle (/me pulling his bullet-proof vest on), :-) > as some of you might guess, because I feel it's going away from the idea, > that I've been hard pressed to accept of hard-coding the media-bus > configuration and in the direction of direct communication of > bus-parameters between the (sub-)devices, e.g., a camera host and a camera > device in soc-camera terminology. > > But before reviewing the patch as such, I'd like to discuss the strategy, > that we want to pursue here - what exactly do we want to hard-code and > what we want to configure dynamically? As explained before, my preference > would be to only specify the absolute minimum in the platform data, i.e., > parameters that either are ambiguous or special for this platform. So, > once again, my approach to configure interface parameters like signal > polarities and edge sensitivity is: > > 1. if at least one side has a fixed value of the specific parameter, > usually no need to specify it in platform data. Example: sensor only > supports HSYNC active high, host supports both, normally "high" should be > selected. > > 2. as above, but there's an inverter on the board in the signal path. The > "invert" parameter must be specified in the platform data and the host > will configure itself to "low" and send "high" confirmed to the sensor. > > 3. both are configurable. In this case the platform data has to specify, > which polarity shall be used. > > This is simple, it is implemented, it has worked that way with no problem > for several years now. > > The configuration procedure in this case looks like: > > 1. host requests supported interface configurations from the client > (sensor) > > 2. host matches returned parameters against platform data and its own > capabilities > > 3. if no suitable configuration possible - error out > > 4. the single possible configuration is identified and sent to the sensor > back for its configuration > > This way we need one more method: s_interface_parms. > > Shortly talking to Laurent earlier today privately, he mentioned, that one > of the reasons for this move is to support dynamic bus reconfiguration, > e.g., the number of used CSI lanes. The same is useful for parallel > interfaces. E.g., I had to hack the omap3spi driver to capture only 8 > (parallel) data lanes from the sensor, connected with all its 10 lanes to > get a format, easily supported by user-space applications. Ideally you > don't want to change anything in the code for this. If the user is > requesting the 10-bit format, all 10 lanes are used, if only 8 - the > interface is reconfigured accordingly. I have no problems with dynamic bus reconfiguration as such. So if the host driver wants to do lane reconfiguration, then that's fine by me. When it comes to signal integrity (polarity, rising/falling edge), then I remain convinced that this should be set via platform data. This is not something that should be negotiated since this depends not only on the sensor and host devices, but also on the routing of the lines between them on the actual board, how much noise there is on those lines, the quality of the clock signal, etc. Not really an issue with PAL/NTSC type signals, but when you get to 1080p60 and up, then such things become much more important. So these settings should not be negotiated, but set explicitly. It actually doesn't have to be done through platform data (although that makes the most sense), as long as it is explicitly set based on board-specific data. Regards, Hans > > Thanks > Guennadi > > > --- > > It tries to create a common API for getting the sensor interface type > > - serial or parallel, modes and interface clocks. The interface clocks > > then are used in the host side to calculate it's configuration, check > > that the clocks are not beyond host limitations etc. > > > > "phy_rate" in serial interface (CSI DDR clk) is used to calculate > > the CSI2 PHY receiver timing parameters: ths_settle, ths_term, > > clk_settle and clk_term. > > > > As the "phy_rate" depends on current sensor mode (configuration of the > > sensor's PLL and internal clock domains) it can be treated as dynamic > > parameter and can vary (could be different for viewfinder and still > > capture), in this context g_interface_parms should be called after > >
Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
On Tue, 22 Feb 2011, Stanimir Varbanov wrote: > This RFC patch adds a new subdev sensor operation named g_interface_parms. > It is planned as a not mandatory operation and it is driver's developer > decision to use it or not. > > Please share your opinions and ideas. Yes, I like the idea in principle (/me pulling his bullet-proof vest on), as some of you might guess, because I feel it's going away from the idea, that I've been hard pressed to accept of hard-coding the media-bus configuration and in the direction of direct communication of bus-parameters between the (sub-)devices, e.g., a camera host and a camera device in soc-camera terminology. But before reviewing the patch as such, I'd like to discuss the strategy, that we want to pursue here - what exactly do we want to hard-code and what we want to configure dynamically? As explained before, my preference would be to only specify the absolute minimum in the platform data, i.e., parameters that either are ambiguous or special for this platform. So, once again, my approach to configure interface parameters like signal polarities and edge sensitivity is: 1. if at least one side has a fixed value of the specific parameter, usually no need to specify it in platform data. Example: sensor only supports HSYNC active high, host supports both, normally "high" should be selected. 2. as above, but there's an inverter on the board in the signal path. The "invert" parameter must be specified in the platform data and the host will configure itself to "low" and send "high" confirmed to the sensor. 3. both are configurable. In this case the platform data has to specify, which polarity shall be used. This is simple, it is implemented, it has worked that way with no problem for several years now. The configuration procedure in this case looks like: 1. host requests supported interface configurations from the client (sensor) 2. host matches returned parameters against platform data and its own capabilities 3. if no suitable configuration possible - error out 4. the single possible configuration is identified and sent to the sensor back for its configuration This way we need one more method: s_interface_parms. Shortly talking to Laurent earlier today privately, he mentioned, that one of the reasons for this move is to support dynamic bus reconfiguration, e.g., the number of used CSI lanes. The same is useful for parallel interfaces. E.g., I had to hack the omap3spi driver to capture only 8 (parallel) data lanes from the sensor, connected with all its 10 lanes to get a format, easily supported by user-space applications. Ideally you don't want to change anything in the code for this. If the user is requesting the 10-bit format, all 10 lanes are used, if only 8 - the interface is reconfigured accordingly. Thanks Guennadi > --- > It tries to create a common API for getting the sensor interface type > - serial or parallel, modes and interface clocks. The interface clocks > then are used in the host side to calculate it's configuration, check > that the clocks are not beyond host limitations etc. > > "phy_rate" in serial interface (CSI DDR clk) is used to calculate > the CSI2 PHY receiver timing parameters: ths_settle, ths_term, > clk_settle and clk_term. > > As the "phy_rate" depends on current sensor mode (configuration of the > sensor's PLL and internal clock domains) it can be treated as dynamic > parameter and can vary (could be different for viewfinder and still > capture), in this context g_interface_parms should be called after > s_fmt. > > "pix_clk" for parallel interface reflects the current sensor pixel > clock. With this clock the image data is clocked out of the sensor. > > "pix_clk" for serial interface reflects the current sensor pixel > clock at which image date is read from sensor matrix. > > "lanes" for serial interface reflects the number of PHY lanes used from > the sensor to output image data. This should be known from the host > side before the streaming is started. For some sensor modes it's > enough to use one lane, for bigger resolutions two lanes and more > are used. > > "channel" for serial interface is also needed from host side to > configure it's PHY receiver at particular virtual channel. > > --- > Some background and inspiration. > > - Currently in the OMAP3 ISP driver we use a set of platform data > callbacks to provide the above parameters and this comes to very > complicated platform code, driver implementation and unneeded > sensor driver <-> host driver dependences. > > - In the present time we seeing growing count of sensor drivers and > host (bridge) drivers but without standard API's for communication. > Currently the subdev sensor operations have only one operation - > g_skip_top_lines. > > Stanimir Varbanov (1): > v4l: Introduce sensor operation for getting interface configuration > > include/media/v4l2-subdev.h | 42 ++ > 1 files c
[RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
This RFC patch adds a new subdev sensor operation named g_interface_parms. It is planned as a not mandatory operation and it is driver's developer decision to use it or not. Please share your opinions and ideas. --- It tries to create a common API for getting the sensor interface type - serial or parallel, modes and interface clocks. The interface clocks then are used in the host side to calculate it's configuration, check that the clocks are not beyond host limitations etc. "phy_rate" in serial interface (CSI DDR clk) is used to calculate the CSI2 PHY receiver timing parameters: ths_settle, ths_term, clk_settle and clk_term. As the "phy_rate" depends on current sensor mode (configuration of the sensor's PLL and internal clock domains) it can be treated as dynamic parameter and can vary (could be different for viewfinder and still capture), in this context g_interface_parms should be called after s_fmt. "pix_clk" for parallel interface reflects the current sensor pixel clock. With this clock the image data is clocked out of the sensor. "pix_clk" for serial interface reflects the current sensor pixel clock at which image date is read from sensor matrix. "lanes" for serial interface reflects the number of PHY lanes used from the sensor to output image data. This should be known from the host side before the streaming is started. For some sensor modes it's enough to use one lane, for bigger resolutions two lanes and more are used. "channel" for serial interface is also needed from host side to configure it's PHY receiver at particular virtual channel. --- Some background and inspiration. - Currently in the OMAP3 ISP driver we use a set of platform data callbacks to provide the above parameters and this comes to very complicated platform code, driver implementation and unneeded sensor driver <-> host driver dependences. - In the present time we seeing growing count of sensor drivers and host (bridge) drivers but without standard API's for communication. Currently the subdev sensor operations have only one operation - g_skip_top_lines. Stanimir Varbanov (1): v4l: Introduce sensor operation for getting interface configuration include/media/v4l2-subdev.h | 42 ++ 1 files changed, 42 insertions(+), 0 deletions(-) -- 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