On Tue, Jun 27, 2017 at 06:33:13PM +0900, Tomasz Figa wrote:
> On Mon, Jun 26, 2017 at 11:51 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> > On Mon, Jun 12, 2017 at 06:59:18PM +0900, Tomasz Figa wrote:
> >
> >>
> >> > + if (WARN_ON(freq <= 0))
> >> > + return -EINVAL;
> >>
> >> It generally doesn't make sense for the frequency to be negative, so
> >> maybe the argument should have been unsigned to start with? (And
> >> 32-bit if we don't expect frequencies higher than 4 GHz anyway.)
> >
> > The value comes from a 64-bit integer V4L2 control so that implies the value
> > range of s64 as well.
>
> Okay, if there is no way to enforce this at control level, then I
> guess we have to keep this here.
>
> >
> >>
> >> > +
> >> > + /* b could be 0, -2 or -8, so r < 500000000 */
> >>
> >> Definitely. Anything <= 0 is also less than 500000000. Let's take a
> >> look at the computation below again:
> >>
> >> 1) accinv is multiplied by b,
> >> 2) 500000000 is divided by 256 (=== shift right by 8 bits) = 1953125,
> >> 3) accinv*b is multiplied by 1953125 to form the value of r.
> >>
> >> Now let's see at possible maximum absolute values for particular steps:
> >> 1) 16 * -8 = -128 (signed 8 bits),
> >> 2) 1953125 (unsigned 21 bits),
> >> 3) -128 * 1953125 = -249999872 (signed 29 bits).
> >>
> >> So I think the important thing to note in the comment is:
> >>
> >> /* b could be 0, -2 or -8, so |accinv * b| is always less than (1 <<
> >> ds) and thus |r| < 500000000. */
> >>
> >> > + r = accinv * b * (500000000 >> ds);
> >>
> >> On the other hand, you lose some precision here. If you used s64
> >> instead and did the divide shift at the end ((accinv * b * 500000000)
> >> >> ds), for the example above you would get -250007629. (Depending on
> >> how big freq is, it might not matter, though.)
> >>
> >
> > The frequency is typically hundreds of mega-Hertz.
>
> I think it still would make sense to have the calculation a bit more precise.
Then the solution is to divide by the 64-bit number, i.e. do_div(). IMO
this shouldn't be a big deal either way: the result needs to be in a value
range and this is only done once when streaming is started.
>
> >
> >> Also nit: What is 500000000? We have local constants defined above, I
> >> think it could also make sense to do the same for this one. The
> >> compiler should do constant propagation and simplify respective
> >> calculations anyway.
> >
> > COUNT_ACC in the formula in the comment a few decalines above is in
> > nanoseconds. Performing the calculations in integer arithmetics results in
> > having 500000000 in the resulting formula.
> >
> > So this is actually a constant related to the hardware but it does not have
> > a pre-determined name because it is derived from COUNT_ACC.
>
> Which, I believe, doesn't stop us from naming it.
No, but the value is derived from another value and used once. There's not
much value in adding a macro for IMO.
The formula can be perhaps easier written as:
accinv * a + (accinv * b * (500000000 >> ds)
/ (int32_t)(link_freq >> ds));
If you insist, how about COUNT_ACC_FACTOR, for it's derived from COUNT_ACC?
>
> >> > +static int cio2_v4l2_querycap(struct file *file, void *fh,
> >> > + struct v4l2_capability *cap)
> >> > +{
> >> > + struct cio2_device *cio2 = video_drvdata(file);
> >> > +
> >> > + strlcpy(cap->driver, CIO2_NAME, sizeof(cap->driver));
> >> > + strlcpy(cap->card, CIO2_DEVICE_NAME, sizeof(cap->card));
> >> > + snprintf(cap->bus_info, sizeof(cap->bus_info),
> >> > + "PCI:%s", pci_name(cio2->pci_dev));
> >> > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> >>
> >> Hmm, I thought single plane queue type was deprecated these days and
> >> _MPLANE recommended for all new drivers. I'll defer this to other
> >> reviewers, though.
> >
> > If the device supports single plane formats only, I don't see a reason to
> > use MPLANE buffer types.
>
> On the other hand, if a further new revision of the hardware (or
> amendment of supported feature set of current hardware) actually adds
> support for multiple planes, changing it to MPLANE will require
> keeping a non-MPLANE variant of the code, due to userspace
> compatibility concerns...
I think I have to correct my earlier statement --- the device supports
multi-planar formats as well. They're only useful with SoC cameras though,
not with raw Bayer cameras.
IMO VB2/V4L2 could better support conversion between single and
multi-planar buffer types so that the applications could just use any and
drivers could manage with one.
I don't have a strong opinion either way, but IMO this could be well
addressed later on by improving the framework when (or if) the support for
formats such as NV12 is added.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
