From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-bk0-f46.google.com ([209.85.214.46]:40897 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756786Ab2EIRjW (ORCPT ); Wed, 9 May 2012 13:39:22 -0400 Received: by mail-bk0-f46.google.com with SMTP id ji2so519913bkc.19 for ; Wed, 09 May 2012 10:39:21 -0700 (PDT) Message-ID: <4FAAABC6.6020709@gmail.com> Date: Wed, 09 May 2012 19:39:18 +0200 From: Sylwester Nawrocki MIME-Version: 1.0 To: Sylwester Nawrocki CC: Sakari Ailus , linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com, g.liakhovetski@gmx.de, hdegoede@redhat.com, moinejf@free.fr, hverkuil@xs4all.nl, m.szyprowski@samsung.com, riverful.kim@samsung.com, sw0312.kim@samsung.com, Kyungmin Park Subject: Re: [PATCH/RFC v4 12/12] V4L: Add camera auto focus controls References: <1336156337-10935-1-git-send-email-s.nawrocki@samsung.com> <1336156337-10935-13-git-send-email-s.nawrocki@samsung.com> <4FA6C6ED.7020406@iki.fi> <4FAA42C4.9050301@samsung.com> In-Reply-To: <4FAA42C4.9050301@samsung.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 05/09/2012 12:11 PM, Sylwester Nawrocki wrote: > On 05/06/2012 08:46 PM, Sakari Ailus wrote: >> Sylwester Nawrocki wrote: >>> Add following auto focus controls: >>> >>> - V4L2_CID_AUTO_FOCUS_START - single-shot auto focus start >>> - V4L2_CID_AUTO_FOCUS_STOP - single-shot auto focus stop >>> - V4L2_CID_AUTO_FOCUS_STATUS - automatic focus status >>> - V4L2_CID_AUTO_FOCUS_AREA - automatic focus area selection >>> - V4L2_CID_AUTO_FOCUS_DISTANCE - automatic focus scan range selection >>> >>> Signed-off-by: Sylwester Nawrocki >>> Signed-off-by: Kyungmin Park >>> --- >>> Documentation/DocBook/media/v4l/controls.xml | 147 +++++++++++++++++++++++++- >>> drivers/media/video/v4l2-ctrls.c | 31 +++++- >>> include/linux/videodev2.h | 25 +++++ >>> 3 files changed, 200 insertions(+), 3 deletions(-) >>> >>> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml >>> index 4a463d3..d8ef71e 100644 >>> --- a/Documentation/DocBook/media/v4l/controls.xml >>> +++ b/Documentation/DocBook/media/v4l/controls.xml >>> @@ -2902,13 +2902,156 @@ negative values towards infinity. This is a write-only control. >>> >>> V4L2_CID_FOCUS_AUTO  >>> boolean >>> - Enables automatic focus >>> -adjustments. The effect of manual focus adjustments while this feature >>> + Enables continuous automatic >>> +focus adjustments. The effect of manual focus adjustments while this feature >>> is enabled is undefined, drivers should ignore such requests. >>> >>> >>> >>> >>> + V4L2_CID_AUTO_FOCUS_START  >>> + button >>> + Starts single auto focus process. >>> +The effect of setting this control whenV4L2_CID_FOCUS_AUTO >>> +is set toTRUE (1) is undefined, drivers should ignore >>> +such requests. >>> + >>> + >>> + >>> + >>> + V4L2_CID_AUTO_FOCUS_STOP  >>> + button >>> + Aborts automatic focusing >>> +started withV4L2_CID_AUTO_FOCUS_START control. It is >>> +effective only when the continuous autofocus is disabled, that is when >>> +V4L2_CID_FOCUS_AUTO control is set toFALSE >>> + (0). >>> + >>> + >>> + >>> + >>> + >>> + V4L2_CID_AUTO_FOCUS_STATUS  >>> + bitmask >>> + >>> + The automatic focus status. This is a read-only >>> + control. >>> + >>> + >>> + >>> + >>> + >>> + V4L2_AUTO_FOCUS_STATUS_IDLE  >>> + Automatic focus is not active. >>> + >>> + >>> + V4L2_AUTO_FOCUS_STATUS_BUSY  >>> + Automatic focusing is in progress. >>> + >>> + >>> + V4L2_AUTO_FOCUS_STATUS_REACHED  >>> + Focus has been reached. >>> + >>> + >>> + V4L2_AUTO_FOCUS_STATUS_LOST  >>> + Focus has been lost. >> >> When does this happen? > > Hmm, good question. I intended this one for continuous auto focus, for the moments > when the focus is lost. I felt the control is incomplete without such status bit. > > Thinking about it a bit more, it is just a negation of V4L2_AUTO_FOCUS_STATUS_FOCUSED. ^^^^^^^^^ Sorry, that should be V4L2_AUTO_FOCUS_STATUS_REACHED. > The focus lost notifications could be well provided to user space be clearing this bit. > > So I would just get rid of V4L2_AUTO_FOCUS_STATUS_LOST, I don't really use it in any > driver, it was supposed to be just for completeness. > > What do you think ? > >>> + >>> + >>> + V4L2_AUTO_FOCUS_STATUS_FAILED  >>> + Automatic focus has failed, the driver will not >>> + transition from this state until another action is >>> + performed by an application. >> >> Which of these are valid for regular autofocus and which ones for >> continuous autofocus? I'm a little bit confused with the above descriptions. > > All, except V4L2_AUTO_FOCUS_STATUS_LOST are valid for both auto focus modes. > But I'm going to remove V4L2_AUTO_FOCUS_STATUS_LOST bit, as indicated above. > >> I might as well say that temporary conditions such as failed and reached >> would return to idle after being read from user space. This is how the >> flash faults behave, too. > > I'm not sure if it would be possible to fulfil such assumption in drivers > in all cases. The status often comes from hardware and the driver might > not be able to change it at will. I'm not sure if it is safe to change > state just by reading it from user-space. This probably wouldn't work well > with multiple processes accessing the camera. > > For instance, from state V4L2_AUTO_FOCUS_STATUS_FAILED a driver would have > transitioned to something else after user-space sets V4L2_CID_AUTO_FOCUS_START > control. Also I would prefer having V4L2_AUTO_FOCUS_STATUS_REACHED bit set > as long as the camera stays in this state, regardless of how many status > readers there are. > >> >> How does this interact with the 3A lock control? > > Setting V4L2_LOCK_FOCUS lock would just stop updates to the status control > value. The 3A lock control is just another one that influences the auto > focus status, among V4L2_CID_AUTO_FOCUS_START and V4L2_CID_AUTO_FOCUS_STOP. > > Nevertheless, I see your point, that it's not clear from the Spec. > How about adding something like this to the AF status control description: > > "Setting V4L2_LOCK_FOCUS lock may stop updates of the V4L2_CID_AUTO_FOCUS_STOP ^^^^^ And this - V4L2_CID_AUTO_FOCUS_STATUS. I guess I need more sleeping, and not on the computer keyboard... :-) The updated patch series is also available here: http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/v4l-camera-controls > control value." > > ? > > I'm not sure how much detailed the documentation should be, I wouldn't like > to add something that would be hard to implement in drivers, for sake of > the applications' simplicity... :) > >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> + V4L2_CID_AUTO_FOCUS_RANGE  >>> + enum v4l2_auto_focus_range >>> + >>> + Determines auto focus distance range >>> +for which lens may be adjusted. >>> + >>> + >>> + >>> + >>> + >>> + V4L2_AUTO_FOCUS_RANGE_AUTO  >>> + The camera automatically selects the focus range. >>> + >>> + >>> + V4L2_AUTO_FOCUS_RANGE_NORMAL  >>> + The auto focus normal distance range. It is limited >>> +for best auto focus algorithm performance. >>> + >>> + >>> + V4L2_AUTO_FOCUS_RANGE_MACRO  >>> + Macro (close-up) auto focus. The camera will >>> +use minimum possible distance that it is capable of for auto focus. >>> + >>> + >>> + V4L2_AUTO_FOCUS_RANGE_INFINITY  >>> + The focus at an object at infinite distance. >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> + >>> + V4L2_CID_AUTO_FOCUS_AREA  >>> + enum v4l2_auto_focus_area >>> + >>> + Determines the area of the frame that >>> +the camera uses for automatic focus. The corresponding coordinates of the >>> +focusing spot or rectangle can be specified and queried using the selection API. >>> +To change the auto focus region of interest applications first select required >>> +mode of this control and then set the rectangle or spot coordinates by means >>> +of the&VIDIOC-SUBDEV-S-SELECTION; or&VIDIOC-S-SELECTION; ioctl. In order to >>> +trigger again an auto focus process with same coordinates applications should >>> +use theV4L2_CID_AUTO_FOCUS_START control. Or alternatively >>> +invoke a&VIDIOC-SUBDEV-S-SELECTION; or a&VIDIOC-S-SELECTION; ioctl again. >>> +In the latter case the new pixel coordinates are applied to hardware only when >>> +the focus area control is set to a value other than >>> +V4L2_AUTO_FOCUS_AREA_ALL. >>> + >>> + >>> + >>> + >>> + >>> + V4L2_AUTO_FOCUS_AREA_ALL  >>> + Normal auto focus, the focusing area extends over the >>> +entire frame. >>> + >>> + >>> + V4L2_AUTO_FOCUS_AREA_SPOT  >>> + Automatic focus on a spot within the frame at position >>> +specified by theV4L2_SEL_TGT_AUTO_FOCUS_ACTUAL or >>> +V4L2_SUBDEV_SEL_TGT_AUTO_FOCUS_ACTUAL selection. When these >>> +selections are not supported by driver the default spot's position is center of >>> +the frame. >>> + >>> + >>> + V4L2_AUTO_FOCUS_AREA_RECTANGLE  >>> + The auto focus area is determined by the >>> +V4L2_SEL_TGT_AUTO_FOCUS_ACTUAL or >>> +V4L2_SUBDEV_SEL_TGT_AUTO_FOCUS_ACTUAL selection rectangle. >>> + >>> + >>> + V4L2_AUTO_FOCUS_AREA_FACE_DETECTION  >>> + The camera automatically focuses on a detected face >>> +area. >> >> I assume there could be one or more faces to focus to, right? > > Indeed, I presume we're going to need another set of controls for Face Detection > features. > > That's true, there can be more faces. I wasn't good enough at expressing this > here :( > > Maybe something like: > > "The camera automatically focuses on the face detection regions." > > would be better ? -- Regards, Sylwester