| Message ID | 20160605190716.GA11321@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 5.06.2016 22:07, Pavel Machek wrote: > Add userspace API definitions. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index b6a357a..23011cc 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -974,4 +975,9 @@ enum v4l2_detect_md_mode { > #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3) > #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4) > > +/* Control IDs specific to the AD5820 driver as defined by V4L2 */ > +#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af) > +#define V4L2_CID_FOCUS_AD5820_RAMP_TIME (V4L2_CID_FOCUS_AD5820_BASE+0) > +#define V4L2_CID_FOCUS_AD5820_RAMP_MODE (V4L2_CID_FOCUS_AD5820_BASE+1) > + > #endif > Sakari, what about adding those as standard camera controls? It seems ad5820 is not the only VCM driver to implement "antiringing" controls, http://rohmfs.rohm.com/en/products/databook/datasheet/ic/motor/mobile_module/bu64241gwz-e.pdf is another example I found by quick search. What about: #define V4L2_CID_FOCUS_STEP_MODE xxx enum v4l2_cid_focus_step_mode { V4L2_CID_FOCUS_STEP_MODE_DIRECT, V4L2_CID_FOCUS_STEP_MODE_LINEAR, V4L2_CID_FOCUS_STEP_MODE_AUTO }; #define V4L2_CID_FOCUS_STEP_TIME xxx+1 Also, how the userspace(or the kernel) is notified by v4l that there is an event? The point is - I think it is a good idea to notify when VCM has completed its movement, we can start a timer based on the current position, mode, step time etc and notify after the pre-calculated movement time. Here ftp://ftp.analog.com/pub/evalcd/AD5820_v1_0/AD5820_Quickstart.pdf can be found the modes/timings description for ad5820 along with the equations needed to calculate timings etc. Ivo
Hi! > On 5.06.2016 22:07, Pavel Machek wrote: > >Add userspace API definitions. > > > >Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > >diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > >index b6a357a..23011cc 100644 > >--- a/include/uapi/linux/v4l2-controls.h > >+++ b/include/uapi/linux/v4l2-controls.h > >@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode { > > #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3) > > #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4) > > > >+/* Control IDs specific to the AD5820 driver as defined by V4L2 */ > >+#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af) > >+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME (V4L2_CID_FOCUS_AD5820_BASE+0) > >+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE (V4L2_CID_FOCUS_AD5820_BASE+1) > >+ > > #endif > > > > Sakari, what about adding those as standard camera controls? It seems ad5820 > is not the only VCM driver to implement "antiringing" controls, http://rohmfs.rohm.com/en/products/databook/datasheet/ic/motor/mobile_module/bu64241gwz-e.pdf > is another example I found by quick search. Well, standartized API may be good idea... but I'd really like the driver to go in, and it looks like camera application needs to know quite a lot of details about the autofocus subsystem. > > What about: > > #define V4L2_CID_FOCUS_STEP_MODE xxx > enum v4l2_cid_focus_step_mode { > V4L2_CID_FOCUS_STEP_MODE_DIRECT, > V4L2_CID_FOCUS_STEP_MODE_LINEAR, > V4L2_CID_FOCUS_STEP_MODE_AUTO > }; > #define V4L2_CID_FOCUS_STEP_TIME xxx+1 > > Also, how the userspace(or the kernel) is notified by v4l that there is an > event? The point is - I think it is a good idea to notify when VCM has > completed its movement, we can start a timer based on the current > position, Why? Look at how fcam-dev/ works. It is not interested when movement is "done". It sets the focus to one distance, then says it to slowly refocus to another distance, and watches the stream for sharpness. When it is sharp, it computes likely lens position at the time of sharpness, and asks hardware to go back there. Best regards, Pavel
Hi Ivaylo, On Mon, Jun 06, 2016 at 09:06:29AM +0300, Ivaylo Dimitrov wrote: > Hi, > > On 5.06.2016 22:07, Pavel Machek wrote: > >Add userspace API definitions. > > > >Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > >diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > >index b6a357a..23011cc 100644 > >--- a/include/uapi/linux/v4l2-controls.h > >+++ b/include/uapi/linux/v4l2-controls.h > >@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode { > > #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3) > > #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4) > > > >+/* Control IDs specific to the AD5820 driver as defined by V4L2 */ > >+#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af) Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled nowadays. > >+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME (V4L2_CID_FOCUS_AD5820_BASE+0) > >+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE (V4L2_CID_FOCUS_AD5820_BASE+1) > >+ > > #endif > > > > Sakari, what about adding those as standard camera controls? It seems ad5820 > is not the only VCM driver to implement "antiringing" controls, http://rohmfs.rohm.com/en/products/databook/datasheet/ic/motor/mobile_module/bu64241gwz-e.pdf > is another example I found by quick search. These controls however seem to be related to some thing else --- configuring the driver to drive the lens little by little to the target using a pre-defined step time and size. I presume it is intended for something else, most likely for performing a full search for a regular AF algorithm. I also wonder how much this functionality is used nowadays, most devices use continuous AF algorithm that has little or no use for such. It also provides no help in synchronising lens movement to exposure of the AF window. Now that I think about this, the original implementation in N900 very likely did not use either of the two controls; the device driver was still written to provide access to full capabilities of the chip. And that one had no continuous AF. > What about: > > #define V4L2_CID_FOCUS_STEP_MODE xxx > enum v4l2_cid_focus_step_mode { > V4L2_CID_FOCUS_STEP_MODE_DIRECT, > V4L2_CID_FOCUS_STEP_MODE_LINEAR, > V4L2_CID_FOCUS_STEP_MODE_AUTO > }; > #define V4L2_CID_FOCUS_STEP_TIME xxx+1 > > Also, how the userspace(or the kernel) is notified by v4l that there is an > event? The point is - I think it is a good idea to notify when VCM has > completed its movement, we can start a timer based on the current position, > mode, step time etc and notify after the pre-calculated movement time. You'd have to start modelling the movement of the lens somehow. And for that, we'd need to know about the lens and the spring in the kernel, too. I presume that's already a lot of the algorithm to get the lens moving, and supposing this is in the kernel, what else would go to the kernel? These device provide very basic functionality for moving the lens; current is applied on a coil and that has the effect of moving the lens, bringing the focus distance closer as more current is applied but that's pretty much it: there's no reliable way to focus at a particular distance for example. I might as well drop the two controls, up to you. If someone ever needs them they can always be reintroduced. I'd be happy to get a new patch, the current driver patch does not compile (just tried) as the definitions of these controls are missing. Ringing compensation functionality present in the other chip should be far more useful. If there are AF experts reading this, feel free to challenge me. :-) I can't claim to be one.
Hi! > > >@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode { > > > #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3) > > > #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4) > > > > > >+/* Control IDs specific to the AD5820 driver as defined by V4L2 */ > > >+#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af) > > Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled > nowadays. Let me see... > Now that I think about this, the original implementation in N900 very likely > did not use either of the two controls; the device driver was still written > to provide access to full capabilities of the chip. And that one had no > continuous AF. I'm not sure about the original implementation, but fcam-dev library (which is our best chance for usable camera) does use both: pavel@duo:~/g/fcam-dev$ grep -ri RAMP_TIME . ./.svn/pristine/05/0574680922f59e07bd49e16a951d69421690a323.svn-base: int val = ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_TIME, 1000000.0f/diopterRateToTickRate(speed)); ./src/N900/Lens.cpp: int val = ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_TIME, 1000000.0f/diopterRateToTickRate(speed)); pavel@duo:~/g/fcam-dev$ grep -ri RAMP_MODE . ./.svn/pristine/05/0574680922f59e07bd49e16a951d69421690a323.svn-base: ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_MODE, 0); ./src/N900/Lens.cpp: ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_MODE, 0); pavel@duo:~/g/fcam-dev$ > I might as well drop the two controls, up to you. If someone ever needs them > they can always be reintroduced. I'd be happy to get a new patch, the > current driver patch does not compile (just tried) as the definitions of > these controls are missing. I'd prefer to keep the controls, as we have userspace using them. I got it to compile but have yet to get it to work (subdevs split, so it needs some modifications). Best regards, Pavel
Hi Pavel, On Sun, Jun 12, 2016 at 09:54:17AM +0200, Pavel Machek wrote: > Hi! > > > > >@@ -974,4 +975,9 @@ enum v4l2_detect_md_mode { > > > > #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3) > > > > #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4) > > > > > > > >+/* Control IDs specific to the AD5820 driver as defined by V4L2 */ > > > >+#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af) > > > > Please check V4L2_CID_USER_*_BASE. That's how custom controls are handled > > nowadays. > > Let me see... > > > Now that I think about this, the original implementation in N900 very likely > > did not use either of the two controls; the device driver was still written > > to provide access to full capabilities of the chip. And that one had no > > continuous AF. > > I'm not sure about the original implementation, but fcam-dev library > (which is our best chance for usable camera) does use both: > > pavel@duo:~/g/fcam-dev$ grep -ri RAMP_TIME . > ./.svn/pristine/05/0574680922f59e07bd49e16a951d69421690a323.svn-base: > int val = ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_TIME, > 1000000.0f/diopterRateToTickRate(speed)); > ./src/N900/Lens.cpp: int val = > ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_TIME, > 1000000.0f/diopterRateToTickRate(speed)); > pavel@duo:~/g/fcam-dev$ grep -ri RAMP_MODE . > ./.svn/pristine/05/0574680922f59e07bd49e16a951d69421690a323.svn-base: > ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_MODE, 0); > ./src/N900/Lens.cpp: ioctlSet(V4L2_CID_FOCUS_AD5820_RAMP_MODE, 0); > pavel@duo:~/g/fcam-dev$ > > > I might as well drop the two controls, up to you. If someone ever needs them > > they can always be reintroduced. I'd be happy to get a new patch, the > > current driver patch does not compile (just tried) as the definitions of > > these controls are missing. > > I'd prefer to keep the controls, as we have userspace using them. I > got it to compile but have yet to get it to work (subdevs split, so it > needs some modifications). Right. I didn't know Fcam used them. Still, using them is hardly an optimal way to control the lens (as far as camera functionality goes, using them requires less CPU time consumption though). The flash control patches should receive a proper RFC that discusses the problem area and proposes a solution. I'll write one in the near future. I think that for this particular controller it's relatively clear though: it provides very basic functionality that maps well to controls that I don't really see alternative options for that. I don't object defining standard controls for ramp mode nor time either. But I expect you to write a patch in that case. :-)
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index b6a357a..23011cc 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -974,4 +975,9 @@ enum v4l2_detect_md_mode { #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3) #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4) +/* Control IDs specific to the AD5820 driver as defined by V4L2 */ +#define V4L2_CID_FOCUS_AD5820_BASE (V4L2_CTRL_CLASS_CAMERA | 0x10af) +#define V4L2_CID_FOCUS_AD5820_RAMP_TIME (V4L2_CID_FOCUS_AD5820_BASE+0) +#define V4L2_CID_FOCUS_AD5820_RAMP_MODE (V4L2_CID_FOCUS_AD5820_BASE+1) + #endif
Add userspace API definitions. Signed-off-by: Pavel Machek <pavel@ucw.cz>