New subdev sensor operation g_interface_parms

This RFC patch adds a new subdev sensor operation named g_interface_parms. It is planned as a not mandatory operation and it is driver's developer decision to use it or not.

Please share your opinions and ideas.

---
It tries to create a common API for getting the sensor interface type
- serial or parallel, modes and interface clocks. The interface clocks then are used in the host side to calculate it's configuration, check that the clocks are not beyond host limitations etc.

"phy_rate" in serial interface (CSI DDR clk) is used to calculate the CSI2 PHY receiver timing parameters: ths_settle, ths_term, clk_settle and clk_term.

As the "phy_rate" depends on current sensor mode (configuration of the sensor's PLL and internal clock domains) it can be treated as dynamic parameter and can vary (could be different for viewfinder and still capture), in this context g_interface_parms should be called after s_fmt.

"pix_clk" for parallel interface reflects the current sensor pixel clock. With this clock the image data is clocked out of the sensor.

"pix_clk" for serial interface reflects the current sensor pixel clock at which image date is read from sensor matrix.

"lanes" for serial interface reflects the number of PHY lanes used from the sensor to output image data. This should be known from the host side before the streaming is started. For some sensor modes it's enough to use one lane, for bigger resolutions two lanes and more are used.

"channel" for serial interface is also needed from host side to configure it's PHY receiver at particular virtual channel.

---
Some background and inspiration.

- Currently in the OMAP3 ISP driver we use a set of platform data callbacks to provide the above parameters and this comes to very complicated platform code, driver implementation and unneeded sensor driver <-> host driver dependences.

- In the present time we seeing growing count of sensor drivers and host (bridge) drivers but without standard API's for communication. Currently the subdev sensor operations have only one operation - g_skip_top_lines.

Stanimir Varbanov (1):
v4l: Introduce sensor operation for getting interface configuration

 include/media/v4l2-subdev.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

Quoted 5 lines · Stanimir Varbanov

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

Quoted 48 lines

---
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 

Quoted 2 lines

 1 files changed, 42 insertions(+), 0 deletions(-)

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

-----Original Message-----
From: Hans Verkuil [mailto:[email protected]]
Sent: Tuesday, February 22, 2011 7:33 AM
To: Guennadi Liakhovetski
Cc: Stanimir Varbanov; [email protected]; [email protected]; Aguirre, Sergio
Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

Quoted 3 lines · On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:

Quoted 1 line · Stanimir Varbanov

This RFC patch adds a new subdev sensor operation named

g_interface_parms.

Quoted 1 line

Quoted 1 line

It is planned as a not mandatory operation and it is driver's

developer

Quoted 3 lines

Quoted 3 lines

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.)

Quoted 1 line

Yes, I like the idea in principle (/me pulling his bullet-proof vest

on),

:-)

Quoted 1 line

as some of you might guess, because I feel it's going away from the

idea,

Quoted 3 lines

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

Quoted 3 lines

device in soc-camera terminology.

But before reviewing the patch as such, I'd like to discuss the

strategy,

Quoted 2 lines

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

Quoted 1 line

would be to only specify the absolute minimum in the platform data,

i.e.,

Quoted 7 lines

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

Quoted 3 lines

selected.

2. as above, but there's an inverter on the board in the signal path.

The

Quoted 7 lines

"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

Quoted 7 lines

for several years now.

The configuration procedure in this case looks like:

1. host requests supported interface configurations from the client (sensor)

Quoted 6 lines

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

Quoted 5 lines

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

Quoted 4 lines

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

Quoted 4 lines

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

Quoted 27 lines

Thanks
Guennadi

Quoted 23 lines

---
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

Quoted 21 lines

Quoted 21 lines

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.

Quoted 4 lines

Quoted 4 lines

Stanimir Varbanov (1):
v4l: Introduce sensor operation for getting interface configuration

 include/media/v4l2-subdev.h |   42

++++++++++++++++++++++++++++++++++++++++++

Quoted 9 lines

Quoted 2 lines

 1 files changed, 42 insertions(+), 0 deletions(-)

---
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

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

On Tuesday, February 22, 2011 7:33 AM Hans Verkuil wrote: > > On Tuesday,

Quoted 83 lines

Quoted 8 lines · Stanimir Varbanov

Quoted 7 lines

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.)

Quoted 47 lines

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.

Quoted 9 lines

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.

Quoted 5 lines

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 should ask to the client after setting size & framerate, or just before VIDIOC_STREAMON.

THIS I think must be handle dynamically by introducing it as part of a V4l2_subdev sensor_ops entry. If this can't be done, then how?

2. Specifying polarity and lanes physical position.

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.

Of course, I'm here discarding the fact that at least some sensors that I know of can change the primary data lane position in sensor configuration (at least some OmniVision sensors do), but I'll assume this is usually Locked, and assume the host is better at that.

Quoted 7 lines · On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:

Quoted 5 lines · Stanimir Varbanov

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.)

Quoted 1 line

Yes, I like the idea in principle (/me pulling his bullet-proof vest on),

:-)

Quoted 44 lines

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.

Quoted 9 lines

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

Quoted 48 lines

Thanks
Guennadi

Quoted 44 lines

---
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.

Quoted 4 lines

Quoted 4 lines

Stanimir Varbanov (1):
v4l: Introduce sensor operation for getting interface configuration

 include/media/v4l2-subdev.h |   42 

++++++++++++++++++++++++++++++++++++++++++

Quoted 12 lines

Quoted 2 lines

 1 files changed, 42 insertions(+), 0 deletions(-)

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Tue, 22 Feb 2011, Hans Verkuil wrote:

Quoted 1 line

I have no problems with dynamic bus reconfiguration as such. So if the

Quoted 5 lines

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

Quoted 2 lines

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

Quoted 1 line

signal, etc. Not really an issue with PAL/NTSC type signals, but when you

Quoted 1 line

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

Quoted 3 lines

So these settings should not be negotiated, but set explicitly.

It actually doesn't have to be done through platform data (although that

Quoted 1 line

the most sense), as long as it is explicitly set based on board-specific

Quoted 25 lines

Regards,

Hans

Quoted 20 lines

Thanks
Guennadi

Quoted 16 lines

---
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.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

"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

Quoted 29 lines

Quoted 21 lines

Quoted 21 lines

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.

Quoted 4 lines

Quoted 4 lines

Stanimir Varbanov (1):
v4l: Introduce sensor operation for getting interface configuration

 include/media/v4l2-subdev.h |   42 

++++++++++++++++++++++++++++++++++++++++++

Quoted 3 lines

Quoted 2 lines

 1 files changed, 42 insertions(+), 0 deletions(-)

Quoted 6 lines

Quoted 6 lines

---
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"

Quoted 4 lines

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Secondly, if we rely on negotiations, then someone at some time might change things and suddenly the negotiation gives different results which may not work on some boards. And such bugs can be extremely hard to track down. So that is

why I don't want to rely on negotiations of these settings. People are free to copy and paste, though. I assume (and hope) that they will test before sending a patch, so if it works with the copy-and-pasted settings, then that's good enough for me.

Quoted 72 lines · Hans Verkuil

Quoted 7 lines · On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:

Quoted 5 lines · Stanimir Varbanov

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.)

Quoted 1 line

Yes, I like the idea in principle (/me pulling his bullet-proof vest on),

:-)

Quoted 44 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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.

In principle I agree with this bus negotiation.

- So. let's start thinking how this could be fit to the subdev sensor
operations.

- 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 :)

Quoted 4 lines · Stan

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;)

Quoted 5 lines

- howto isolate your current work into some common place and reuse it,
even on platform part.
- and is it possible.

The discussion becomes very emotional and this is not a good adviser :)

No, no emotions at least on this side:) But it's also not technical, unfortunately. I'm prepared to discuss technical benefits or drawbacks of each of these approaches, but these arguments - can we trust programmers or can we not? or will anyone at some time in the future break it or not? Sorry, I am not a psychologist:) Personally, I would _exclusively_ consider technical arguments. Of course, things like "clean and simple APIs," "proper separation / layering" etc. are also important, but even they already can become difficult to discuss and are already on the border between technical issues and personal preferences... So, don't know, in the end, I think, it will just come down to who is making decisions and who is implementing them:) I just expressed my opinion, we don't have to agree, eventually, the maintainer will decide whether to apply patches or not:)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 16 lines · On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:

Quoted 4 lines · Stan

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;)

Quoted 5 lines

- 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 :)

Quoted 13 lines

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.

On Tue, 22 Feb 2011, Hans Verkuil wrote:

Quoted 13 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 16 lines · On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:

Quoted 4 lines · Stan

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;)

Quoted 5 lines

- 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 :)

Quoted 13 lines

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

Quoted 12 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi everybody,

Quoted 31 lines · Hans Verkuil

Quoted 16 lines · On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:

Quoted 4 lines · Stan

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;)

Quoted 5 lines

- 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 :)

Quoted 13 lines

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:)

Quoted 3 lines

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

Quoted 43 lines

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

Quoted 12 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

-----Original Message-----
From: Hans Verkuil [mailto:[email protected]]
Sent: Wednesday, February 23, 2011 3:32 AM
To: Hans Verkuil
Cc: Sylwester Nawrocki; Guennadi Liakhovetski; Stan; linux- [email protected]; Laurent Pinchart; Aguirre, Sergio
Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

Quoted 1 line · 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

Quoted 1 line

not necessarily mean that the host should sample at the same edge.

Depending

Quoted 1 line

on the clock line routing and the integrity of the clock signal the host

may

Quoted 1 line

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

Quoted 43 lines · On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:

Hi everybody,

Quoted 31 lines · Hans Verkuil

Quoted 16 lines · On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:

Quoted 4 lines · Stan

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;)

Quoted 5 lines

- 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 :)

Quoted 13 lines

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:)

Quoted 3 lines

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.

Quoted 3 lines

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/[email protected]/msg09041.html

Regards,

Hans

Quoted 45 lines

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

Quoted 31 lines

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

Quoted 1 line

Quoted 14 lines

Quoted 12 lines

Quoted 11 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Quoted 1 line

The only static data I am concerned about are those that affect signal

integrity.

Quoted 1 line

After thinking carefully about this I realized that there is really only

one

Quoted 1 line

setting that is relevant to that: the sampling edge. The polarities do

not

Quoted 1 line

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

Quoted 1 line

Unfortunately, if a subdev is set to 'sample at rising edge', then that

does

Quoted 1 line

not necessarily mean that the host should sample at the same edge.

Depending

Quoted 1 line

on the clock line routing and the integrity of the clock signal the host

may

Quoted 3 lines

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

Quoted 1 line

negotiating the sampling edge. During the Helsinki meeting in June last

year

Quoted 4 lines

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

Quoted 8 lines

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

Quoted 9 lines

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.

Quoted 10 lines

This old RFC gives a good overview of the possible polarities:

http://www.mail-archive.com/[email protected]/msg09041.html

Regards,

Hans

Quoted 1 line

I personally like the Stanimir's proposal as the parameters to be

negotiated

Quoted 1 line

Quoted 1 line

are pretty dynamic. Only the number of lanes could be problematic as

not all

Quoted 1 line

Quoted 1 line

lanes might be routed across different boards. Perhaps we should

consider specifying

Quoted 1 line

Quoted 1 line

an AUTO value for some negotiated parameters. Such as in case of an

attribute that

Quoted 1 line

Quoted 1 line

need to be fixed on some boards or can be fully negotiated on others,

a fixed

Quoted 1 line

Quoted 1 line

value or "auto" could be respectively set up in the host's

platform_data. This could

Quoted 3 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 6 lines

issues.

Regards,
Sylwester

Quoted 1 line

about this if it wasn't for the fact that soc-camera doesn't do this

but instead

Quoted 1 line

Quoted 1 line

Quoted 1 line

negotiates it. Obviously, it isn't a pleasant prospect having to

change all that.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Normally this would be enough of an argument for me to just

negotiate it. The

Quoted 1 line

Quoted 1 line

Quoted 1 line

reason that I don't want this in this particular case is that I know

from

Quoted 1 line

Quoted 1 line

Quoted 1 line

personal experience that incorrect settings can be extremely hard to

find.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I also think that there is a reasonable chance that such bugs can

happen. Take

Quoted 1 line

Quoted 1 line

Quoted 1 line

a scenario like this: someone writes a new host driver. Initially

there is only

Quoted 1 line

Quoted 1 line

Quoted 1 line

support for positive polarity and detection on the rising edge,

because that's

Quoted 1 line

Quoted 1 line

Quoted 1 line

what the current board on which the driver was developed supports.

This is quite

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

typical for an initial version of a driver.

Later someone adds support for negative polarity and falling edge.

Suddenly the

Quoted 1 line

Quoted 1 line

Quoted 1 line

polarity negotiation on the previous board results in negative

instead of positive

Quoted 1 line

Quoted 1 line

Quoted 1 line

which was never tested. Now that board starts producing pixel errors

every so

Quoted 1 line

Quoted 1 line

Quoted 1 line

often. And yes, this type of hardware problems do happen as I know

from painful

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

experience.

Problems like this are next to impossible to debug without the aid

of an

Quoted 1 line

Quoted 1 line

Quoted 1 line

oscilloscope, so this isn't like most other bugs that are relatively

easy to

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

debug.

It is so much easier just to avoid this by putting it in platform

data. It's

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

simple, unambiguous and above all, unchanging.

Regards,

Hans

Quoted 1 line

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Hans Verkuil - video4linux developer - sponsored by Cisco

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Guennadi and Hans,

<snip>

Quoted 5 lines

Quoted 1 line

The only static data I am concerned about are those that affect signal

integrity.

Quoted 1 line

After thinking carefully about this I realized that there is really only

one

Quoted 1 line

setting that is relevant to that: the sampling edge. The polarities do

Quoted 2 lines

not

Quoted 1 line

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.

Quoted 18 lines

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

Quoted 4 lines

(see include/media/soc_camera.h::soc_camera_bus_param_compatible() and drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). Removing the pixclk polarity, the rest we can use as a basis for a new subdev-based implementation.

Don't we need to move this out from soc_camera and make it available in v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI support)

That way both SoC_Camera, and Media Controller frameworks can use that.

Regards,
Sergio

Quoted 141 lines

Thanks
Guennadi

Quoted 1 line

Unfortunately, if a subdev is set to 'sample at rising edge', then that

does

Quoted 1 line

not necessarily mean that the host should sample at the same edge.

Depending

Quoted 1 line

on the clock line routing and the integrity of the clock signal the host

may

Quoted 3 lines

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

Quoted 1 line

negotiating the sampling edge. During the Helsinki meeting in June last

year

Quoted 4 lines

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

Quoted 8 lines

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

Quoted 9 lines

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.

Quoted 10 lines

This old RFC gives a good overview of the possible polarities:

http://www.mail-archive.com/[email protected]/msg09041.html

Regards,

Hans

Quoted 1 line

I personally like the Stanimir's proposal as the parameters to be

negotiated

Quoted 1 line

Quoted 1 line

are pretty dynamic. Only the number of lanes could be problematic as

not all

Quoted 1 line

Quoted 1 line

lanes might be routed across different boards. Perhaps we should

consider specifying

Quoted 1 line

Quoted 1 line

an AUTO value for some negotiated parameters. Such as in case of an

attribute that

Quoted 1 line

Quoted 1 line

need to be fixed on some boards or can be fully negotiated on others,

a fixed

Quoted 1 line

Quoted 1 line

value or "auto" could be respectively set up in the host's

platform_data. This could

Quoted 3 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 6 lines

issues.

Regards,
Sylwester

Quoted 1 line

about this if it wasn't for the fact that soc-camera doesn't do this

but instead

Quoted 1 line

Quoted 1 line

Quoted 1 line

negotiates it. Obviously, it isn't a pleasant prospect having to

change all that.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Normally this would be enough of an argument for me to just

negotiate it. The

Quoted 1 line

Quoted 1 line

Quoted 1 line

reason that I don't want this in this particular case is that I know

from

Quoted 1 line

Quoted 1 line

Quoted 1 line

personal experience that incorrect settings can be extremely hard to

find.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I also think that there is a reasonable chance that such bugs can

happen. Take

Quoted 1 line

Quoted 1 line

Quoted 1 line

a scenario like this: someone writes a new host driver. Initially

there is only

Quoted 1 line

Quoted 1 line

Quoted 1 line

support for positive polarity and detection on the rising edge,

because that's

Quoted 1 line

Quoted 1 line

Quoted 1 line

what the current board on which the driver was developed supports.

This is quite

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

typical for an initial version of a driver.

Later someone adds support for negative polarity and falling edge.

Suddenly the

Quoted 1 line

Quoted 1 line

Quoted 1 line

polarity negotiation on the previous board results in negative

instead of positive

Quoted 1 line

Quoted 1 line

Quoted 1 line

which was never tested. Now that board starts producing pixel errors

every so

Quoted 1 line

Quoted 1 line

Quoted 1 line

often. And yes, this type of hardware problems do happen as I know

from painful

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

experience.

Problems like this are next to impossible to debug without the aid

of an

Quoted 1 line

Quoted 1 line

Quoted 1 line

oscilloscope, so this isn't like most other bugs that are relatively

easy to

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

debug.

It is so much easier just to avoid this by putting it in platform

data. It's

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

simple, unambiguous and above all, unchanging.

Regards,

Hans

Quoted 1 line

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Hans Verkuil - video4linux developer - sponsored by Cisco

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Quoted 14 lines · Sergio

Guennadi and Hans,

<snip>

Quoted 7 lines

Quoted 1 line

The only static data I am concerned about are those that affect signal

integrity.

Quoted 1 line

After thinking carefully about this I realized that there is really only

one

Quoted 1 line

setting that is relevant to that: the sampling edge. The polarities do

not

Quoted 1 line

matter in this.

I respectfully disagree.

Quoted 3 lines

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.

<snip>

Quoted 6 lines

Quoted 6 lines

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.

Quoted 14 lines

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

Quoted 1 line

data bus width

Quoted 6 lines

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.

Quoted 6 lines

Quoted 6 lines

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 ?

Quoted 1 line

Quoted 1 line

data bus width

The data bus width can already be configured through the media bus format. Do we need to set it explicitly ?

Quoted 5 lines · Sergio

<snip>

Quoted 3 lines

Quoted 2 lines

The only static data I am concerned about are those that affect signal integrity.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

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

account.

Quoted 19 lines

AFAIK, There is not such thing as sampling edge configuration for MIPI Receivers, and the polarities DO matter, since it's a differential signal.

Quoted 14 lines

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 ?

Quoted 1 line

Quoted 1 line

data bus width

The data bus width can already be configured through the media bus format.

we need to set it explicitly ?

Quoted 12 lines

Quoted 6 lines

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.

Signature · 7 lines

Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 12 lines · On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:

Quoted 10 lines · Sergio

<snip>

Quoted 6 lines

Quoted 6 lines

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.

Quoted 2 lines

So do I. Sampling edge is related to polarities, so you need to take both into account.

When you switch polarity for data/field/hsync/vsync signals on a simple bus you just invert whether a 1 bit is output as high or low voltage. So you just change the meaning of the bit. This does not matter for signal integrity, since you obviously have to be able to sample both low and high voltages. It is *when* you sample that can have a major effect.

This might be different for differential clocks. I have no experience with this, so I can't say anything sensible about that.

-----Original Message-----
From: Laurent Pinchart [mailto:[email protected]]
Sent: Wednesday, February 23, 2011 10:15 AM
To: Hans Verkuil
Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester Nawrocki; Stan; [email protected]
Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

Quoted 8 lines · On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:

Quoted 7 lines · On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:

Quoted 6 lines · Sergio

<snip>

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

only one setting that is relevant to that: the sampling edge.

The

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

polarities do not matter in this.

I respectfully disagree.

So do I. Sampling edge is related to polarities, so you need to take

both

Quoted 3 lines

Quoted 1 line

into account.

When you switch polarity for data/field/hsync/vsync signals on a simple

bus

Quoted 3 lines

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

Quoted 1 line

voltages. It is *when* you sample that can have a major effect.

When you switch the polarity you will likely have to sample on the opposite
edge. If, for signal integrity reasons, you can only sample on a given edge,
you will want to use a fixed polarity and not negotiate it.

Given the very small number of parameters that are negotiated by soc- camera at
the moment, I'm very much in favour of hardcoding all of them in platform data
and just adding a g_interface_parms subdev operation.

Quoted 2 lines

This might be different for differential clocks. I have no experience

with

Quoted 1 line

this, so I can't say anything sensible about that.

--
Regards,

Laurent Pinchart

Hi,

Quoted 33 lines

-----Original Message-----
From: Laurent Pinchart [mailto:[email protected]]
Sent: Wednesday, February 23, 2011 10:15 AM
To: Hans Verkuil
Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester Nawrocki; Stan; [email protected]
Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

Quoted 8 lines · On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:

Quoted 7 lines · On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:

Quoted 6 lines · Sergio

<snip>

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

only one setting that is relevant to that: the sampling edge.

The

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

polarities do not matter in this.

I respectfully disagree.

So do I. Sampling edge is related to polarities, so you need to take

both

Quoted 3 lines

Quoted 1 line

into account.

When you switch polarity for data/field/hsync/vsync signals on a simple

bus

Quoted 3 lines

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

Quoted 8 lines

high

Quoted 1 line

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.

Quoted 6 lines

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.

-----Original Message-----
From: Guennadi Liakhovetski [mailto:[email protected]]
Sent: Wednesday, February 23, 2011 10:46 AM
To: Aguirre, Sergio
Cc: Laurent Pinchart; Hans Verkuil; Sylwester Nawrocki; Stan; linux- [email protected]
Subject: RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

Quoted 9 lines · Sergio

Hi,

Quoted 7 lines

-----Original Message-----
From: Laurent Pinchart [mailto:[email protected]]
Sent: Wednesday, February 23, 2011 10:15 AM
To: Hans Verkuil
Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester Nawrocki; Stan; [email protected]
Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation

g_interface_parms

Quoted 7 lines

Quoted 7 lines · On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:

Quoted 5 lines · On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:

Quoted 4 lines · Sergio

Quoted 3 lines

<snip>

Quoted 1 line

Quoted 1 line

The only static data I am concerned about are those that

affect

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

signal integrity.

After thinking carefully about this I realized that there is

really

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

only one setting that is relevant to that: the sampling

edge.

Quoted 6 lines

Quoted 6 lines

The

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

polarities do not matter in this.

I respectfully disagree.

So do I. Sampling edge is related to polarities, so you need to

take

Quoted 4 lines

Quoted 4 lines

both

Quoted 3 lines

Quoted 1 line

into account.

When you switch polarity for data/field/hsync/vsync signals on a

simple

Quoted 2 lines

Quoted 2 lines

bus

Quoted 1 line

you just invert whether a 1 bit is output as high or low voltage. So

you

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 10 lines

Quoted 8 lines

high

Quoted 1 line

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

Quoted 6 lines

board file.

Quoted 4 lines

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

Quoted 6 lines

Quoted 2 lines

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

Quoted 2 lines

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

Quoted 1 line

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/

Quoted 3 lines

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.

Quoted 4 lines

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?

Quoted 3 lines

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

Quoted 6 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:

Quoted 7 lines · On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:

Quoted 6 lines · Sergio

<snip>

Quoted 4 lines

Quoted 4 lines

The only static data I am concerned about are those that affect signal integrity.

After thinking carefully about this I realized that there is

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 1 line

into account.

When you switch polarity for data/field/hsync/vsync signals on a simple

Quoted 4 lines

you just invert whether a 1 bit is output as high or low voltage. So you just change the meaning of the bit. This does not matter for signal integrity, since you obviously have to be able to sample both low and high voltages. It is *when* you sample that can have a major effect.

When you switch the polarity you will likely have to sample on the opposite edge. If, for signal integrity reasons, you can only sample on a given edge, you will want to use a fixed polarity and not negotiate it.

Given the very small number of parameters that are negotiated by soc-camera

the moment, I'm very much in favour of hardcoding all of them in platform

and just adding a g_interface_parms subdev operation.

Quoted 3 lines

This might be different for differential clocks. I have no experience with this, so I can't say anything sensible about that.

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 1 line · On Wednesday, February 23, 2011 17:14:41 Laurent Pinchart wrote:

On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:

Quoted 7 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Currently soc-camera auto-configures the following parameters:

hsync polarity
vsync polarity
data polarity

master / slave mode
data bus width
pixel clock polarity

Hi Guennadi,

On Wednesday 23 February 2011 10:31:21 Guennadi Liakhovetski wrote:

[snip]

Quoted 5 lines

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 ?

Quoted 4 lines

master / slave mode
data bus width
pixel clock polarity

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 9 lines · Hans Verkuil

Quoted 3 lines · 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!

Hi Guennadi and others,

Apologies for the late reply...

Quoted 12 lines · Guennadi Liakhovetski

Quoted 9 lines · Hans Verkuil

Quoted 3 lines · 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.

Quoted 4 lines

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.

Quoted 32 lines · On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:

Hi Guennadi and others,

Apologies for the late reply...

Quoted 12 lines · Guennadi Liakhovetski

Quoted 9 lines · Hans Verkuil

Quoted 3 lines · 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.

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 33 lines · Hans Verkuil

Quoted 32 lines · On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:

Hi Guennadi and others,

Apologies for the late reply...

Quoted 12 lines · Guennadi Liakhovetski

Quoted 9 lines · Hans Verkuil

Quoted 3 lines · 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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

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...

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 8 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 32 lines · On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:

Hi Guennadi and others,

Apologies for the late reply...

Quoted 12 lines · Guennadi Liakhovetski

Quoted 9 lines · Hans Verkuil

Quoted 3 lines · 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.

Quoted 4 lines

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.

Quoted 8 lines

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

Quoted 27 lines · On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:

Quoted 11 lines · Guennadi Liakhovetski

Quoted 9 lines · Hans Verkuil

Quoted 3 lines · 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.

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 5 lines · Stanimir Varbanov

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.

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.