Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Laurent, On Fri, Sep 28, 2018 at 03:49:38PM +0200, Laurent Pinchart wrote: > I don't think we'll reach an agreement here if we don't start talking about > real use cases. Would you have some to share ? Fair enough, but at that point, we very much disconnect from the imx208 in the subject. I'm working with a stereo camera. In that setup, you have two image sensors and infer a three dimensional structure from images captured at equal time points. For that to happen, it is important that the image sensors use the same settings. In particular, settings such as expoure and gain must match. That in turn means that you cannot use the automatic exposure mode that comes with your image sensor. This is one reason for implementing exposure control outside of the image sensor. Typically you can categorize your parameters into three classes that affect the brightness of an image: aperture, shutter speed and some kind of gain. If you know the units of these parameters, you can estimate the effect of changing them on the resulting image. The algorithm for controlling brightness can be quite generic if you know the units. V4L2_CID_EXPOSURE_ABSOLUTE is given in 100 µs. That tends to work well. Typically, you prefer increasing exposure over increasing gain to avoid noise. Similarly, you prefer increasing analogue gain over increasing digital gain. On the other hand, there is a limit on exposure to avoid motion blur. If an algorithm knows valid values for these parameters, it can produce settings independently of what concrete image sensor you use. > > I can see two solutions here: > > > > 1) Define the control range from 0 to 4 and treat it as an exponent > > of 2, so that the value for the sensor becomes (1 << val) * 256. > > (Suggested by Sakari offline.) > > > > This approach has the problem of losing the original unit (and scale) > > of the value. > > > > This approach is the one where users will need to know which sensor they > > talk to. The one where the hardware abstraction happens in userspace. > > Can we please not do that? > > Let's talk about it based on real use cases. So if you change your gain from 0 to 1, your image becomes roughly twice as bright. In the typical scenario that's too bright, so when increasing gain, you simultaneously decrease something else (e.g. exposure). But if you don't know the effect of your gain change, you cannot tell how much your exposure needs to be reduced. The only way out here is just doing it and reducing exposure afterwards. Users tend to not like the flickering resulting from this approach. > > * If it is non-linear and has fewer than X (1025?) values, use the > >integer menu. > > 1024 ioctl calls to query the menu values ? :-( We need a better API than > that. I'm also concerned that it wouldn't be very usable by userspace. Having > a list of supported values is one thing, making efficient use of it is > another. Again, use cases :-) You only need to query it once, but I'm not opposed to a better API either. I just don't think it is that important or urgent. > I expect many algorithms to need a mathematical view of the valid values, not > just a list. What particular algorithms do you have in mind ? A very simple algorithm could go like this: * Assume that exposure time and gain have a linear effect on the brightness of a captured image. This tends to not hold exactly, but close enough. * Compare brightness of the previous frame with a target value. * Compute the product of current exposure time and gain. Adapt the product according to the brightness error. * Distribute this product to exposure time and gain such that exposure time is maximal below a user-defined limit and that gain is one of the valid values. All you need to know for using this besides V4L2_CID_EXPOSURE_ABSOLUTE, is the valid gain values. Now I wonder, does this help in reaching a conclusion about whether querying valid gain values is a relevant use case? Helmut
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: > On 09/25/2018 12:14 PM, Sakari Ailus wrote: > > +/* V4L2 control unit prefixes */ > > +#define V4L2_CTRL_PREFIX_NANO -9 > > +#define V4L2_CTRL_PREFIX_MICRO -6 > > +#define V4L2_CTRL_PREFIX_MILLI -3 > > +#define V4L2_CTRL_PREFIX_1 0 > > I would prefer PREFIX_NONE, since there is no prefix in this case. > > I assume this prefix is only valid if the unit is not UNDEFINED and not > NONE? Why should it? The prefix is concerned with rescaling a value prior to presenting it to a user. Even a unitless quantity or a value of undefined unit can be reasonably scaled. Displaying a unit and scaling look like orthogonal concepts to me. > Is 'base' also dependent on a valid unit? (it doesn't appear to be) I'd argue it should not depend on a valid unit like the prefix. Helmut
Re: [PATCH 0/5] Add units to controls
On Tue, Sep 25, 2018 at 02:30:31PM +0200, Sakari Ailus wrote: > On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote: > > 1. There are a number of controls whose value is not easily described as > >either linear or exponential. I'm faced with at least two controls > >that actually are floating point numbers. One with two bits for the > >exponent and one (strange) bit for the mantissa (no joke) and another > >with three bits for the exponent and four bits for the mantissa. > >Neither can suitably be represented. > The proposal does not address all potential situations, that's true. > There's no way to try to represent everything out there (without > enumerating the values) in an easily generalised way but something can be > done. Sure, but the rounding control flag would address that. The rounding control flag could also solve exponential controls. Since there is a certain overlap here. These proposals should be discussed together. We should avoid solving problems twice. > There are devices such as some flash LED controllers where the flash current > if simply a value you can pick from the list. It's currently implemented as > an integer control. AFAIR the driver is drivers/leds/leds-aat1290.c . Possibly, relaxing the "each control id has a fixed type" requirement is another option. Allowing an integer menu wherever an integer is specified could solve issues such as these in a different way. Again it is fine that your proposal, does not cover that. Still these uapi changes are interdependent and therefore need to be considered together. > The fact is that the unit is specific to hardware. The documentation > documents something, and often suggests a unit, but if the hardware does > something else, then that's what you get --- independently of what the > documentation says. > > Hence the need to convey it via the API. That is vaguely convincing. It still seems like a niche case. > Some controls could have the unit set by the framework if that makes sense. I'd use a stronger s/could/should/ here as it directly translates into maintenance cost. > Most drivers shouldn't actually need to touch this if they're fine with > defaults (whenever a control has a default). Exactly this. Therefore I think you shouldn't update smiapp, but the framework instead. > A macro or two, it's not a major change. From the user space point of view, > does it make a difference if a control has no unit or when it's not known > what the unit is? There are situations where there is a difference. If you count things (e.g. reference pictures V4L2_CID_MPEG_VIDEO_MAX_REF_PIC), there is no unit, but that's different from unknown. Having unknown separate from no unit also helps with the transition period where controls lack units. Therefore, i'm in favour of keeping the distinction between unknown and no unit. Still, I don't think that merging them is fundamentally "wrong". > Yes, I think [V4L2_CTRL_FLAG_EXPONENTIAL] can be dropped as I suggested. Ok. Looking forward to the rounding flag then. > > Thus, I think that control over the rounding is tightly related to this > > patchset and needs to be discussed together. > > It addresses some of the same problem area but the implementation is > orthogonal to this. I don't disagree here. Still I think that the question "what should be implemented" (not how) is dependent on those other problems. > Providing that would probably make the base field less useful: the valid > control values could be enumerated by the user using TRY_EXT_CTRLS without > the need to tell the valid values are powers of e.g. two. After dropping V4L2_CTRL_FLAG_EXPONENTIAL, the base field is meaningless, no? > I don't really have a strong opinion on that actually when it comes to the > API itself. The imx208 driver could proceed to use linear relation between > the control value and the digital gain. My worry is just the driver > implementation: this may not be entirely trivial. There's still no way to > address this problem in a generic way otherwise. Yeah, I'm mostly looking at this from an uapi pov (as that is the one that cannot be changed later) and have no good answer here. Allowing integer menus for integers would be easy from a driver pov though. Helmut
Re: [PATCH 0/5] Add units to controls
On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote: > This set adds a few things to the current control framework in terms of > what kind of information the user space may have on controls. It adds > support for units and prefixes, exponential base as well as information on > whether a control is linear or exponential, to struct v4l2_query_ext_ctrl. That's a great improvement. Being able to give meaning to controls is great. However, I see two significant weaknesses in the approach being taken: 1. There are a number of controls whose value is not easily described as either linear or exponential. I'm faced with at least two controls that actually are floating point numbers. One with two bits for the exponent and one (strange) bit for the mantissa (no joke) and another with three bits for the exponent and four bits for the mantissa. Neither can suitably be represented. Since the value ranges are small for the cases I mentioned, I looked into using an integer menu. The present approach does not allow for replacing an integer with an integer menu though. Each control id has a fixed type. I'm not sure how to solve this. 2. The present implementation places the responsibility of assigning units and scales into drivers. A number of controls (e.g. V4L2_CID_EXPOSURE_ABSOLUTE, V4L2_CID_AUDIO_LIMITER_RELEASE_TIME, V4L2_CID_PIXEL_RATE, ...) clearly state the scale and unit in the documentation. Having each and every driver set units and scales in the documented way will lead to duplication and buggy code. Having each driver choose unit and scale will lead to inconsistency. It would be very good to have a mechanism that puts the framework in charge of maintaining units and scales for the standard control ids. What is a consumer supposed to do with a control that changes unit? The algorithm expected e.g. a duration. It might be able to convert to pixels, but what should it do if it gets back amperes? I argue that the unit should be a property of the control id and be documented (i.e. what is done now). If it is reported via an ioctl, the framework needs to be in charge of setting it. The story is much different for scales. Scaling the value up and down is something consumers can reasonably be expected to do. It allows shifting the value range such that the relevant values can be fully represented. Allowing drivers to change scales is much more reasonable. Still the framework should provide a default such that most drivers should not need any update. I acknowledge that these expectations are high and see that they're partially covered in your later remarks. > The smiapp driver gains support for the feature. In the near term, some > controls could also be assigned the unit automatically. The pixel rate, > for instance. Fewer driver changes would be needed this way. A driver > could override the value if there's a need to. I believe that in the interest of keeping maintenance cost low, this should happen rather sooner than later. Just even adding the support to smiapp seems wrong when it would be possible to have the framework do the work. > I think I'll merge the undefined and no unit cases. Same for the > exponential base actually --- the flag can be removed, too... I'm not sure I understand. It reads like you are going to revert a significant part of the patch. > Regarding Ricardo's suggestion --- I was thinking of adding a control flag > (yes, there are a few bits available) to tell how to round the value. The > user could use the TRY_EXT_CTRLS IOCTL to figure out the next (or > previous) control value by incrementing the current value and setting the > appropriate flag. This is out of the scope of this set though. This approach sounds really useful to me. Having control over the rounding would allow reading supported control values with reasonable effort. With such an approach, a very sparsely populated control becomes feasible and with integer64 controls that'd likely allow representing most exponential controls with linear values. If going this route, I don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL. Thus, I think that control over the rounding is tightly related to this patchset and needs to be discussed together. Helmut
Re: [PATCH 3/5] Documentation: media: Document control exponential bases, units, prefixes
On Tue, Sep 25, 2018 at 12:14:32PM +0200, Sakari Ailus wrote: > +* - ``V4L2_CTRL_UNIT_PIXEL`` > + - 5 > + - A pixel in sensor's pixel matrix. This is a unit of time commonly > used > +by camera sensors in e.g. exposure control, i.e. the time it takes > for > + a sensor to read a pixel from the sensor's pixel matrix. > + > +* - ``V4L2_CTRL_UNIT_PIXEL`` > + - 6 The latter V4L2_CTRL_UNIT_PIXEL looks like a typo that should say V4L2_CTRL_UNIT_PIXELS_PER_SEC. Helmut
Re: [PATCH 1/1] v4l: Remove support for crop default target in subdev drivers
On Mon, Sep 24, 2018 at 04:42:27PM +0200, Sakari Ailus wrote: > --- a/drivers/media/i2c/mt9t112.c > +++ b/drivers/media/i2c/mt9t112.c > @@ -888,12 +888,6 @@ static int mt9t112_get_selection(struct v4l2_subdev *sd, > sel->r.width = MAX_WIDTH; > sel->r.height = MAX_HEIGHT; > return 0; > - case V4L2_SEL_TGT_CROP_DEFAULT: > - sel->r.left = 0; > - sel->r.top = 0; > - sel->r.width = VGA_WIDTH; > - sel->r.height = VGA_HEIGHT; > - return 0; > case V4L2_SEL_TGT_CROP: > sel->r = priv->frame; > return 0; Together with the change in soc_scale_crop.c, this constitutes an (unintentional?) behaviour change. It was formerly reporting 640x480 and will now be reporting 2048x1536. I cannot tell whether that is reasonable. > --- a/drivers/media/i2c/soc_camera/mt9t112.c > +++ b/drivers/media/i2c/soc_camera/mt9t112.c > @@ -884,12 +884,6 @@ static int mt9t112_get_selection(struct v4l2_subdev *sd, > sel->r.width = MAX_WIDTH; > sel->r.height = MAX_HEIGHT; > return 0; > - case V4L2_SEL_TGT_CROP_DEFAULT: > - sel->r.left = 0; > - sel->r.top = 0; > - sel->r.width = VGA_WIDTH; > - sel->r.height = VGA_HEIGHT; > - return 0; > case V4L2_SEL_TGT_CROP: > sel->r = priv->frame; > return 0; This one looks duplicate. Is there a good reason to have two drivers for mt9t112? This is lilely out of scope for the patch. Cced Jacopo Mondi as he introduced the copy. Other than your patch looks fine to me. Helmut
Use of V4L2_SEL_TGT_CROP_DEFAULT in i2c subdev drivers
Hi, Documentation/media/uapi/v4l/v4l2-selection-targets.rst says that V4L2_SEL_TGT_CROP_DEFAULT is not valid for subdev drivers. Looking into drivers/media/i2c (which contains only subdev drivers except for video-i2c.c), the following drivers implement V4L2_SEL_TGT_CROP_DEFAULT: ak881x.c mt9m111.c mt9t112.c ov2640.c ov6650.c ov772x.c rj54n1cb0c.c soc_camera/mt9m001.c soc_camera/mt9t112.c soc_camera/mt9v022.c soc_camera/ov5642.c soc_camera/ov772x.c soc_camera/ov9640.c soc_camera/ov9740.c soc_camera/rj54n1cb0c.c tvp5150.c The majority of drivers behave equally for V4L2_SEL_TGT_CROP_DEFAULT and V4L2_SEL_TGT_CROP_BOUNDS. The only exceptions are mt9t112.c and soc_camera/mt9t112.c. Actually these two look very similar. A significant fraction of differences is white space, case and operand order. Is this a bug in 16 drivers? Is this a documentation bug? Am I getting something wrong? Helmut
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
On Thu, Sep 20, 2018 at 11:00:26PM +0200, Laurent Pinchart wrote:
> On Thursday, 20 September 2018 23:16:47 EEST Sylwester Nawrocki wrote:
> > On 09/20/2018 06:49 PM, Grant Grundler wrote:
> > > On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
> > >> We have a problem here. The sensor supports only a discrete range of
> > >> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> > >> fixed point). This makes it possible for the userspace to set values
> > >> that are not allowed by the sensor specification and also leaves no
> > >> way to enumerate the supported values.
I'm not sure I understand this correctly. Tomasz appears to imply here
that the number of actually supported gain values is 5.
> > I'm not sure if it's best approach but I once did something similar for
> > the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
> > for fractional part. The driver reports values multiplied by 16. See
> > ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
> > Total Gain to Control Bit Correlation" in the OV9650 datasheet for details.
> > The integer menu control just seemed not suitable for 2^10 values.
As long as values scale linearly (as is the case for fixed point
numbers) that seems like a reasonable approach to me. That assumption
appears to not hold for imx208 where the values scale exponentially.
> I've had a similar discussion on IRC recently with Helmut, who posted a nice
> summary of the problem on the mailing list (see https://www.mail-archive.com/
> linux-media@vger.kernel.org/msg134502.html). This is a known issue, and while
> I proposed the same approach, I understand that in some cases userspace may
> need to know exactly what values are acceptable. In such a case, however, I
> would expect userspace to have knowledge of the particular sensor model, so
> the information may not need to come from the kernel.
A big reason to use V4L2 is to abstract hardware. Having to know what
sensor model you use runs counter that goal. Indeed, gain (whether
digital or analogue) can be abstracted in principle. I see little reason
to not do that.
> > Now the gain control has range 16...1984 out of which only 1024 values
> > are valid. It might not be best approach for a GUI but at least the driver
> > exposes mapping of all valid values, which could be enumerated with
> > VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific
> > user space code.
If you have very many values, a reasonable compromise could be reducing
the precision. If you try to represent a floating point number, values
with higher exponent will have larger "gaps" when represented as
integers for v4l2. A compromise could be increasing the step size and
thus removing a few of the gains with lower exponent.
> > >> I can see two solutions here:
> > >>
> > >> 1) Define the control range from 0 to 4 and treat it as an exponent of
> > >> 2, so that the value for the sensor becomes (1 << val) * 256.
> > >> (Suggested by Sakari offline.)
> > >>
> > >> This approach has the problem of losing the original unit (and scale)
> > >> of the value.
This approach is the one where users will need to know which sensor they
talk to. The one where the hardware abstraction happens in userspace.
Can we please not do that?
> > >> 2) Use an integer menu control, which reports only the supported
> > >> discrete values - {1, 2, 4, 8, 16}.
That's what I ended up doing, though I kinda deferred the problem as I
started using V4L2_CID_ISO_SENSITIVITY, which is an integer menu but not
exactly the right one. My choice will backfire once I submit the driver
though that'll still take a little while.
> > >> With this approach, userspace can enumerate the real gain values, but
> > >> we would either need to introduce a new control (e.g.
> > >> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> > >> register V4L2_CID_DIGITAL_GAIN as an integer menu.
> > >>
> > >> Any opinions or better ideas?
Abusing the specification sounds like it would break userspace. I'd
avoid doing that.
We do have a history of adding "duplicate" CIDs for gaining a better
specification. For instance, there is V4L2_CID_EXPOSURE (unspecified)
and then there came V4L2_CID_EXPOSURE_ABSOLUTE (unit 100 µs).
V4L2_CID_GAIN got split into V4L2_CID_ANALOGUE_GAIN and
V4L2_CID_DIGITAL_GAIN. Further splitting that into integer menu variants
of analogue and digital gain seems reasonable to me.
If doing so, I suggest using the following rules (up to discussion):
* Drivers must not provide both the integer menu and an integer control
for a single gain.
* Define which value means "no amplification". For
V4L2_CID_DIGITAL_GAIN this was defined as 0x100. Keeping that could
be reasonable, but V4L2_CID_ANALOGUE_GAIN presently leavs that
undefined.
* If a gain is linear, use the integer control.
* If it is non-linear and has fewer than X (1025?) values, use the
integer menu.
* Otherwise use the integer
Re: V4L2 analogue gain contol
Hi,
On Sun, Aug 26, 2018 at 08:52:09AM +0200, Pavel Machek wrote:
> > Can we give more structure to the analogue gain as exposed by V4L2?
> > Ideally, I'd like to query a driver for the possible gain values if
> > there are few (say < 256) and their factors (which are often given in
> > data sheets). The nature of gains though is that they are often similar
> > to floating point numbers (2 ** exp * (1 + mant / precision)), which
> > makes it difficult to represent them using min/max/step/default.
>
> Yes, it would be nice to have uniform controls for that. And it would
> be good if mapping to "ISO" sensitivity from digital photography existed.
Thank you very much for this pointer.
There is V4L2_CID_ISO_SENSITIVITY. It is an integer menu, which means
that I can introspect the available values. It is already used by
s5c73m3 and m5mols. That looks mostly like what I need. It makes no
provision on how the image is amplified, whether digital or analogue.
I'd need analogue gain only here.
Reading the platform/exynos4-is/fimc and the i2c/s5c73m3 driver, I get
the impression that the scaling is not in accordance with
Documentation/media/uapi/v4l/extended-controls.rst ("standard ISO values
multiplied by 1000") though. -> Adding the maintainers/supporters to Cc.
> > Would it be reasonable to add a new V4L2_CID_ANALOGUE_GAIN_MENU that
> > claims linearity and uses fixed-point numbers like
> > V4L2_CID_DIGITAL_GAIN? There already is the integer menu
> > V4L2_CID_AUTO_EXPOSURE_BIAS, but it also affects the exposure.
>
> I'm not sure if linear scale is really appropriate. You can expect
> camera to do ISO100 or ISO200, but if your camera supports ISO48,
> you don't really expect it to support ISO480100.
I may have been ambigue here. With "linear" I was not trying to imply
that cameras should support every possible value and maybe "linear" is
not the property I actually need.
What I need is a correspondence between gain value (the value you pass
to V4L2_CID_ANALOGUE_GAIN) and amplification factor (brightness increase
of the resulting image). A linear connection is the simplest of course,
but logarithmic works as well in principle.
My idea of using an integer menu here was that a significant number of
cameras have a low count of valid gain settings. For them, listing all
valid values may be a legitimate option. Indeed, that's what happened
for V4L2_CID_ISO_SENSITIVITY.
> ./drivers/media/i2c/et8ek8/et8ek8_driver.c already does that.
>
> IOW logarithmic scale would be more appropriate; min/max would be
> nice, and step
I'm sorry for missing this driver in the analysis. It certainly adds to
the picture.
Note however that simply logarithmic with a step will not be a
one-size-fits-all. Fixed point numbers do not map to a logarithmic scale
with fixed steps. You can achieve fewer "holes" in your representation,
but you won't get rid of them entirely.
In the majority of cases, you could represent the gain as a product of a
logarithmic and a linear scale each with fixed steps. Is that an option?
> > An important application is implementing a custom gain control when the
> > built-in auto exposure is not applicable.
>
> Looking at et8ek8 again, perhaps that's the right solution? Userland
> just sets the gain, and the driver automatically selects best
> analog/digital gain combination.
>
> /*
> * This table describes what should be written to the sensor register
> * for each gain value. The gain(index in the table) is in terms of
>* 0.1EV, i.e. 10 indexes in the table give 2 time more gain [0] in
> * the *analog gain, [1] in the digital gain
> *
> * Analog gain [dB] = 20*log10(regvalue/32); 0x20..0x100
>*/
That may work (even for just analogue gain), but it comes at a little
loss of flexibility. You stop exposing a number of gain values and
combinations. In some cases, you loose more than half of the valid
configurations.
Striking a balance between a simple and a flexible interface of course
is difficult. I'm not opposed to providing such a simple interface, but
I'd also like to retain the flexibility (with another and likely more
complex interface).
Given your reply, I see three significant alternatives to my proposal:
* V4L2_CID_ISO_SENSITIVITY (even though it may use digital gain)
* V4L2_CID_ANALOGUE_GAIN_ISO could be an integer menu control modeled
after V4L2_CID_ISO_SENSITIVITY.
* V4L2_CID_ANALOGUE_GAIN_LOG x + V4L2_CID_ANALOGUE_GAIN_LINEAR y such
that the actual gain amplification value is 2 ** x * y (where x and y
are each fixed point numbers with a to-be-determined fixed point).
I guess I'll try to work with V4L2_CID_ISO_SENSITIVITY.
Helmut
Re: [PATCH] media: aptina-pll: allow approximating the requested pix_clock
On Sat, Aug 25, 2018 at 01:32:47PM +0200, Sakari Ailus wrote:
> On Fri, Aug 24, 2018 at 02:05:17PM +0200, Helmut Grohne wrote:
> > Take for instance MT9M024. The data sheet
> > (http://www.mouser.com/ds/2/308/MT9M024-D-606228.pdf) allows deducing
> > the following limits:
> >
> > const struct aptina_pll_limits mt9m024_limits = {
> > .ext_clock_min = 600,
> > .ext_clock_max = 5000,
> > .int_clock_min = 200,
> > .int_clock_max = 2400,
> > .out_clock_min = 38400,
> > .out_clock_max = 76800,
> > .pix_clock_max = 7425,
> > .n_min = 1,
> > .n_max = 63,
> > .m_min = 32,
> > .m_max = 255,
> > .p1_min = 4,
> > .p1_max = 16,
> > };
> >
> > Now if you choose ext_clock and pix_clock maximal within the given
> > limits, the existing aptina_pll_calculate gives up. Lowering the
> > pix_clock does not help either. Even down to 73 MHz, it is unable to
> > find any pll configuration.
> >
> > The new algorithm finds a solution (n=11, m=98, p1=6) with 7.5 KHz
> > error. Incidentally, that solution is close to the one given by the
> > vendor tool (n=22, m=196, p1=6).
>
> These values don't seem valid for 6 MHz --- the frequency after the PLL is
> less than 384 MHz. Did you use a different external clock frequency?
I wrote that I used the maximal external clock frequency, which is 50
MHz. For that value, the output clock is within the requested bounds.
Are you implying that the chosen pll parameters should be valid for all
possible external clocks simultaneously?
Helmut
Re: [PATCH] media: aptina-pll: allow approximating the requested pix_clock
Hi Laurent,
Thank you for taking the time to reply to my patch and to my earlier
questions.
On Thu, Aug 23, 2018 at 01:12:15PM +0200, Laurent Pinchart wrote:
> Could you please share numbers, ideally when run in kernel space ?
Can you explain the benefits of profiling this inside the kernel rather
than outside? Doing it outside is much simpler, so I'd like to
understand your reasons. The next question is what kind of system to
profile on. I guess doing it on an X86 will not help. Typical users will
use some arm CPU and integer division on arm is slower than on x86. Is a
Cortex A9 ok?
If you can provide more relevant inputs, that'd also help. I was able to
derive an example input from the dt bindings for mt9p031 (ext=6MHz,
pix=96MHz) and also used random inputs for testing. Getting more
real-world inputs would help in producing a useful benchmark.
> This patch is very hard to review as you rewrite the whole, removing all the
> documentation for the existing algorithm, without documenting the new one.
That's not entirely fair. Unlike the original algorithm, I've split it
to multiple functions and unlike the original algorithm, I've documented
pre- and post-conditions. In a sense, that's actually more
documentation. At least, you now see what the functions are supposed to
be doing.
Inside the alogrithm, I've often writting which precondition
(in)equation I used in which particular step.
The variable naming choice makes it very clear that the first step is
reducing the value ranges for n, m, and p1 as much as possible before
proceeding to an actual parameter computation.
There was little choice in removing much of the old algorithm as the
approach of using gcd is numerically unstable. I actually kept
everything until the first gcd computation.
> There's also no example of a failing case with the existing code that works
> with yours.
That is an unfortunate omission. I should have thought of this and
should have given it upfront. I'm sorry.
Take for instance MT9M024. The data sheet
(http://www.mouser.com/ds/2/308/MT9M024-D-606228.pdf) allows deducing
the following limits:
const struct aptina_pll_limits mt9m024_limits = {
.ext_clock_min = 600,
.ext_clock_max = 5000,
.int_clock_min = 200,
.int_clock_max = 2400,
.out_clock_min = 38400,
.out_clock_max = 76800,
.pix_clock_max = 7425,
.n_min = 1,
.n_max = 63,
.m_min = 32,
.m_max = 255,
.p1_min = 4,
.p1_max = 16,
};
Now if you choose ext_clock and pix_clock maximal within the given
limits, the existing aptina_pll_calculate gives up. Lowering the
pix_clock does not help either. Even down to 73 MHz, it is unable to
find any pll configuration.
The new algorithm finds a solution (n=11, m=98, p1=6) with 7.5 KHz
error. Incidentally, that solution is close to the one given by the
vendor tool (n=22, m=196, p1=6).
> Could you please document the algorithm in details (especially explaining the
> background ideas), and share at least one use case, with with numbers for all
> the input and output parameters ?
I'll try to improve the documentation in the next version. Summarizing
the idea is something I can do, but beyond that I don't see much to add
beyond prose.
Ahead of posting a V2, let me suggest this:
/* The first part of the algorithm reduces the given aptina_pll_limits
* for n, m and p1 using the (in)equalities and the concrete values for
* ext_clock and pix_clock in order to reduce the search space.
*
* The main loop iterates over all remaining possible p1 values and
* computes the necessary out_clock frequency. The ext_clock / out_clock
* ratio is then approximated with n / m within their respective bounds.
* For each parameter choice, the preconditions must be rechecked,
* because integer rounding errors may result in violating some of the
* preconditions. The parameter set with the least frequency error is
* returned.
*/
Is this what you are looking for?
Helmut
Re: [PATCH] media: aptina-pll: allow approximating the requested pix_clock
Hi Sakari, Thank you for taking the time to look into the issue. On Thu, Aug 23, 2018 at 01:30:12PM +0200, Sakari Ailus wrote: > Knowing the formula, the limits as well as the external clock frequency, it > should be relatively straightforward to come up with a functional pixel > clock value. Was there a practical problem in coming up with such a value? I've added a concrete example to my other reply and should have done that with posting the initial question. I'm sorry for not having done it earlier. > You can't pick a value at random; it needs to be one that actually works. > The purpose of having an exact frequency is that the typical systems where > these devices are being used, as I explained earlier, is that having a > random frequency, albeit with which the sensor could possibly work, the > other devices in the system may not do so. We already refuted the concept of an "exact frequency". In nature, it simply isn't exact. Every board will have a slightly different frequency no matter how precise you calculate it. I'm not after random frequencies in any case. The goal of course is to approximate the requested frequency as good as possible. In particular, when the requested pix_clock allows using a parameter set that attains the frequency exactly, that parameter set will be emitted rather than some other approximation. Only for parameters where the old algorithm returns -EINVAL there should be an observable difference. Using an exact frequency is difficult here. How am I supposed to write e.g. 74242424.24242424... Hz into the device tree? Helmut
[PATCH] media: aptina-pll: allow approximating the requested pix_clock
Clock frequencies are not exact values, but rather imprecise, physical
properties. The present pll computation however, treats them as exact.
It tries to compute parameters that attain the requested pix_clock
exactly. Failing that, it gives up.
The new implementation approximates the requested pix_clock and reports
the actual value resulting from the computed parameters. If the
requested pix_clock can be attained, the original behaviour of
maximizing p1 is retained.
Experiments with the algorithm in userspace on an arm device show that
the computational cost is negligible compared to executing a binary for
all practical inputs.
Signed-off-by: Helmut Grohne
---
drivers/media/i2c/aptina-pll.c | 263 -
drivers/media/i2c/mt9m032.c| 15 ++-
2 files changed, 165 insertions(+), 113 deletions(-)
I tried using aptina_pll_calculate in a driver for a similar image sensor.
Using it, I was unable to find a pix_clock value such that the computation was
successful. Most of the practical parameters result in a non-integer pix_clock,
something that struct pll cannot represent.
The driver is not ready for submission at this point, but the changes to
aptina_pll_calculate are reasonable on their own, which is why I submit them
separately now.
Helmut
diff --git a/drivers/media/i2c/aptina-pll.c b/drivers/media/i2c/aptina-pll.c
index 224ae4e4cf8b..249018772b2b 100644
--- a/drivers/media/i2c/aptina-pll.c
+++ b/drivers/media/i2c/aptina-pll.c
@@ -2,6 +2,7 @@
* Aptina Sensor PLL Configuration
*
* Copyright (C) 2012 Laurent Pinchart
+ * Copyright (C) 2018 Intenta GmbH
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -14,23 +15,84 @@
*/
#include
-#include
#include
-#include
+#include
#include
#include "aptina-pll.h"
+/* A variant of mult_frac that works even when (x % denom) * numer produces a
+ * 32bit overflow.
+ */
+#define mult_frac_exact(x, numer, denom) \
+ ((u32)div_u64(mul_u32_u32((x), (numer)), (denom)))
+
+static unsigned int absdiff(unsigned int x, unsigned int y)
+{
+ return x > y ? x - y : y - x;
+}
+
+/* Find n_min <= *np <= n_max and d_min <= *dp <= d_max such that *np / *dp
+ * approximates n_target / d_target.
+ */
+static int approximate_fraction(unsigned int n_min, unsigned int n_max,
+ unsigned int d_min, unsigned int d_max,
+ unsigned int n_target, unsigned int d_target,
+ unsigned int *np, unsigned int *dp)
+{
+ unsigned int d, best_err = UINT_MAX;
+
+ d_min = max(d_min, mult_frac_exact(n_min, d_target, n_target));
+ d_max = min(d_max, mult_frac_exact(n_max, d_target, n_target) + 1);
+ if (d_min > d_max)
+ return -EINVAL;
+
+ for (d = d_min; d < d_max; ++d) {
+ unsigned int n = mult_frac_exact(d, n_target, d_target);
+
+ if (n >= n_min) {
+ unsigned int err = absdiff(
+ n_target, mult_frac_exact(n, d_target, d));
+
+ if (err < best_err) {
+ best_err = err;
+ *np = n;
+ *dp = d;
+ }
+ if (err == 0)
+ return 0;
+ }
+ ++n;
+ if (n <= n_max) {
+ unsigned int err = absdiff(
+ n_target, mult_frac_exact(n, d_target, d));
+
+ if (err < best_err) {
+ best_err = err;
+ *np = n;
+ *dp = d;
+ }
+ }
+ }
+ return best_err == UINT_MAX ? -EINVAL : 0;
+}
+
+/* Find parameters n, m, p1 such that:
+ * n_min <= n <= n_max
+ * m_min <= m <= m_max
+ * p1_min <= p1 <= p1_max, even
+ * int_clock_min <= ext_clock / n <= int_clock_max
+ * out_clock_min <= ext_clock / n * m <= out_clock_max
+ * pix_clock = ext_clock / n * m / p1 (approximately)
+ * p1 is maximized
+ * Report the achieved pix_clock (input/output parameter).
+ */
int aptina_pll_calculate(struct device *dev,
const struct aptina_pll_limits *limits,
struct aptina_pll *pll)
{
- unsigned int mf_min;
- unsigned int mf_max;
- unsigned int p1_min;
- unsigned int p1_max;
- unsigned int p1;
- unsigned int div;
+ unsigned int n_min, n_max, m_min, m_max, p1_min, p1_max, p1;
+ unsigned int clock_err = UINT_MAX;
dev_dbg(dev, "PLL: ext clock %u pix clock %u\n",
pll->ext_clock, pll->pix_clock);
@@ -46,120 +108,103 @@ int aptina_pll_cal
V4L2 analogue gain contol
Hi, I've been looking at various image sensor drivers to see how they expose gains, in particular analogue ones. What I found in 4.18 looks like a mess to me. In particular, my interest is about separation of analogue vs digital gain and an understanding of what effect a change in gain has on the brightness of an image. The latter is characterized in the following table in the "linear" column. driver | CID | register name | min | max | def | linear | comments +-+-+-+--+-++- adv7343 | G | ADV7343_DAC2_OUTPUT_LEVEL | -64 | 64 | 0 || adv7393 | G | ADV7393_DAC123_OUTPUT_LEVEL | -64 | 64 | 0 || imx258 | A | IMX258_REG_ANALOG_GAIN | 0 | 8191 | 0 || imx274 | G | multiple| | | | yes| [1] mt9m032 | G | MT9M032_GAIN_ALL| 0 | 127 | 64 | no | [2] mt9m111 | G | GLOBAL_GAIN | 0 | 252 | 32 | no | [3] mt9p031 | G | MT9P031_GLOBAL_GAIN | 8 | 1024 | 8 | no | [4] mt9v011 | G | multiple| 0 | 4063 | 32 || mt9v032 | G | MT9V032_ANALOG_GAIN | 16 | 64 | 16 | no | [5] ov13858 | A | OV13858_REG_ANALOG_GAIN | 0 | 8191 | 128 || ov2685 | A | OV2685_REG_GAIN | 0 | 2047 | 54 || ov5640 | G | OV5640_REG_AEC_PK_REAL_GAIN | 0 | 1023 | 0 || ov5670 | A | OV5670_REG_ANALOG_GAIN | 0 | 8191 | 128 || ov5695 | A | OV5695_REG_ANALOG_GAIN | 16 | 248 | 248 || mt9m001 | G | MT9M001_GLOBAL_GAIN | 0 | 127 | 64 | no | mt9v022 | G | MT9V022_ANALOG_GAIN | 0 | 127 | 64 || CID: A -> V4L2_CID_ANALOGUE_GAIN G -> V4L2_CID_GAIN, no V4L2_CID_ANALOGUE_GAIN present step: always 1 comments: [1] controls a product of analogue and digital gain, value scales roughly linear [2] code comments contradict data sheet [3] it is not clear whether it also controls a digital gain. [4] controls a combination of analogue and digital gain [5] analogue only The documentation (extended-controls.rst) says that the digital gain is supposed to be a linear fixed-point number with 0x100 meaning factor 1. The situation for analogue is much less precise. Typically, the number of analogue gains is much smaller than the number of digital gains. No driver exposes more than 13 bit for the analogue gain and half of them use at most 8 bits. Can we give more structure to the analogue gain as exposed by V4L2? Ideally, I'd like to query a driver for the possible gain values if there are few (say < 256) and their factors (which are often given in data sheets). The nature of gains though is that they are often similar to floating point numbers (2 ** exp * (1 + mant / precision)), which makes it difficult to represent them using min/max/step/default. Would it be reasonable to add a new V4L2_CID_ANALOGUE_GAIN_MENU that claims linearity and uses fixed-point numbers like V4L2_CID_DIGITAL_GAIN? There already is the integer menu V4L2_CID_AUTO_EXPOSURE_BIAS, but it also affects the exposure. An important application is implementing a custom gain control when the built-in auto exposure is not applicable. Helmut
Re: why does aptina_pll_calculate insist on exact division?
Hi,
Thank you for the quick and helpful answer.
On Tue, Aug 14, 2018 at 09:30:14AM +0200, Laurent Pinchart wrote:
> How do you mean ? The only place where pix_clock_max is used is in the
> following code:
>
> if (pll->pix_clock == 0 || pll->pix_clock > limits->pix_clock_max) {
> dev_err(dev, "pll: invalid pixel clock frequency.\n");
> return -EINVAL;
> }
>
> aptina_pll_calculate() rejects a request pixel clock value higher than the
> pixel clock frequency higher limit, which is also given by the caller. That
> shouldn't happen, it would be a bug in the caller.
Of course, I am trying values lower than pix_clock_max. For a number of
imagers, that pix_clock_max is 74.25 MHz. It seems that any pix_clock of
at least 72 MHz is rejected here.
> I'm not sure what you mean by avoiding fractional numbers. Could you please
> elaborate ? Please keep in mind that I last touched the code 6 years, so my
> memory isn't exactly fresh.
The first thing the code does is computing the gcd of pix_clock and
ext_clock. Immediately, it conludes that m must be a multiple of
pix_clock / gcd(pix_clock, ext_clock). Varying either clock value
slightly causes large jumps in the computed gcd value (in particular, it
will be 1 whenever either clock happens to be a prime number).
> If you mean using floating point operations to calculate PLL parameters,
> remember that the code runs in the Linux kernel, where floating point isn't
> allowed. You would thus have to implement the algorithm using fixed-point
> calculation.
I'm not after using floating points. In a sense, we are already
fixed-point calculation and the precision is 1 Hz. Rounding errors in
that range look ok to me.
> There's no such thing as an exact frequency anyway, as it's a physical value.
> I'd got for 50 MHz for simplicity.
That's exactly my point. The exact value should not matter. However, for
the present aptina_pll_calculate, the exact value matters a lot. Were
you to use 4991 Hz as ext_clock (which happens to be prime, but
reasonably close), aptina_pll_calculate fails entirely as m is deemed to
be a multiple of the pix_clock in Hz. No imager allows for such large m
values and thus aptina_pll_calculate rejects any such configuration with
-EINVAL.
I'm arguing that rather than failing to compute pll parameters, it
should compromise on exactness. Presently, aptina_pll_calculate ensures
that whenever it is successful, the assertion pix_clock = ext_clock / n
* m / p1 holds exactly and all intermediate values are whole numbers.
I'm arguing that having it hold exactly reduces utility of
aptina_pll_calculate, because frequencies are not exact in the real
world. There is no need to have whole numbered frequencies.
> aptina_pll_calculate() also approximates the requested frequency, but as it
> appears from your test, fails in some cases. That's certainly an issue in the
> code and should be fixed. Feel free to convince the manufacturer to release
> their PLL parameters computation code under the GPL ;-)
We both know that the exercise of extracting code from manufacturers is
futile.
However you appear to imply that aptina_pll_calculate should approximate
the requested frequency. That's not what it does today. That's a useful
answer to me already and I'll be looking into doing the work of coming
up with an alternative lifting the requirement.
> Again, please elaborate on what you mean by avoiding fractional numbers. I
> would certainly be open to a different algorithm (or possibly fixes to the
> existing code), as long as it fulfills the requirements behind the current
> implementation. In particular the code should compute the optimum PLL
> parameters when multiple options are possible, and its complexity should be
> lower than O(n^2) (ideally O(1), if not possible O(n)).
Beware though that discussing complexities requires more precision as to
what "n" means here. The code interprets it as n = p1_max - p1_min (not
accounting for the gcd computation), which is not the usual
interpretation. What you really want is that it completes in a
reasonable amount of time on slow, embedded devices for any input.
Once you lift the exactness requirement, you optimize multiple aspects
simultaneously. The present code maximizes P1, but we also want to
minimize the difference between the requested pix_clock and the
resulting pix_clock. There has to be some kind of trade-off here. The
trade-off chosen by the present code is to always have that difference
be 0. Once non-zero differences are allowed, optimum is no longer
well-defined.
So could you go into more detail as to what "optimum PLL parameters"
mean to you?
Helmut
why does aptina_pll_calculate insist on exact division?
Hi, I tried using the aptina_pll_calculate for a "new" imager and ran into problems. After filling out aptina_pll_limits from the data sheet, I was having a hard time finding a valid pix_clock. Most of the ones I tried are rejected by aptina_pll_calculate for various reasons. In particular, no pix_clock close to pix_clock_max is allowed. Why does the calculation method insist on exact division and avoiding fractional numbers? I'm using an ext_clock of 50 MHz. This clock is derived from a 33 MHz clock and the 50 MHz is not attained exactly. Rather it ends up being more like 49.76 Hz. This raises the question, what value I should put into ext_clock (or the corresponding device tree property). Should I use the requested frequency or the actual frequency? Worse, depending on the precise value of the ext_clock, aptina_pll_calculate may or may not be able to compute pll parameters. On the other hand, the manufacturer provided configuration tool happily computes pll parameters that result in close, but not exactly, the requested pix_clock. In particular, the pll parameters do not necessarily result in a whole number. It appears to merely approximate the requested frequency. Can you explain where the requirement to avoid fractional numbers comes from? Would it be reasonable to use a different algorithm that avoids this requirement? Helmut
V4L2_CID_USER_MAX217X_BASE == V4L2_CID_USER_IMX_BASE
Hi,
I found it strange that the macros V4L2_CID_USER_MAX217X_BASE and
V4L2_CID_USER_IMX_BASE have equal value even though each of them state
that they reserved a range. Those reservations look conflicting to me.
The macro V4L2_CID_USER_MAX217X_BASE came first, and
V4L2_CID_USER_IMX_BASE was introduced in e130291212df ("media: Add i.MX
media core driver") with the conflicting assignment (not a merge error).
The authors of that patch mostly make up the recipient list.
Is such a conflict fixable at all given that it resides in a uapi
header?
Helmut
