Hi Laurent, Laurent Pinchart wrote: > On Saturday 14 January 2012 21:51:31 Sakari Ailus wrote: >> Laurent Pinchart wrote: >>> On Tuesday 20 December 2011 21:28:00 Sakari Ailus wrote: >>>> From: Sakari Ailus <sakari.ailus@xxxxxx> >>>> >>>> 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@xxxxxx> >>>> --- >>>> >>>> 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 >>> >>> Is the V4L2 documentation written in US or UK English ? US uses analog, >>> UK uses analogue. Analog would be shorter in control names. >> >> Both appear to be used, but the US English appears to be more commonly >> used. I guess it's mostly chosen by whatever happened to be used by the >> author of the patch. I prefer UK spelling which you might have noticed >> already. :-) > > Yes I have. I haven't checked whether V4L2 prefers the UK or US spelling. I'll > trust you on that. I have checked and most seem to have used US spelling. If you wish me to change it, I can do that. >> I remember there was a discussion on this topic years ago within the >> kernel community but I don't remember how it ended up with... LWN.net >> appears to remember better than I do, but by a quick check I can't find >> any definitive conclusion. >> >> <URL:http://lwn.net/Articles/44262/> >> <URL:http://lkml.org/lkml/2003/8/7/245> >> >>>> + 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></entr >>>> y> + <entry>integer menu</entry> >>>> + </row> >>>> + <row> >>>> + <entry spanname="descr">Image source's data bus frequency. >>> >>> What's the frequency unit ? Sample/second ? >> >> Hz --- that's the unit of frequency. I fixed that in the new version. > > Is the user supposed to compute the pixel clock from this information ? That's > what the text below seems to imply. Apparently I have forgotten to update this in the new patchset. But yes, these factors do define it. The sensors' internal clock tree will be involved and calculation is non-trivial. This is why we also have the PIXEL_RATE control --- where I will refer to in the next patchset. >>>> + 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></ >>>> en try> + <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. >>> >>> Should we define one gain per color component ? >> >> I think that in the end we may need up to six analogue gains: >> >> - Gain for all components > > Many sensors that provide per-component gains also provide a global gain > control that sets the four component gains. I'm not sure how (and if) we > should handle that. If it directly affects all of them, I don't think we should support it. But if it's independent of the colour-specific ones, then, sure, it should be supported. >> - Blue gain >> - Red gain >> - Green gain (for both greens) >> - Gr gain >> - Gb gain >> >> It may be debatable whether Gr / Gb gain will always be the same or not. >> I'm not fully certain about that. As Hans G. suggested, it might be >> possible to go with just one for green. > > I'm also unsure about that. Having different gains for the two green > components doesn't seem very useful. On the other hand, if we find a use case > later, we'll have to break driver ABIs. Let's add the gains later on. Now we'll need a single analogue gain for all components and that's good for the time being. >>>> + </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"; >>> >>> Please capitalize each word, as done for the other controls. >> >> This isn't done for the flash controls either, have you noticed that? >> >> Well, I guess I have to admit that they were added by myself. ;-) >> >> I can fix this for the next patchset. >> >>>> 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; >>> >>> Will this always be an integer menu control, or can you foresee cases >>> where the range would be continuous ? >> >> Good question. On some hardware this definitely is an integer menu, but >> hardware may exist where more selections would be available. >> >> However, on e.g. the SMIA++ sensor the calculation of the clock tree >> values depends heavily on the selected link rate. Choosing a wrong link >> rate may yield to the clock tree calculation to fail. So the driver >> likely would need to enforce some rules which values are allowed. That >> might prove unfeasible --- already the PLL code in the SMIA++ driver is >> relatively complex. >> >> I don't see much benefit in being able to specify this freely. > > For SMIA++, definitely not. For other sensors, I don't know. At least one Aptina sensor had a clock tree which basically had a few dividers and a multiplier. The same restrictions apply. >> AFAIR the conclusion was that controls may only have one type when the >> control framework was written. > > Yes, but I'm not sure whether that's a good conclusion :-) It allows you to assume a type for controls, whether that's good or not. This could be one use case for that, but I don't really see much advantage in (attepmpting) to support fully free sensor link frequency configuration. Regards, -- Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
