Hi Pavel,
On Wed, May 31, 2017 at 09:58:21PM +0200, Pavel Machek wrote:
> Hi!
>
> > +/* min/typical/max system clock (xclk) frequencies */
> > +#define OV5640_XCLK_MIN 6000000
> > +#define OV5640_XCLK_MAX 24000000
> > +
> > +/*
> > + * FIXME: there is no subdev API to set the MIPI CSI-2
> > + * virtual channel yet, so this is hardcoded for now.
> > + */
> > +#define OV5640_MIPI_VC 1
>
> Can the FIXME be fixed?
Yes, but it's quite a bit of work. It makes sense to use a static virtual
channel for now. A patchset which is however incomplete can be found here:
<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=vc>
For what it's worth, all other devices use virtual channel zero for image
data and so should this one.
>
> > +/*
> > + * image size under 1280 * 960 are SUBSAMPLING
>
> -> Image
>
> > + * image size upper 1280 * 960 are SCALING
>
> above?
>
> > +/*
> > + * FIXME: all of these register tables are likely filled with
> > + * entries that set the register to their power-on default values,
> > + * and which are otherwise not touched by this driver. Those entries
> > + * should be identified and removed to speed register load time
> > + * over i2c.
> > + */
>
> load->loading? Can the FIXME be fixed?
>
> > + /* Auto/manual exposure */
> > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > + V4L2_CID_EXPOSURE_AUTO,
> > + V4L2_EXPOSURE_MANUAL, 0,
> > + V4L2_EXPOSURE_AUTO);
> > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > + V4L2_CID_EXPOSURE_ABSOLUTE,
> > + 0, 65535, 1, 0);
>
> Is exposure_absolute supposed to be in microseconds...?
Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
Ideally we should have only one control for exposure.
--
Regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > > + /* Auto/manual exposure */
> > > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > > + V4L2_CID_EXPOSURE_AUTO,
> > > + V4L2_EXPOSURE_MANUAL, 0,
> > > + V4L2_EXPOSURE_AUTO);
> > > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > > + V4L2_CID_EXPOSURE_ABSOLUTE,
> > > + 0, 65535, 1, 0);
> >
> > Is exposure_absolute supposed to be in microseconds...?
>
> Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
>
> Ideally we should have only one control for exposure.
No. N-o. No no no. NO! No. N-o. NONONO. No. NooooooooooOOO!!!!!!!!!!!!!
Sorry, no.
Userspace needs to know exposure times. It is not so important for a
webcam, but it is mandatory for digital camera. As it gets darker,
autogain wants to scale exposure to cca 1/100 sec, then it wants to
scale gain up to maximum, and only then it wants to continue scaling
exposure. (Threshold will be shorter in "sports" mode, perhaps
1/300sec?)
Plus, we want user to be able to manually set exposure parameters.
So... _this_ driver probably should use V4L2_CID_EXPOSURE. (If the
units are not known). But in general we'd prefer drivers using
V4L2_CID_EXPOSURE_ABSOLUTE. Your car has speedometer calibrated in
km/h or mph, not in "% of max", right?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 06/01/2017 01:26 AM, Sakari Ailus wrote:
> Hi Pavel,
>
> On Wed, May 31, 2017 at 09:58:21PM +0200, Pavel Machek wrote:
>> Hi!
>>
>>> +/* min/typical/max system clock (xclk) frequencies */
>>> +#define OV5640_XCLK_MIN 6000000
>>> +#define OV5640_XCLK_MAX 24000000
>>> +
>>> +/*
>>> + * FIXME: there is no subdev API to set the MIPI CSI-2
>>> + * virtual channel yet, so this is hardcoded for now.
>>> + */
>>> +#define OV5640_MIPI_VC 1
>>
>> Can the FIXME be fixed?
>
> Yes, but it's quite a bit of work. It makes sense to use a static virtual
> channel for now. A patchset which is however incomplete can be found here:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=vc>
>
> For what it's worth, all other devices use virtual channel zero for image
> data and so should this one.
Actually no. The CSI2IPU gasket in i.MX6 quad sends virtual channel 0
streams to IPU1-CSI0. But input to IPU1-CSI0 is also muxed with parallel
bus cameras. So if vc0 were chosen instead, platforms that support
parallel cameras to IPU1-CSI0 (SabreLite, SabreSD) would not be able
to use them concurrently with a MIPI CSI-2 source to IPU1-CSI1. So I
prefer to use static channel 1 to support those platforms.
I could convert this to a module parameter however, until a virtual
channel selection subdev API becomes available, at which point that
would have to be stripped.
>
>>
>>> +/*
>>> + * image size under 1280 * 960 are SUBSAMPLING
>>
>> -> Image
>>
>>> + * image size upper 1280 * 960 are SCALING
>>
>> above?
>>
done.
>>> +/*
>>> + * FIXME: all of these register tables are likely filled with
>>> + * entries that set the register to their power-on default values,
>>> + * and which are otherwise not touched by this driver. Those entries
>>> + * should be identified and removed to speed register load time
>>> + * over i2c.
>>> + */
>>
>> load->loading? Can the FIXME be fixed?
That's a lot of work, and risky work at that. If someone could take
this on (strip out power-on default values from the tables), I'd
be grateful, but I don't have the time. For now at least, these
registers sets work fine.
>>
>>> + /* Auto/manual exposure */
>>> + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
>>> + V4L2_CID_EXPOSURE_AUTO,
>>> + V4L2_EXPOSURE_MANUAL, 0,
>>> + V4L2_EXPOSURE_AUTO);
>>> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
>>> + V4L2_CID_EXPOSURE_ABSOLUTE,
>>> + 0, 65535, 1, 0);
>>
>> Is exposure_absolute supposed to be in microseconds...?
>
> Yes.
According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
taking 100 usec units, so unit-less is better.
Steve
Hi!
> >>>+ /* Auto/manual exposure */
> >>>+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> >>>+ V4L2_CID_EXPOSURE_AUTO,
> >>>+ V4L2_EXPOSURE_MANUAL, 0,
> >>>+ V4L2_EXPOSURE_AUTO);
> >>>+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> >>>+ V4L2_CID_EXPOSURE_ABSOLUTE,
> >>>+ 0, 65535, 1, 0);
> >>
> >>Is exposure_absolute supposed to be in microseconds...?
> >
> >Yes.
>
> According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
>
> OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
>
> Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> taking 100 usec units, so unit-less is better.
Thanks. If you know the units, it would be of course better to use
right units...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote:
> Hi!
>
> > >>>+ /* Auto/manual exposure */
> > >>>+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > >>>+ V4L2_CID_EXPOSURE_AUTO,
> > >>>+ V4L2_EXPOSURE_MANUAL, 0,
> > >>>+ V4L2_EXPOSURE_AUTO);
> > >>>+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > >>>+ V4L2_CID_EXPOSURE_ABSOLUTE,
> > >>>+ 0, 65535, 1, 0);
> > >>
> > >>Is exposure_absolute supposed to be in microseconds...?
> > >
> > >Yes.
> >
> > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> >
> > OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> >
> > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> > taking 100 usec units, so unit-less is better.
>
> Thanks. If you know the units, it would be of course better to use
> right units...
Steve: what's the unit in this case? Is it lines or something else?
Pavel: we do need to make sure the user space will be able to know the unit,
too. It's rather a case with a number of controls: the unit is known but
there's no API to convey it to the user.
The exposure is a bit special, too: granularity matters a lot on small
values. On most other controls it does not.
--
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> > >
> > > OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> > > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> > >
> > > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> > > taking 100 usec units, so unit-less is better.
> >
> > Thanks. If you know the units, it would be of course better to use
> > right units...
>
> Steve: what's the unit in this case? Is it lines or something else?
>
> Pavel: we do need to make sure the user space will be able to know the unit,
> too. It's rather a case with a number of controls: the unit is known but
> there's no API to convey it to the user.
>
> The exposure is a bit special, too: granularity matters a lot on small
> values. On most other controls it does not.
Yeah. Basically problem with exposure is that the control is
logarithmic; by using linear scale we got too much resolution at long
times and too little resolution at short times.
(Plus, 100 usec ... n900 can do times _way_ shorter than that.)
Anyway, even u32 gives us enough range, but I so the linear/log
confusion does not matter. But it would be nicer if values were in 10
usec or usec, not 100 usec...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 06/03/2017 02:57 PM, Sakari Ailus wrote:
> On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote:
>> Hi!
>>
>>>>>> + /* Auto/manual exposure */
>>>>>> + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
>>>>>> + V4L2_CID_EXPOSURE_AUTO,
>>>>>> + V4L2_EXPOSURE_MANUAL, 0,
>>>>>> + V4L2_EXPOSURE_AUTO);
>>>>>> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
>>>>>> + V4L2_CID_EXPOSURE_ABSOLUTE,
>>>>>> + 0, 65535, 1, 0);
>>>>>
>>>>> Is exposure_absolute supposed to be in microseconds...?
>>>>
>>>> Yes.
>>>
>>> According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
>>>
>>> OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
>>>> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
>>>
>>> Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
>>> taking 100 usec units, so unit-less is better.
>>
>> Thanks. If you know the units, it would be of course better to use
>> right units...
>
> Steve: what's the unit in this case? Is it lines or something else?
Yes, the register interface for exposure takes lines*16.
Maybe converting from seconds to lines is as simple as
framerate * height * seconds. But I'm not sure about that.
Steve
>
> Pavel: we do need to make sure the user space will be able to know the unit,
> too. It's rather a case with a number of controls: the unit is known but
> there's no API to convey it to the user.
>
> The exposure is a bit special, too: granularity matters a lot on small
> values. On most other controls it does not.
>
On Sat, Jun 03, 2017 at 09:46:36PM -0700, Steve Longerbeam wrote:
>
>
> On 06/03/2017 02:57 PM, Sakari Ailus wrote:
> >On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote:
> >>Hi!
> >>
> >>>>>>+ /* Auto/manual exposure */
> >>>>>>+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> >>>>>>+ V4L2_CID_EXPOSURE_AUTO,
> >>>>>>+ V4L2_EXPOSURE_MANUAL, 0,
> >>>>>>+ V4L2_EXPOSURE_AUTO);
> >>>>>>+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> >>>>>>+ V4L2_CID_EXPOSURE_ABSOLUTE,
> >>>>>>+ 0, 65535, 1, 0);
> >>>>>
> >>>>>Is exposure_absolute supposed to be in microseconds...?
> >>>>
> >>>>Yes.
> >>>
> >>>According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> >>>
> >>> OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> >>>>Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> >>>
> >>>Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> >>>taking 100 usec units, so unit-less is better.
> >>
> >>Thanks. If you know the units, it would be of course better to use
> >>right units...
> >
> >Steve: what's the unit in this case? Is it lines or something else?
>
> Yes, the register interface for exposure takes lines*16.
>
> Maybe converting from seconds to lines is as simple as
> framerate * height * seconds. But I'm not sure about that.
The smiapp and a few other drivers are using lines. One option could be to
use lines as the unit and have step as 16.
Then the hblank + vblank controls will be needed, too, to enable the user to
at least figure out the conversion to Si units.
--
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]