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
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 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 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
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
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
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
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
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
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 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 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
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
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 Clk: 330 MHz Number of Datalanes: 2 - 12MPix @ 10fps DDR Clk: 480 MHz Number of Datalanes: 2 This usually is something you know from the sensor config selected based on your params. So, I think this is something that the host should ask to the client
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
On Wednesday, February 23, 2011 15:06:49 Aguirre, Sergio wrote:
Guennadi and Hans,
snip
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 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
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, snip 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
-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 Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote: snip 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
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: snip 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, 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: snip 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
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
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 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
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: snip 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
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: snip 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 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). 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
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 changed, 42 insertions(+), 0 deletions(-) --- Guennadi Liakhovetski,
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 s_fmt. pix_clk for parallel interface reflects the current sensor pixel clock. With this clock the
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 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. 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 Clk: 330 MHz Number of Datalanes: 2 - 12MPix @ 10fps DDR Clk: 480 MHz Number of Datalanes: 2 This usually is something you know from the sensor config selected based on your params. So, I think this is something that the host
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 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
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: 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
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 convinced, that even if those parameters will be compulsory, most programmers will anyway just copy-paste them from similar set ups... Guennadi, IMO, to force peoples to __not__ thinking about the above parameter configurations you need to do
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, 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
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
