* [RFC] On controlling sensors
@ 2011-12-01 14:30 Sakari Ailus
2011-12-14 15:22 ` [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt Sakari Ailus
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Sakari Ailus @ 2011-12-01 14:30 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, hverkuil, snjw23, g.liakhovetski, dacohen,
hdegoede, pawel
Hi all,
I've been working a few years with digital cameras and Linux and come to the
conclusion that the way the sensors are typically configured through V4L2
subdevs is far from optimal for the use of the low level software that
typically accesses it, nor we make efficient use of the standard V4L2 subdev
API currently.
The regular S_FMT and S_PARM IOCTLs are fine as a high level interface. This
RFC does _not_ take a negative stance against them nor suggest they should
ever be deprecated. No new interfaces are proposed for regular V4L2
applications either.
Comments to this RFC are the most welcome.
What's wrong
============
Pixel array, binning and scaling
--------------------------------
ENUM_FRAMESIZES interface makes no sense on V4L2 subdevice user space
interface. Why so? Since the size of the sensor's pixel array is constant.
Other sizes are derived from that by scaling and cropping.
These days most sensors can perform a lot of image processing by themselves
--- for example binning and cropping are available on most sensors the
pixel array of which is larger than 640x480 or so. More advanced sensors can
also perform scaling.
We have existing interface in V4L2 subdev to configure scaling. The sensors
should impelement this interface. This may involve exporting more than one
subdevs from the sensor driver.
Frame rates
-----------
Sensors are utterly unaware of the whole concept of frame rate. Instead,
they deal with pixel clocks, horizontal and vertical blanking, binning and
scaling factors and link frequencies. The same goes for the low level
software controlling automatic white balance and exposure on the sensor:
this information is required to set and get detailed timing information from
the sensor.
However, the current control mechanisms provided by the V4L2 subdev
interface provides neither.
It is also impossible to provide detailed frame rate information using the
current interfaces.
Pixel rate
---------
The pixel rate is important as well. The maximum pixel rate for some of the
hardware blocks may be lower than those earlier in the pipeline. This
becomes an issue in the OMAP 3 ISP where only the CSI-2 receiver can handle
pixel rates up to 200 Mp/s whereas the rest of the ISP blocks only can do
100 Mp/s.
The current APIs do not reveal this, but it is also impossible to know the
output data rate of the sensor in order to come up with a valid
configuration.
How to fix this
===============
Sensors should be controlled by exposing their natural parameters to the
user. This means the following:
Link frequency as control
-------------------------
Typically the hardware allows few different link frequencies to be used. The
actual data rate may depend on the bits-per-pixel value and how many lanes
are being used on serial links.
Togethet with the binning and scaling factors, the link pixel rate defines
how fast the sensor pixel array can be read. The pixel rate at the pixel
array is important for calculating the sensor timing for the user of the
control algorithms.
Link frequency is chosen over pixel rate since pixel rate is dependent on
the format of the data --- bits per pixel.
Horizontal and vertical blanking as controls
--------------------------------------------
Horizontal and vertical blanking typically is relatively freely configurable
in a range between a lower and an upper limit. The limits are hardware
specific. The unit of the horizontal blanking is line and for vertical
blanking it's pixel.
Together with the link rate, vertical and horizontal blanking, image width
and height define the frame rate.
There are also other reasons why the user is interested in the blanking
information, such as to minimise the rolling shutter effect or amortise the
data rate on the memory bus.
Binning, scaling and cropping configuration
-------------------------------------------
Binning, scaling and cropping must be configured using the same Media
controller and V4L2 subdevice APIs as the image pipe inside the ISPs. It
makes really no difference whether the scaler is located in the ISP or in
the sensor: the API for configuring it must be the same. This may mean
exposing the sensor as multiple subdevices to make it configurable using the
V4L2 subdevice interface.
A sensor could look like this, for example:
pixel array:0 [crop] ---> 0:binner:1 ---> 0:ISP's CSI2 receiver
The pixel array's pixel rate would only be defined after configuring the
binner and the link rate between the sensor and the ISP. The reason for this
is that the pixel clock in the pixel array may well be higher than after the
scaler. But the pixel clock, and the limits for vertical and horizontal
blanking are only available after the binning factor and the link rate have
been specified.
The blanking must be thus specified after configuring the formats on the
pipeline.
Proposal
========
In the spec and the interfaces not much would change: only the purposes we
are currently using the interfaces would change a little bit. The sensor
drivers would need to make more use of the V4L2 subdev interface.
Control class for low-level controls
------------------------------------
A new control class will be created for low level controls. It is to be
called
V4L2_CTRL_CLASS_LL,
where "LL" is for "low level". Better proposals on the name of the class are
welcome. My feeling is that we might get lots of controls to this class over
time.
Controls in this class will include
V4L2_CID_LL_VBLANK,
V4L2_CID_LL_HBLANK and
V4L2_CID_LL_LINK_FREQ.
Link frequency is chosen instead of pixel rate since the pixel rate is
dependent on the format. That would make the available values depend on
selected format which I consider potentially confusing.
The same class should likely hold the controls for separate analogue and
digital gains and per-component gains. Per-component gains are typically
digital-only, but some sensors support per-component analog gains. So we
should have common gains and per-component gains, both digital and analog.
Pixel rate as part of struct v4l2_mbus_pixelfmt
-----------------------------------------------
Pixel rate would be added to the struct v4l2_mbus_pixelfmt in kP/s
(kilopixels per second).
struct v4l2_mbus_framefmt {
__u32 width;
__u32 height;
__u32 code;
__u32 field;
__u32 colorspace;
__u32 pixel_rate;
__u32 reserved[6];
};
Sensors with only a few pre-defined configurations
--------------------------------------------------
Some sensors, such as the infamous Omnivision ones, only provide register
lists to configure the sensor from a few pre-defined configurations. These
sensor drivers cannot meaningfully expose the low-level API for controlling
them and are bound to continue to provide high level information on supported
resolutions and frame rates.
The generic user space should have no issues in recognising such sensors
from the ones which do provide the low-level controls. We still need to
define the exact method to distinguish them.
I don't like very much that we would have different ways to configure
devices, but as we simply do not have enough information to configure these
properly I do not really see a way around it.
Regular V4L2 applications
-------------------------
Controlling sensors this way is obviously the most suitable for embedded
systems. Desktops might not care. But still on embedded systems one may well
want to run regular V4L2 applications.
The solution for this is the same as previously agreed on: a pipeline setup
library, a configuration configuration program and a libv4l2 plugin using
the library. Vendors are encouraged to provide their own libraries which
know the features of their hardware the best and can best configure them as
well.
It also would require using these libraries on devices which have not needed
them before: if you combine a sensor driver exposing this kind of an
interface with a regular camera bridge which just exposes regular V4L2 API,
it won't work as such.
To avoid creating double effort in pipeline configuration, the bridge driver
also must expose V4L2 subdev API so the full pipeline configuration can be
done from the user space.
Cheers,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt
2011-12-01 14:30 [RFC] On controlling sensors Sakari Ailus
@ 2011-12-14 15:22 ` Sakari Ailus
2011-12-15 21:46 ` Sylwester Nawrocki
2011-12-14 15:22 ` [RFC 2/3] v4l: Image source control class Sakari Ailus
2011-12-14 15:22 ` [RFC 3/3] v4l: Add pad op for pipeline validation Sakari Ailus
2 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2011-12-14 15:22 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, snjw23, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Pixel clock is an essential part of the image data parameters. Add this.
Together, the current parameters also define the frame rate.
Sensors do not have a concept of frame rate; pixel clock is much more
meaningful in this context. Also, it is best to combine the pixel clock with
the other format parameters since there are dependencies between them.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Documentation/DocBook/media/v4l/subdev-formats.xml | 9 ++++++++-
include/linux/v4l2-mediabus.h | 4 +++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
index 49c532e..b4591ef 100644
--- a/Documentation/DocBook/media/v4l/subdev-formats.xml
+++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
@@ -35,7 +35,14 @@
</row>
<row>
<entry>__u32</entry>
- <entry><structfield>reserved</structfield>[7]</entry>
+ <entry><structfield>pixel_clock</structfield></entry>
+ <entry>Pixel clock in kHz. This clock is the maximum rate at
+ which pixels are transferred on the bus. The pixel_clock
+ field is read-only.</entry>
+ </row>
+ <row>
+ <entry>__u32</entry>
+ <entry><structfield>reserved</structfield>[6]</entry>
<entry>Reserved for future extensions. Applications and drivers must
set the array to zero.</entry>
</row>
diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
index 5ea7f75..76a0df2 100644
--- a/include/linux/v4l2-mediabus.h
+++ b/include/linux/v4l2-mediabus.h
@@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
* @code: data format code (from enum v4l2_mbus_pixelcode)
* @field: used interlacing type (from enum v4l2_field)
* @colorspace: colorspace of the data (from enum v4l2_colorspace)
+ * @pixel_clock: pixel clock, in kHz
*/
struct v4l2_mbus_framefmt {
__u32 width;
@@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
__u32 code;
__u32 field;
__u32 colorspace;
- __u32 reserved[7];
+ __u32 pixel_clock;
+ __u32 reserved[6];
};
#endif
--
1.7.2.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC 2/3] v4l: Image source control class
2011-12-01 14:30 [RFC] On controlling sensors Sakari Ailus
2011-12-14 15:22 ` [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt Sakari Ailus
@ 2011-12-14 15:22 ` Sakari Ailus
2011-12-31 14:42 ` Sylwester Nawrocki
2011-12-14 15:22 ` [RFC 3/3] v4l: Add pad op for pipeline validation Sakari Ailus
2 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2011-12-14 15:22 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, snjw23, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Add image source control class. This control class is intended to contain
low level controls which deal with control of the image capture process ---
the A/D converter in image sensors, for example.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Documentation/DocBook/media/v4l/controls.xml | 101 ++++++++++++++++++++
.../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 6 +
drivers/media/video/v4l2-ctrls.c | 10 ++
include/linux/videodev2.h | 10 ++
4 files changed, 127 insertions(+), 0 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 3bc5ee8..69ede83 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -3356,6 +3356,107 @@ interface and may change in the future.</para>
</table>
</section>
+
+ <section id="image-source-controls">
+ <title>Image Source Control Reference</title>
+
+ <note>
+ <title>Experimental</title>
+
+ <para>This is an <link
+ linkend="experimental">experimental</link> interface and may
+ change in the future.</para>
+ </note>
+
+ <para>
+ The Image Source control class is intended for low-level
+ control of image source devices such as image sensors. The
+ devices feature an analogue to digital converter and a bus
+ transmitter to transmit the image data out of the device.
+ </para>
+
+ <table pgwide="1" frame="none" id="image-source-control-id">
+ <title>Image Source Control IDs</title>
+
+ <tgroup cols="4">
+ <colspec colname="c1" colwidth="1*" />
+ <colspec colname="c2" colwidth="6*" />
+ <colspec colname="c3" colwidth="2*" />
+ <colspec colname="c4" colwidth="6*" />
+ <spanspec namest="c1" nameend="c2" spanname="id" />
+ <spanspec namest="c2" nameend="c4" spanname="descr" />
+ <thead>
+ <row>
+ <entry spanname="id" align="left">ID</entry>
+ <entry align="left">Type</entry>
+ </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
+ </row>
+ </thead>
+ <tbody valign="top">
+ <row><entry></entry></row>
+ <row>
+ <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry>
+ <entry>class</entry>
+ </row>
+ <row>
+ <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry>
+ </row>
+ <row>
+ <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry>
+ <entry>integer</entry>
+ </row>
+ <row>
+ <entry spanname="descr">Vertical blanking. The idle
+ preriod after every frame during which no image data is
+ produced. The unit of vertical blanking is a line. Every
+ line has length of the image width plus horizontal
+ blanking at the pixel clock specified by struct
+ v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt"
+ />.</entry>
+ </row>
+ <row>
+ <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry>
+ <entry>integer</entry>
+ </row>
+ <row>
+ <entry spanname="descr">Horizontal blanking. The idle
+ preriod after every line of image data during which no
+ image data is produced. The unit of horizontal blanking is
+ pixels.</entry>
+ </row>
+ <row>
+ <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_LINK_FREQ</constant></entry>
+ <entry>integer menu</entry>
+ </row>
+ <row>
+ <entry spanname="descr">Image source's data bus frequency.
+ Together with the media bus pixel code, bus type (clock
+ cycles per sample), the data bus frequency defines the
+ pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The
+ frame rate can be calculated from the pixel clock, image
+ width and height and horizontal and vertical blanking. The
+ frame rate control is performed by selecting the desired
+ horizontal and vertical blanking.
+ </entry>
+ </row>
+ <row>
+ <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></entry>
+ <entry>integer</entry>
+ </row>
+ <row>
+ <entry spanname="descr">Analogue gain is gain affecting
+ all colour components in the pixel matrix. The gain
+ operation is performed in the analogue domain before A/D
+ conversion.
+ </entry>
+ </row>
+ <row><entry></entry></row>
+ </tbody>
+ </tgroup>
+ </table>
+
+ </section>
+
</section>
<!--
diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
index 5122ce8..250c1cf 100644
--- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
@@ -257,6 +257,12 @@ These controls are described in <xref
These controls are described in <xref
linkend="flash-controls" />.</entry>
</row>
+ <row>
+ <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry>
+ <entry>0x9d0000</entry> <entry>The class containing image
+ source controls. These controls are described in <xref
+ linkend="image-source-controls" />.</entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 083bb79..da1ec52 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -606,6 +606,12 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_FLASH_CHARGE: return "Charge";
case V4L2_CID_FLASH_READY: return "Ready to strobe";
+ case V4L2_CID_IMAGE_SOURCE_CLASS: return "Image source controls";
+ case V4L2_CID_IMAGE_SOURCE_VBLANK: return "Vertical blanking";
+ case V4L2_CID_IMAGE_SOURCE_HBLANK: return "Horizontal blanking";
+ case V4L2_CID_IMAGE_SOURCE_LINK_FREQ: return "Link frequency";
+ case V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN: return "Analogue gain";
+
default:
return NULL;
}
@@ -694,6 +700,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:
*type = V4L2_CTRL_TYPE_MENU;
break;
+ case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:
+ *type = V4L2_CTRL_TYPE_INTEGER_MENU;
+ break;
case V4L2_CID_RDS_TX_PS_NAME:
case V4L2_CID_RDS_TX_RADIO_TEXT:
*type = V4L2_CTRL_TYPE_STRING;
@@ -703,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_MPEG_CLASS:
case V4L2_CID_FM_TX_CLASS:
case V4L2_CID_FLASH_CLASS:
+ case V4L2_CID_IMAGE_SOURCE_CLASS:
*type = V4L2_CTRL_TYPE_CTRL_CLASS;
/* You can neither read not write these */
*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 9633c69..0f8f904 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1080,6 +1080,7 @@ struct v4l2_ext_controls {
#define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */
#define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator control class */
#define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */
+#define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009d0000 /* Image source flash controls */
#define V4L2_CTRL_ID_MASK (0x0fffffff)
#define V4L2_CTRL_ID2CLASS(id) ((id) & 0x0fff0000UL)
@@ -1690,6 +1691,15 @@ enum v4l2_flash_strobe_source {
#define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
#define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
+/* Image source controls */
+#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
+#define V4L2_CID_IMAGE_SOURCE_CLASS (V4L2_CTRL_CLASS_IMAGE_SOURCE | 1)
+
+#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
+#define V4L2_CID_IMAGE_SOURCE_HBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
+#define V4L2_CID_IMAGE_SOURCE_LINK_FREQ (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
+#define V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
+
/*
* T U N I N G
*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC 3/3] v4l: Add pad op for pipeline validation
2011-12-01 14:30 [RFC] On controlling sensors Sakari Ailus
2011-12-14 15:22 ` [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt Sakari Ailus
2011-12-14 15:22 ` [RFC 2/3] v4l: Image source control class Sakari Ailus
@ 2011-12-14 15:22 ` Sakari Ailus
2 siblings, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2011-12-14 15:22 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, snjw23, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
smiapp_pad_ops.validate_pipeline is intended to validate the full pipeline
which is implemented by the driver to which the subdev implementing this op
belongs to. The validate_pipeline op must also call validate_pipeline on
any external entity which is linked to its sink pads.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
include/media/v4l2-subdev.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 26eeaa4..a5ebe86 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -470,6 +470,7 @@ struct v4l2_subdev_pad_ops {
struct v4l2_subdev_selection *sel);
int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
struct v4l2_subdev_selection *sel);
+ int (*validate_pipeline)(struct v4l2_subdev *sd);
};
struct v4l2_subdev_ops {
--
1.7.2.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt
2011-12-14 15:22 ` [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt Sakari Ailus
@ 2011-12-15 21:46 ` Sylwester Nawrocki
2011-12-15 22:01 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: Sylwester Nawrocki @ 2011-12-15 21:46 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Hi Sakari,
thanks for the patch.
On 12/14/2011 04:22 PM, Sakari Ailus wrote:
> Pixel clock is an essential part of the image data parameters. Add this.
> Together, the current parameters also define the frame rate.
>
> Sensors do not have a concept of frame rate; pixel clock is much more
> meaningful in this context. Also, it is best to combine the pixel clock with
> the other format parameters since there are dependencies between them.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> Documentation/DocBook/media/v4l/subdev-formats.xml | 9 ++++++++-
> include/linux/v4l2-mediabus.h | 4 +++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
> index 49c532e..b4591ef 100644
> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
> @@ -35,7 +35,14 @@
> </row>
> <row>
> <entry>__u32</entry>
> - <entry><structfield>reserved</structfield>[7]</entry>
> + <entry><structfield>pixel_clock</structfield></entry>
> + <entry>Pixel clock in kHz. This clock is the maximum rate at
> + which pixels are transferred on the bus. The pixel_clock
> + field is read-only.</entry>
I searched a couple of datasheets to find out where I could use this pixel_clock
field but didn't find any so far. I haven't tried too hard though ;)
There seems to be more benefits from having the link frequency control.
It might be easy to confuse pixel_clock with the bus clock. The bus clock is
often referred in datasheets as Pixel Clock (PCLK, AFAIU it's described with
link frequency in your RFC). IMHO your original proposal was better, i.e.
using more explicit pixel_rate. Also why it is in kHz ? Doesn't it make more
sense to use bits or pixels per second ?
> + </row>
> + <row>
> + <entry>__u32</entry>
> + <entry><structfield>reserved</structfield>[6]</entry>
> <entry>Reserved for future extensions. Applications and drivers must
> set the array to zero.</entry>
> </row>
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 5ea7f75..76a0df2 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
> * @code: data format code (from enum v4l2_mbus_pixelcode)
> * @field: used interlacing type (from enum v4l2_field)
> * @colorspace: colorspace of the data (from enum v4l2_colorspace)
> + * @pixel_clock: pixel clock, in kHz
> */
> struct v4l2_mbus_framefmt {
> __u32 width;
> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
> __u32 code;
> __u32 field;
> __u32 colorspace;
> - __u32 reserved[7];
> + __u32 pixel_clock;
I'm wondering, whether it is worth to make it 'pixelclock' for consistency
with other fields? Perhaps it would make more sense to have color_space and
pixel_clock.
> + __u32 reserved[6];
> };
>
> #endif
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt
2011-12-15 21:46 ` Sylwester Nawrocki
@ 2011-12-15 22:01 ` Sakari Ailus
2011-12-15 22:19 ` Sylwester Nawrocki
0 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2011-12-15 22:01 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Hi Sylwester,
Thanks for the comments!
On Thu, Dec 15, 2011 at 10:46:22PM +0100, Sylwester Nawrocki wrote:
> Hi Sakari,
>
> thanks for the patch.
>
> On 12/14/2011 04:22 PM, Sakari Ailus wrote:
> > Pixel clock is an essential part of the image data parameters. Add this.
> > Together, the current parameters also define the frame rate.
> >
> > Sensors do not have a concept of frame rate; pixel clock is much more
> > meaningful in this context. Also, it is best to combine the pixel clock with
> > the other format parameters since there are dependencies between them.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > Documentation/DocBook/media/v4l/subdev-formats.xml | 9 ++++++++-
> > include/linux/v4l2-mediabus.h | 4 +++-
> > 2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
> > index 49c532e..b4591ef 100644
> > --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
> > +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
> > @@ -35,7 +35,14 @@
> > </row>
> > <row>
> > <entry>__u32</entry>
> > - <entry><structfield>reserved</structfield>[7]</entry>
> > + <entry><structfield>pixel_clock</structfield></entry>
> > + <entry>Pixel clock in kHz. This clock is the maximum rate at
> > + which pixels are transferred on the bus. The pixel_clock
> > + field is read-only.</entry>
>
> I searched a couple of datasheets to find out where I could use this pixel_clock
> field but didn't find any so far. I haven't tried too hard though ;)
> There seems to be more benefits from having the link frequency control.
There are a few reasons to have the pixel clock available to the user space.
The previously existing reason is that the user may get information on the
pixel rates, including cases where the pixel rate of a subdev isn't enough
for the streaming to be possible. Earlier on it just failed. Such cases are
common on the OMAP 3 ISP, for example.
The second reason is to provide that for timing calculations in the user
space.
> It might be easy to confuse pixel_clock with the bus clock. The bus clock is
> often referred in datasheets as Pixel Clock (PCLK, AFAIU it's described with
> link frequency in your RFC). IMHO your original proposal was better, i.e.
> using more explicit pixel_rate. Also why it is in kHz ? Doesn't it make more
> sense to use bits or pixels per second ?
Oh, yes, now that you mention it I did call it pixel rate. I'm fine
withrenaming it back to e.g. "pixelrate".
I picked kHz since the 32-bit field would allow rates up to 4 GiP/s. Not
sure if that's overkill though. Could be. But in practice it should give
good enough precision this way, too.
> > + </row>
> > + <row>
> > + <entry>__u32</entry>
> > + <entry><structfield>reserved</structfield>[6]</entry>
> > <entry>Reserved for future extensions. Applications and drivers must
> > set the array to zero.</entry>
> > </row>
> > diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> > index 5ea7f75..76a0df2 100644
> > --- a/include/linux/v4l2-mediabus.h
> > +++ b/include/linux/v4l2-mediabus.h
> > @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
> > * @code: data format code (from enum v4l2_mbus_pixelcode)
> > * @field: used interlacing type (from enum v4l2_field)
> > * @colorspace: colorspace of the data (from enum v4l2_colorspace)
> > + * @pixel_clock: pixel clock, in kHz
> > */
> > struct v4l2_mbus_framefmt {
> > __u32 width;
> > @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
> > __u32 code;
> > __u32 field;
> > __u32 colorspace;
> > - __u32 reserved[7];
> > + __u32 pixel_clock;
>
> I'm wondering, whether it is worth to make it 'pixelclock' for consistency
> with other fields? Perhaps it would make more sense to have color_space and
> pixel_clock.
"pixelrate" is fine for me.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt
2011-12-15 22:01 ` Sakari Ailus
@ 2011-12-15 22:19 ` Sylwester Nawrocki
2011-12-16 7:40 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: Sylwester Nawrocki @ 2011-12-15 22:19 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
On 12/15/2011 11:01 PM, Sakari Ailus wrote:
>>> <entry>__u32</entry>
>>> - <entry><structfield>reserved</structfield>[7]</entry>
>>> + <entry><structfield>pixel_clock</structfield></entry>
>>> + <entry>Pixel clock in kHz. This clock is the maximum rate at
>>> + which pixels are transferred on the bus. The pixel_clock
>>> + field is read-only.</entry>
>>
>> I searched a couple of datasheets to find out where I could use this pixel_clock
>> field but didn't find any so far. I haven't tried too hard though ;)
>> There seems to be more benefits from having the link frequency control.
>
> There are a few reasons to have the pixel clock available to the user space.
>
> The previously existing reason is that the user may get information on the
> pixel rates, including cases where the pixel rate of a subdev isn't enough
> for the streaming to be possible. Earlier on it just failed. Such cases are
> common on the OMAP 3 ISP, for example.
>
> The second reason is to provide that for timing calculations in the user
> space.
Fair enough. Perhaps, if I have worked more with image signal processing
algorithms in user space I would not ask about that in the first place :-)
>
>> It might be easy to confuse pixel_clock with the bus clock. The bus clock is
>> often referred in datasheets as Pixel Clock (PCLK, AFAIU it's described with
>> link frequency in your RFC). IMHO your original proposal was better, i.e.
>> using more explicit pixel_rate. Also why it is in kHz ? Doesn't it make more
>> sense to use bits or pixels per second ?
>
> Oh, yes, now that you mention it I did call it pixel rate. I'm fine
> withrenaming it back to e.g. "pixelrate".
I'm fine with that too, sounds good!
>
> I picked kHz since the 32-bit field would allow rates up to 4 GiP/s. Not
> sure if that's overkill though. Could be. But in practice it should give
> good enough precision this way, too.
All right, however I was more concerned by the "Hz" part, rather than "k" ;)
It might be good to have the relevant unit defined in the spec, to avoid
misinterpretation and future interoperability issues .
>>> + </row>
>>> + <row>
>>> + <entry>__u32</entry>
>>> + <entry><structfield>reserved</structfield>[6]</entry>
>>> <entry>Reserved for future extensions. Applications and drivers must
>>> set the array to zero.</entry>
>>> </row>
>>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>>> index 5ea7f75..76a0df2 100644
>>> --- a/include/linux/v4l2-mediabus.h
>>> +++ b/include/linux/v4l2-mediabus.h
>>> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>>> * @code: data format code (from enum v4l2_mbus_pixelcode)
>>> * @field: used interlacing type (from enum v4l2_field)
>>> * @colorspace: colorspace of the data (from enum v4l2_colorspace)
>>> + * @pixel_clock: pixel clock, in kHz
>>> */
>>> struct v4l2_mbus_framefmt {
>>> __u32 width;
>>> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>>> __u32 code;
>>> __u32 field;
>>> __u32 colorspace;
>>> - __u32 reserved[7];
>>> + __u32 pixel_clock;
>>
>> I'm wondering, whether it is worth to make it 'pixelclock' for consistency
>> with other fields? Perhaps it would make more sense to have color_space and
>> pixel_clock.
>
> "pixelrate" is fine for me.
Ack.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt
2011-12-15 22:19 ` Sylwester Nawrocki
@ 2011-12-16 7:40 ` Sakari Ailus
0 siblings, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2011-12-16 7:40 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Hi Sylwester,
On Thu, Dec 15, 2011 at 11:19:40PM +0100, Sylwester Nawrocki wrote:
> On 12/15/2011 11:01 PM, Sakari Ailus wrote:
> >>> <entry>__u32</entry>
> >>> - <entry><structfield>reserved</structfield>[7]</entry>
> >>> + <entry><structfield>pixel_clock</structfield></entry>
> >>> + <entry>Pixel clock in kHz. This clock is the maximum rate at
> >>> + which pixels are transferred on the bus. The pixel_clock
> >>> + field is read-only.</entry>
> >>
> >> I searched a couple of datasheets to find out where I could use this pixel_clock
> >> field but didn't find any so far. I haven't tried too hard though ;)
> >> There seems to be more benefits from having the link frequency control.
> >
> > There are a few reasons to have the pixel clock available to the user space.
> >
> > The previously existing reason is that the user may get information on the
> > pixel rates, including cases where the pixel rate of a subdev isn't enough
> > for the streaming to be possible. Earlier on it just failed. Such cases are
> > common on the OMAP 3 ISP, for example.
> >
> > The second reason is to provide that for timing calculations in the user
> > space.
>
> Fair enough. Perhaps, if I have worked more with image signal processing
> algorithms in user space I would not ask about that in the first place :-)
It's not only the algorithms, but it gives you a way to perform low level
sensor configuration. It gives the user an easy way to configure the frame
rate freely between a range which is easy to obtain, and also taking into
account the policy decisions instead of the kernel doing them for the user.
There's more in the parent, albeit I forgot to mention the above:
<URL:http://www.spinics.net/lists/linux-media/msg40861.html>
>
> >
> >> It might be easy to confuse pixel_clock with the bus clock. The bus clock is
> >> often referred in datasheets as Pixel Clock (PCLK, AFAIU it's described with
> >> link frequency in your RFC). IMHO your original proposal was better, i.e.
> >> using more explicit pixel_rate. Also why it is in kHz ? Doesn't it make more
> >> sense to use bits or pixels per second ?
> >
> > Oh, yes, now that you mention it I did call it pixel rate. I'm fine
> > withrenaming it back to e.g. "pixelrate".
>
> I'm fine with that too, sounds good!
>
> >
> > I picked kHz since the 32-bit field would allow rates up to 4 GiP/s. Not
> > sure if that's overkill though. Could be. But in practice it should give
> > good enough precision this way, too.
>
> All right, however I was more concerned by the "Hz" part, rather than "k" ;)
> It might be good to have the relevant unit defined in the spec, to avoid
> misinterpretation and future interoperability issues .
Indeed. kp/s then? :-)
Regards,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 2/3] v4l: Image source control class
2011-12-14 15:22 ` [RFC 2/3] v4l: Image source control class Sakari Ailus
@ 2011-12-31 14:42 ` Sylwester Nawrocki
2011-12-31 14:53 ` Sylwester Nawrocki
2012-01-01 12:05 ` Sakari Ailus
0 siblings, 2 replies; 11+ messages in thread
From: Sylwester Nawrocki @ 2011-12-31 14:42 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Hi Sakari,
thanks for the patch.
On 12/14/2011 04:22 PM, Sakari Ailus wrote:
> Add image source control class. This control class is intended to contain
> low level controls which deal with control of the image capture process ---
> the A/D converter in image sensors, for example.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> Documentation/DocBook/media/v4l/controls.xml | 101 ++++++++++++++++++++
> .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 6 +
> drivers/media/video/v4l2-ctrls.c | 10 ++
> include/linux/videodev2.h | 10 ++
> 4 files changed, 127 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index 3bc5ee8..69ede83 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -3356,6 +3356,107 @@ interface and may change in the future.</para>
> </table>
>
> </section>
> +
> + <section id="image-source-controls">
> + <title>Image Source Control Reference</title>
> +
> + <note>
> + <title>Experimental</title>
> +
> + <para>This is an <link
> + linkend="experimental">experimental</link> interface and may
> + change in the future.</para>
> + </note>
> +
> + <para>
> + The Image Source control class is intended for low-level
> + control of image source devices such as image sensors. The
> + devices feature an analogue to digital converter and a bus
> + transmitter to transmit the image data out of the device.
> + </para>
> +
> + <table pgwide="1" frame="none" id="image-source-control-id">
> + <title>Image Source Control IDs</title>
> +
> + <tgroup cols="4">
> + <colspec colname="c1" colwidth="1*" />
> + <colspec colname="c2" colwidth="6*" />
> + <colspec colname="c3" colwidth="2*" />
> + <colspec colname="c4" colwidth="6*" />
> + <spanspec namest="c1" nameend="c2" spanname="id" />
> + <spanspec namest="c2" nameend="c4" spanname="descr" />
> + <thead>
> + <row>
> + <entry spanname="id" align="left">ID</entry>
> + <entry align="left">Type</entry>
> + </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
> + </row>
> + </thead>
> + <tbody valign="top">
> + <row><entry></entry></row>
> + <row>
> + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry>
> + <entry>class</entry>
> + </row>
> + <row>
> + <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry>
> + </row>
> + <row>
> + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry>
> + <entry>integer</entry>
> + </row>
> + <row>
> + <entry spanname="descr">Vertical blanking. The idle
> + preriod after every frame during which no image data is
s/preriod/period
> + produced. The unit of vertical blanking is a line. Every
> + line has length of the image width plus horizontal
> + blanking at the pixel clock specified by struct
> + v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt"
> + />.</entry>
> + </row>
> + <row>
> + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry>
> + <entry>integer</entry>
> + </row>
> + <row>
> + <entry spanname="descr">Horizontal blanking. The idle
> + preriod after every line of image data during which no
s/preriod/period
> + image data is produced. The unit of horizontal blanking is
> + pixels.</entry>
> + </row>
> + <row>
> + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_LINK_FREQ</constant></entry>
> + <entry>integer menu</entry>
> + </row>
> + <row>
> + <entry spanname="descr">Image source's data bus frequency.
> + Together with the media bus pixel code, bus type (clock
> + cycles per sample), the data bus frequency defines the
> + pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The
> + frame rate can be calculated from the pixel clock, image
> + width and height and horizontal and vertical blanking. The
> + frame rate control is performed by selecting the desired
> + horizontal and vertical blanking.
> + </entry>
> + </row>
> + <row>
> + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></entry>
> + <entry>integer</entry>
> + </row>
> + <row>
> + <entry spanname="descr">Analogue gain is gain affecting
> + all colour components in the pixel matrix. The gain
> + operation is performed in the analogue domain before A/D
> + conversion.
> + </entry>
> + </row>
> + <row><entry></entry></row>
> + </tbody>
> + </tgroup>
> + </table>
> +
> + </section>
> +
> </section>
>
> <!--
> diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> index 5122ce8..250c1cf 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> @@ -257,6 +257,12 @@ These controls are described in <xref
> These controls are described in <xref
> linkend="flash-controls" />.</entry>
> </row>
> + <row>
> + <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry>
> + <entry>0x9d0000</entry> <entry>The class containing image
> + source controls. These controls are described in <xref
> + linkend="image-source-controls" />.</entry>
> + </row>
> </tbody>
> </tgroup>
> </table>
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index 083bb79..da1ec52 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -606,6 +606,12 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_FLASH_CHARGE: return "Charge";
> case V4L2_CID_FLASH_READY: return "Ready to strobe";
>
> + case V4L2_CID_IMAGE_SOURCE_CLASS: return "Image source controls";
> + case V4L2_CID_IMAGE_SOURCE_VBLANK: return "Vertical blanking";
nit: have you considered making it "Blanking, horizontal"
> + case V4L2_CID_IMAGE_SOURCE_HBLANK: return "Horizontal blanking";
and "Blanking, vertical" ?
> + case V4L2_CID_IMAGE_SOURCE_LINK_FREQ: return "Link frequency";
> + case V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN: return "Analogue gain";
> +
> default:
> return NULL;
> }
> @@ -694,6 +700,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:
> *type = V4L2_CTRL_TYPE_MENU;
> break;
> + case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:
> + *type = V4L2_CTRL_TYPE_INTEGER_MENU;
> + break;
> case V4L2_CID_RDS_TX_PS_NAME:
> case V4L2_CID_RDS_TX_RADIO_TEXT:
> *type = V4L2_CTRL_TYPE_STRING;
> @@ -703,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_MPEG_CLASS:
> case V4L2_CID_FM_TX_CLASS:
> case V4L2_CID_FLASH_CLASS:
> + case V4L2_CID_IMAGE_SOURCE_CLASS:
> *type = V4L2_CTRL_TYPE_CTRL_CLASS;
> /* You can neither read not write these */
> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 9633c69..0f8f904 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1080,6 +1080,7 @@ struct v4l2_ext_controls {
> #define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */
> #define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator control class */
> #define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */
> +#define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009d0000 /* Image source flash controls */
>
> #define V4L2_CTRL_ID_MASK (0x0fffffff)
> #define V4L2_CTRL_ID2CLASS(id) ((id) & 0x0fff0000UL)
> @@ -1690,6 +1691,15 @@ enum v4l2_flash_strobe_source {
> #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
> #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
>
> +/* Image source controls */
> +#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
> +#define V4L2_CID_IMAGE_SOURCE_CLASS (V4L2_CTRL_CLASS_IMAGE_SOURCE | 1)
> +
> +#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
> +#define V4L2_CID_IMAGE_SOURCE_HBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
> +#define V4L2_CID_IMAGE_SOURCE_LINK_FREQ (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
> +#define V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
Since these are really low level controls, do you think we could shorten
the class name a bit, i.e. make it V4L2_CID_IMG_SRC_CLASS ? :)
Somehow I'm getting the misterious error again when compiling after this
patch is applied (and others from you last series of 17, xmlto version
0.0.23, perl, v5.10.1):
8<--------------------
HTML Documentation/DocBook/media_api.html
warning: failed to load external entity
"/home/snawrocki/linux-media/media_tree/Documentation/DocBook/vidioc-subdev-g-selection.xml"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The problem is in patch "v4l: Add selections documentation." [1].
Somehow file vidioc-subdev-g-selection.xml doesn't get copied to
Documentation/DocBook/ directory. When I copy it manually it compiles
fine. I'm looking now at Documentation/DocBook/Makefile to find the
reason of that.
/home/snawrocki/linux-media/media_tree/Documentation/DocBook/dev-subdev.xml:295:
parser error : Failure to process entity sub-subdev-g-selection
configured using &sub-subdev-g-selection; and
^
/home/snawrocki/linux-media/media_tree/Documentation/DocBook/dev-subdev.xml:295:
parser error : Entity 'sub-subdev-g-selection' not defined
configured using &sub-subdev-g-selection; and
^
/home/snawrocki/linux-media/media_tree/Documentation/DocBook/dev-subdev.xml:369:
parser error : chunk is not well balanced
^
/home/snawrocki/linux-media/media_tree/Documentation/DocBook/v4l2.xml:456:
parser error : Failure to process entity sub-dev-subdev
<section id="subdev"> &sub-dev-subdev; </section>
^
/home/snawrocki/linux-media/media_tree/Documentation/DocBook/v4l2.xml:456:
parser error : Entity 'sub-dev-subdev' not defined
<section id="subdev"> &sub-dev-subdev; </section>
^
/usr/bin/xmlto: line 576: 27987 Segmentation fault "$XSLTPROC_PATH"
$XSLTOPTS -o "$XSLT_PROCESSED" "$STYLESHEET" "$INPUT_FILE"
/bin/cp: cannot stat `*.*htm*': No such file or directory
make[1]: *** [Documentation/DocBook/media_api.html] Error 1
make: *** [htmldocs] Error 2
------------------>8
[1]. http://patchwork.linuxtv.org/patch/8923/
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 2/3] v4l: Image source control class
2011-12-31 14:42 ` Sylwester Nawrocki
@ 2011-12-31 14:53 ` Sylwester Nawrocki
2012-01-01 12:05 ` Sakari Ailus
1 sibling, 0 replies; 11+ messages in thread
From: Sylwester Nawrocki @ 2011-12-31 14:53 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
On 12/31/2011 03:42 PM, Sylwester Nawrocki wrote:
>>
>> + case V4L2_CID_IMAGE_SOURCE_CLASS: return "Image source controls";
>> + case V4L2_CID_IMAGE_SOURCE_VBLANK: return "Vertical blanking";
>
> nit: have you considered making it "Blanking, horizontal"
Oops, it supposed to be: "Blanking, vertical"
>> + case V4L2_CID_IMAGE_SOURCE_HBLANK: return "Horizontal blanking";
>
> and "Blanking, vertical" ?
and "Blanking, horizontal" :/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 2/3] v4l: Image source control class
2011-12-31 14:42 ` Sylwester Nawrocki
2011-12-31 14:53 ` Sylwester Nawrocki
@ 2012-01-01 12:05 ` Sakari Ailus
1 sibling, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2012-01-01 12:05 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, laurent.pinchart, t.stanislaws, dacohen,
andriy.shevchenko, g.liakhovetski, hverkuil
Hi Sylwester,
Thanks for the review!
On Sat, Dec 31, 2011 at 03:42:51PM +0100, Sylwester Nawrocki wrote:
> On 12/14/2011 04:22 PM, Sakari Ailus wrote:
> > Add image source control class. This control class is intended to contain
> > low level controls which deal with control of the image capture process ---
> > the A/D converter in image sensors, for example.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > Documentation/DocBook/media/v4l/controls.xml | 101 ++++++++++++++++++++
> > .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 6 +
> > drivers/media/video/v4l2-ctrls.c | 10 ++
> > include/linux/videodev2.h | 10 ++
> > 4 files changed, 127 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> > index 3bc5ee8..69ede83 100644
> > --- a/Documentation/DocBook/media/v4l/controls.xml
> > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > @@ -3356,6 +3356,107 @@ interface and may change in the future.</para>
> > </table>
> >
> > </section>
> > +
> > + <section id="image-source-controls">
> > + <title>Image Source Control Reference</title>
> > +
> > + <note>
> > + <title>Experimental</title>
> > +
> > + <para>This is an <link
> > + linkend="experimental">experimental</link> interface and may
> > + change in the future.</para>
> > + </note>
> > +
> > + <para>
> > + The Image Source control class is intended for low-level
> > + control of image source devices such as image sensors. The
> > + devices feature an analogue to digital converter and a bus
> > + transmitter to transmit the image data out of the device.
> > + </para>
> > +
> > + <table pgwide="1" frame="none" id="image-source-control-id">
> > + <title>Image Source Control IDs</title>
> > +
> > + <tgroup cols="4">
> > + <colspec colname="c1" colwidth="1*" />
> > + <colspec colname="c2" colwidth="6*" />
> > + <colspec colname="c3" colwidth="2*" />
> > + <colspec colname="c4" colwidth="6*" />
> > + <spanspec namest="c1" nameend="c2" spanname="id" />
> > + <spanspec namest="c2" nameend="c4" spanname="descr" />
> > + <thead>
> > + <row>
> > + <entry spanname="id" align="left">ID</entry>
> > + <entry align="left">Type</entry>
> > + </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
> > + </row>
> > + </thead>
> > + <tbody valign="top">
> > + <row><entry></entry></row>
> > + <row>
> > + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry>
> > + <entry>class</entry>
> > + </row>
> > + <row>
> > + <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry>
> > + </row>
> > + <row>
> > + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry>
> > + <entry>integer</entry>
> > + </row>
> > + <row>
> > + <entry spanname="descr">Vertical blanking. The idle
> > + preriod after every frame during which no image data is
>
> s/preriod/period
>
> > + produced. The unit of vertical blanking is a line. Every
> > + line has length of the image width plus horizontal
> > + blanking at the pixel clock specified by struct
> > + v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt"
> > + />.</entry>
> > + </row>
> > + <row>
> > + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry>
> > + <entry>integer</entry>
> > + </row>
> > + <row>
> > + <entry spanname="descr">Horizontal blanking. The idle
> > + preriod after every line of image data during which no
>
> s/preriod/period
>
> > + image data is produced. The unit of horizontal blanking is
> > + pixels.</entry>
> > + </row>
> > + <row>
> > + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_LINK_FREQ</constant></entry>
> > + <entry>integer menu</entry>
> > + </row>
> > + <row>
> > + <entry spanname="descr">Image source's data bus frequency.
> > + Together with the media bus pixel code, bus type (clock
> > + cycles per sample), the data bus frequency defines the
> > + pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The
> > + frame rate can be calculated from the pixel clock, image
> > + width and height and horizontal and vertical blanking. The
> > + frame rate control is performed by selecting the desired
> > + horizontal and vertical blanking.
> > + </entry>
> > + </row>
> > + <row>
> > + <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></entry>
> > + <entry>integer</entry>
> > + </row>
> > + <row>
> > + <entry spanname="descr">Analogue gain is gain affecting
> > + all colour components in the pixel matrix. The gain
> > + operation is performed in the analogue domain before A/D
> > + conversion.
> > + </entry>
> > + </row>
> > + <row><entry></entry></row>
> > + </tbody>
> > + </tgroup>
> > + </table>
> > +
> > + </section>
> > +
> > </section>
> >
> > <!--
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > index 5122ce8..250c1cf 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > @@ -257,6 +257,12 @@ These controls are described in <xref
> > These controls are described in <xref
> > linkend="flash-controls" />.</entry>
> > </row>
> > + <row>
> > + <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry>
> > + <entry>0x9d0000</entry> <entry>The class containing image
> > + source controls. These controls are described in <xref
> > + linkend="image-source-controls" />.</entry>
> > + </row>
> > </tbody>
> > </tgroup>
> > </table>
> > diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> > index 083bb79..da1ec52 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -606,6 +606,12 @@ const char *v4l2_ctrl_get_name(u32 id)
> > case V4L2_CID_FLASH_CHARGE: return "Charge";
> > case V4L2_CID_FLASH_READY: return "Ready to strobe";
> >
> > + case V4L2_CID_IMAGE_SOURCE_CLASS: return "Image source controls";
> > + case V4L2_CID_IMAGE_SOURCE_VBLANK: return "Vertical blanking";
>
> nit: have you considered making it "Blanking, horizontal"
>
> > + case V4L2_CID_IMAGE_SOURCE_HBLANK: return "Horizontal blanking";
>
> and "Blanking, vertical" ?
I'll make the above four changes.
> > + case V4L2_CID_IMAGE_SOURCE_LINK_FREQ: return "Link frequency";
> > + case V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN: return "Analogue gain";
> > +
> > default:
> > return NULL;
> > }
> > @@ -694,6 +700,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:
> > *type = V4L2_CTRL_TYPE_MENU;
> > break;
> > + case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:
> > + *type = V4L2_CTRL_TYPE_INTEGER_MENU;
> > + break;
> > case V4L2_CID_RDS_TX_PS_NAME:
> > case V4L2_CID_RDS_TX_RADIO_TEXT:
> > *type = V4L2_CTRL_TYPE_STRING;
> > @@ -703,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > case V4L2_CID_MPEG_CLASS:
> > case V4L2_CID_FM_TX_CLASS:
> > case V4L2_CID_FLASH_CLASS:
> > + case V4L2_CID_IMAGE_SOURCE_CLASS:
> > *type = V4L2_CTRL_TYPE_CTRL_CLASS;
> > /* You can neither read not write these */
> > *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 9633c69..0f8f904 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1080,6 +1080,7 @@ struct v4l2_ext_controls {
> > #define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */
> > #define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator control class */
> > #define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */
> > +#define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009d0000 /* Image source flash controls */
> >
> > #define V4L2_CTRL_ID_MASK (0x0fffffff)
> > #define V4L2_CTRL_ID2CLASS(id) ((id) & 0x0fff0000UL)
> > @@ -1690,6 +1691,15 @@ enum v4l2_flash_strobe_source {
> > #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
> > #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
> >
> > +/* Image source controls */
> > +#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
> > +#define V4L2_CID_IMAGE_SOURCE_CLASS (V4L2_CTRL_CLASS_IMAGE_SOURCE | 1)
> > +
> > +#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
> > +#define V4L2_CID_IMAGE_SOURCE_HBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
> > +#define V4L2_CID_IMAGE_SOURCE_LINK_FREQ (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
> > +#define V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
>
> Since these are really low level controls, do you think we could shorten
> the class name a bit, i.e. make it V4L2_CID_IMG_SRC_CLASS ? :)
Fine for me.
> Somehow I'm getting the misterious error again when compiling after this
> patch is applied (and others from you last series of 17, xmlto version
> 0.0.23, perl, v5.10.1):
>
> 8<--------------------
>
> HTML Documentation/DocBook/media_api.html
> warning: failed to load external entity
> "/home/snawrocki/linux-media/media_tree/Documentation/DocBook/vidioc-subdev-g-selection.xml"
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> The problem is in patch "v4l: Add selections documentation." [1].
> Somehow file vidioc-subdev-g-selection.xml doesn't get copied to
> Documentation/DocBook/ directory. When I copy it manually it compiles
> fine. I'm looking now at Documentation/DocBook/Makefile to find the
> reason of that.
>
> /home/snawrocki/linux-media/media_tree/Documentation/DocBook/dev-subdev.xml:295:
> parser error : Failure to process entity sub-subdev-g-selection
> configured using &sub-subdev-g-selection; and
> ^
> /home/snawrocki/linux-media/media_tree/Documentation/DocBook/dev-subdev.xml:295:
> parser error : Entity 'sub-subdev-g-selection' not defined
> configured using &sub-subdev-g-selection; and
> ^
> /home/snawrocki/linux-media/media_tree/Documentation/DocBook/dev-subdev.xml:369:
> parser error : chunk is not well balanced
>
> ^
> /home/snawrocki/linux-media/media_tree/Documentation/DocBook/v4l2.xml:456:
> parser error : Failure to process entity sub-dev-subdev
> <section id="subdev"> &sub-dev-subdev; </section>
> ^
> /home/snawrocki/linux-media/media_tree/Documentation/DocBook/v4l2.xml:456:
> parser error : Entity 'sub-dev-subdev' not defined
> <section id="subdev"> &sub-dev-subdev; </section>
> ^
> /usr/bin/xmlto: line 576: 27987 Segmentation fault "$XSLTPROC_PATH"
> $XSLTOPTS -o "$XSLT_PROCESSED" "$STYLESHEET" "$INPUT_FILE"
> /bin/cp: cannot stat `*.*htm*': No such file or directory
> make[1]: *** [Documentation/DocBook/media_api.html] Error 1
> make: *** [htmldocs] Error 2
That shouldn't of course happen with the patchset. I'll see how that goes
early next week. Btw. I really love the xmlto's way of error handling. ;)
Cheers,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-01-01 12:05 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-01 14:30 [RFC] On controlling sensors Sakari Ailus
2011-12-14 15:22 ` [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt Sakari Ailus
2011-12-15 21:46 ` Sylwester Nawrocki
2011-12-15 22:01 ` Sakari Ailus
2011-12-15 22:19 ` Sylwester Nawrocki
2011-12-16 7:40 ` Sakari Ailus
2011-12-14 15:22 ` [RFC 2/3] v4l: Image source control class Sakari Ailus
2011-12-31 14:42 ` Sylwester Nawrocki
2011-12-31 14:53 ` Sylwester Nawrocki
2012-01-01 12:05 ` Sakari Ailus
2011-12-14 15:22 ` [RFC 3/3] v4l: Add pad op for pipeline validation Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox