On Wed, Jun 28, 2017 at 10:36:54PM +0900, Tomasz Figa wrote:
> On Wed, Jun 28, 2017 at 10:31 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> > 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.
>
> The problem is that it couldn't, because it would change the userspace ABI.
Not if the driver supports single plane formats using both single-planar
and multi-planar API. I don't think there's much missing from that. It'd
make sense to do that anyway, independently of this driver.
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
