On 06/14/2017 06:29 PM, Dave Stevenson wrote:
Hi Hans.
On 14 June 2017 at 16:42, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
Hi Dave,
How does this driver relate to this staging driver:
drivers/staging/vc04_services/bcm2835-camera/
It's not obvious to me.
drivers/staging/vc04_services/bcm2835-camera/ is using the VideoCore
firmware to control Unicam, ISP, and all the tuner algorithms. The ARM
gets delivered fully processed buffers from the VideoCore side. The
firmware only has drivers for the Omnivision OV5647 and Sony IMX219
(and an unsupported one for the Toshiba TC358743).
This driver is solely the Unicam block, reading the data in over
CSI2/CCP2 from the sensor and writing it to memory. No ISP or control
loops.
Other than power management, this driver is running solely on the ARM
with no involvement from the VideoCore firmware.
The sensor driver is whatever suitable V4L2 subdevice driver you fancy
attaching (as long as it supports CSI2, or eventually CCP2).
What is the interaction between these two drivers? Can they co-exist?
I would expect them to be mutually exclusive.
On 06/14/2017 05:15 PM, Dave Stevenson wrote:
Hi All.
This is adding a V4L2 subdevice driver for the CSI2/CCP2 camera
receiver peripheral on BCM283x, as used on Raspberry Pi.
v4l2-compliance results depend on the sensor subdevice this is
connected to. It passes the basic tests cleanly with TC358743,
but objects with OV5647
fail: v4l2-test-controls.cpp(574): g_ext_ctrls does not support count == 0
Neither OV5647 nor Unicam support any controls.
Are you compiling v4l2-compliance from the v4l-utils git repo? If not,
then please do so and run again. The version packaged by distros tends
to be seriously outdated.
Yes, I'm building from v4l-utils.git.
I updated within the last week, although you appear to have added 2
commits since (both CEC related).
I'm on "ef074cf media-ctl: add colorimetry support"
But the line with that error is at line number 587 in my repo, not 574.
So I'm a bit suspicious.
Anyway, can you give the output of 'v4l2-ctl -l'?
I must admit to not having got OV5647 to stream with the current driver
register settings. It works with a set of register settings for VGA RAW10.
I also have a couple of patches pending for OV5647, but would like to
understand the issues better before sending them out.
Two queries I do have in V4L2-land:
- When s_dv_timings or s_std is called, is the format meant to
be updated automatically?
Yes. Exception is if the new timings/std is exactly the same as the old
timings/std, in that case you can just return 0 and do nothing.
OK, can do that.
Even if we're already streaming?
That's not allowed. Return -EBUSY in that case.
Also reasonable.
So if the TC358743 flags a source change we have to stop streaming,
set the new timings (which will update the format), and start up again
with fresh buffers. That's what I was expecting, but wanted to
confirm.
Correct. In theory there are ways around this provided the buffers are
large enough to accommodate the new format size, but nobody actually
supports that (might change in the not-to-distant future).
Some existing drivers seem to, but others don't.
- With s_fmt, is sizeimage settable by the application in the same
way as bytesperline?
No, the driver will fill in this field, overwriting anything the
application put there.
bytesperline IS settable, but most drivers will ignore what userspace
did and overwrite this as well.
Normally the driver knows about HW requirements and will set sizeimage
to something that will work (e.g. make sure it is a multiple of 16 lines).
There are subtly different requirements in different hardware blocks :-(
eg Unicam needs bytesperline to be a multiple of 16 bytes,whilst the
ISP requires a multiple of 32.
The vertical padding is generally where we're doing software
processing on the VideoCore side as it's easier to just leave the the
16 way SIMD processor running all 16 ways, hence needing scratch space
to avoid reading beyond buffers.
The main consumer is likely to be the ISP and that doesn't need
vertical context, so I'll look at removing the requirement there
rather than forcing it in this driver.
As long as we can set bytesperline (which is already supported) then
that requirement of the ISP is already handled.
It's up to you, but given the large sizes of the buffers the extra bit
of padding doesn't really matter all that much. And if it would make it
easier to digest by other HW blocks, then I would just add the padding.
Note that users can also call VIDIOC_CREATE_BUFS instead of VIDIOC_REQBUFS
in order to request buffers of a larger-than-needed size. I'm not sure if
you are aware of that.
yavta allows you to specify it on the command
line, whilst v4l2-ctl doesn't. Some of the other parts of the Pi
firmware have a requirement that the buffer is a multiple of 16 lines
high, which can be matched by V4L2 if we can over-allocate the
buffers by the app specifying sizeimage. But if I allow that,
then I get a v4l2-compliance failure as the size doesn't get
reset when switching from RGB3 to UYVY as it takes the request as
a request to over-allocate.
Apologies if I've messed up in sending these patches - so many ways
to do something.
It looks fine at a glance.
I will probably review this on Friday or Monday. But I need some
clarification
of the difference between this and the staging driver first.
Thanks. Hopefully I've given you that clarification above.
I'll fix the s_dv_timings and s_std handling, and s_fmt of sizeimage,
but will wait for other review comments before sending a v2.
OK. As said, I'll take a closer look on Friday or Monday.
Regards,
Hans
