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.
...and somehow I still recall (voice echoing in my head ;)) someone
saying (writing) that single plane ABI is deprecated and all new
drivers should be using MPLANE. Hans, was that you?
Best regards,
Tomasz
