Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unicam driver #2513

Merged
merged 17 commits into from Jun 29, 2018

Conversation

Projects
None yet
2 participants
@6by9
Copy link
Contributor

commented Apr 16, 2018

Gordon had asked me to get this merged into our tree.
I know that several of these files are upstream, and I will be merging stuff upstream as I have the energy. Are there any that you want to enforce that they're upstreamed first, the patches modified until they are upstream, or currently dropped?

"tc358743: fix connected/active CSI-2 lane reporting" was submitted upstream, but has sat unloved since September - https://patchwork.linuxtv.org/patch/44397/. I'll nudge them when I'm upstreaming the other stuff.

My testing had shown that the normal dtparam=i2c_vc=on command worked, and i2c0 then appeared on GPIO 0&1 (or appropriate), but I will double check it as it was a while ago.

@pelwell

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

I've not been through the whole PR but I have a few early comments:

  1. My overlaycheck script is complaining that i2c0_pins isn't defined (it's referenced by the i2c0-bcm2708 script). How do you square using the i2c muxer with users who are used to also having i2c0 to themselves (usually on Compute Modules) with the ability to choose the pin usage?
  2. Early Pis (Rev 1 Model B and Model A) had the I2C buses swapped (see CAMERA_0_I2C_PORT in dt-blob.dts). This means that the mux would have to be on i2c1 for those Pis - how are you going to do that?
@pelwell

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

To partly answer 1 myself, could you leave i2c0_pins in place and use it instead of i2c0_gpio in the muxer?

@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

  1. It's a tricky one. i2c0-bcm2708 probably needs deprecating for normal variants - the only time it really got used was when you wanted to talk to the display or camera from Linux, and that now isn't necessary if you go via the mux. CM would potentially make sense to leave the mux out to avoid it claiming pins that they may not want.
  2. That also came up when I had tried getting this upstream. The intent was that the mux wasn't defined for the early Pi variants. I need to remind myself whether we have that differentiation available in kernel DT between rev1 and rev2 B.
@pelwell

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

  1. What about my suggestion?
  2. Upstream has dedicated rev1 and rev2 dtbs, we adjust at boot time by defining aliases accordingly.
@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

  1. Yes we could leave i2c0_pins there. (It's a shame you can't set an alias to point at an alias seeing as upstream have defined all the GPIO combinations. Duplication is likely to cause confusion.)
    Do we still have a problem if people do use i2c0-bcm2708 on a Pi 0/B+/2/3 where they will potentially remap the mux port 0 to be the same as port 1. The mux won't load as the second port won't be able to claim the GPIO pins. Do we rework i2c0-bcm2708 to disable the mux and revert to direct access?
  2. OK, that's a pain. I think the only solution is to drop the mux for both then, and not support camera/display access on them without custom DT changes. I can live with that.
@pelwell

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

  1. Yes - I thought the overlay could disable the mux in any way you choose.
  2. I'm not averse to a splitting bcm2708-rpi-b.dts - we are trying to move closer to upstream, and the firmware has a shiny new way of specifying fallback DTB files it would be nice to make more use of.
@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

  1. OK, that's a plan. i2c0-bcm2708 will kill the mux and give you i2c0 on the specified node. I suspect it will have issues with dynamic loading as it would need to rebind the i2c nodes in /dev to alternate numbers.
  2. As just discussed, those early models don't bring the alternative pin mux options out to a useful header. One BSC always goes to header pins 3&5, and the other BSC goes to the camera. Blast, B rev2 exposes GPIOs 28 & 29 on the P5 header. OK, I can look at splitting rev1 & 2 so that we can fully describe that. I'm still tempted to leave the mux out though for those boards - if I load the mux by default then it'll claim 28 & 29, and P5 was generally used for I2S so we'd end up with a conflict.

@6by9 6by9 force-pushed the 6by9:unicam_4_14_rebased branch from c42b8ee to 61f6924 Jun 8, 2018

@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

I'm trying to push just the driver changes here, leaving DT changes for a new PR. Looks like I've left the CSI DT changes in here though, so updates incoming.

6by9 and others added some commits Mar 20, 2017

ov5647: Add set_fmt and get_fmt calls.
There's no way to query the subdevice for the supported
resolutions.
Add set_fmt and get_fmt implementations.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
ov5647: dt: add device tree for PWDN control
Add optional GPIO pwdn to connect to the PWDN line on the sensor.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
ov5647: Add support for PWDN GPIO.
Add support for an optional GPIO connected to PWDN on the sensor.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
ov5647: Add support for non-continuous clock mode
The driver was only supporting continuous clock mode
although this was not stated anywhere.
Non-continuous clock saves a small amount of power and
on some SoCs is easier to interface with.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
media: tc358743: Increase FIFO level to 374.
The existing fixed value of 16 worked for UYVY 720P60 over
2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
1080P60 needs 6 lanes at 594MHz).
It doesn't allow for lower resolutions to work as the FIFO
underflows.

374 is required for 1080P24-30 UYVY over 2 lanes @ 972Mbit/s, but
>374 means that the FIFO underflows on 1080P50 UYVY over 2 lanes
@ 972Mbit/s.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
tc358743: fix connected/active CSI-2 lane reporting
g_mbus_config was supposed to indicate all supported lane numbers, not
only the number of those currently in active use. Since the TC358743
can dynamically reduce the number of active lanes if the required
bandwidth allows for it, report all lane numbers up to the connected
number of lanes as supported in pdata mode.
In device tree mode, do not report lane count and clock mode at all, as
the receiver driver can determine these from the device tree.

To allow communicating the number of currently active lanes, add a new
bitfield to the v4l2_mbus_config flags. This is a temporary fix, to be
used only until a better solution is found.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
tc358743: Add support for 972Mbit/s link freq.
Adds register setups for running the CSI lanes at 972Mbit/s,
which allows 1080P50 UYVY down 2 lanes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>

@6by9 6by9 force-pushed the 6by9:unicam_4_14_rebased branch from 61f6924 to a355eff Jun 8, 2018

@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

OK, I think that's the driver side all sorted. New PR incoming for DT.

@6by9 6by9 referenced this pull request Jun 8, 2018

Open

Unicam driver DT changes #2578

@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

@pelwell Can this lot get a review please?
I can then sort overlays for the devices that just remux i2c0 rather than needing all the changes of #2578.

@pelwell
Copy link
Contributor

left a comment

I didn't review the driver internals in detail - that's a job for V4L2 experts upstream - but apart from the questions and comments it looks OK.

static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
.enum_mbus_code = ov5647_enum_mbus_code,
.set_fmt = ov5647_set_get_fmt,
.get_fmt = ov5647_set_get_fmt,

This comment has been minimized.

Copy link
@pelwell

pelwell Jun 28, 2018

Contributor

I find it strange that this shared set_get handler is acceptable, but I see at least one other driver using it.

This comment has been minimized.

Copy link
@6by9

6by9 Jun 29, 2018

Author Contributor

set_fmt is obliged to do the best it can to match the requested format/resolution.
In this case there is only one format/resolution, therefore the answer is always the same, and is the same as getting the format.

This comment has been minimized.

Copy link
@pelwell

pelwell Jun 29, 2018

Contributor

So it isn't strange that the parameters supplied to the set get overwritten?

This comment has been minimized.

Copy link
@6by9

6by9 Jun 29, 2018

Author Contributor

No, not in this case.
Should the driver support multiple modes then it'd get updated to choose the best match, but often in sensor driver such as this where only certain modes are supported, that will still result in an overwrite of the input.

Docs at https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/vidioc-g-fmt.html and https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/vidioc-subdev-g-fmt.html#vidioc-subdev-g-fmt (Main V4L2 and subdev APIs respectively.)
V4L2 docs section 7.31.4 para 3:

When the application calls the VIDIOC_S_FMT ioctl with a pointer to a struct v4l2_format structure the driver checks and adjusts the parameters against hardware abilities. Drivers should not return an error code unless the type field is invalid, this is a mechanism to fathom device capabilities and to approach parameters acceptable for both the application and driver. On success the driver may program the hardware, allocate resources and generally prepare for data exchange. Finally the VIDIOC_S_FMT ioctl returns the current format parameters as VIDIOC_G_FMT does. Very simple, inflexible devices may even ignore all input and always return the default parameter

switch (state->csi_lanes_in_use) {
case 1:
/* Support for non-continuous CSI-2 clock is missing in pdata mode */
cfg->flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;

This comment has been minimized.

Copy link
@pelwell

pelwell Jun 28, 2018

Contributor

Why is this assignment unconditional, and not copying (some of) state->bus.flags?

This comment has been minimized.

Copy link
@6by9

6by9 Jun 29, 2018

Author Contributor

Sorry, missed this one earlier. That's part of an upstream commit by Philipp Zabel that needs to be pinged. https://patchwork.linuxtv.org/patch/44397/

Much discussion on the linux-media over g_mbus_config fullstop. Hans Verkuil was fighting that it was unneeded and deprecated as DT conveys all the info, but that doesn't handle the case where the device (eg TC358743) can dynamically choose to use fewer lanes than are physically present (DT only describes those physically present).
This line is part of the original mode of operation, but is now only valid for platform data driven systems (Cisco use this chip and driver in one of their video conferencing products via platform data). DT systems have already aborted by this point.

@@ -219,7 +223,7 @@ static noinline void i2c_wrreg(struct v4l2_subdev *sd, u16 reg, u32 val, u32 n)

static u8 i2c_rd8(struct v4l2_subdev *sd, u16 reg)
{
return i2c_rdreg(sd, reg, 1);
return i2c_rdreg(sd, reg, 1, NULL);

This comment has been minimized.

Copy link
@pelwell

pelwell Jun 28, 2018

Contributor

This is a bit ugly - why not make i2c_rdreg a call to a new function - i2c_rdreg_err, say, passing in a NULL error pointer? The callers that care about the error would then call the new function.

This comment has been minimized.

Copy link
@6by9

6by9 Jun 29, 2018

Author Contributor

Will do.

@@ -1266,6 +1267,18 @@ static int init_device(struct adv7180_state *state)
goto out_unlock;
}

/* Select first valid input */
for (i = 0; i < 31; i++) {

This comment has been minimized.

Copy link
@pelwell

pelwell Jun 28, 2018

Contributor

Is it not possible to have 32 inputs? Is 31 the best maximum?

This comment has been minimized.

Copy link
@6by9

6by9 Jun 29, 2018

Author Contributor

The valid values are defined by ADV7180_INPUT_* and ADV7182_INPUT_* at line 156ish. They max out at 0x11.
Checking the datasheet, whilst INSEL is 5 bits, 10010 to 11111 are reserved, so there is no valid use case for it to be set to greater than 0x11.

Seeing as I need to update tc358743, I'll amend this to check the top bit anyway.

6by9 added some commits Nov 28, 2017

media: tc358743: Check I2C succeeded during probe.
The probe for the TC358743 reads the CHIPID register from
the device and compares it to the expected value of 0.
If the I2C request fails then that also returns 0, so
the driver loads thinking that the device is there.

Generally I2C communications are reliable so there is
limited need to check the return value on every transfer,
therefore only amend the one read during probe to check
for I2C errors.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
adv7180: Default to the first valid input
The hardware default is differential CVBS on AIN1 & 2, which
isn't very useful.

Select the first input that is defined as valid for the
chip variant (typically CVBS_AIN1).

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
videodev2: Add helper defines for printing FOURCCs
New helper defines that allow printing of a FOURCC using
printf(V4L2_FOURCC_CONV, V4L2_FOURCC_CONV_ARGS(fourcc));

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
dt-bindings: Document BCM283x CSI2/CCP2 receiver
Document the DT bindings for the CSI2/CCP2 receiver peripheral
(known as Unicam) on BCM283x SoCs.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Acked-by: Rob Herring <robh@kernel.org>
bcm2835-unicam: Driver for CCP2/CSI2 camera interface
Add driver for the Unicam camera receiver block on
BCM283x processors.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
bcm2835-unicam: Revert changes for notifier fn ptrs
4.15 is using const function pointers for the notifier
functions. Those changes don't cherry-pick easily, so
revert those mods in this driver.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
MAINTAINERS: Add entry for BCM2835 camera driver
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
defconfig: Enable Unicam driver and various sources on Pi platforms.
Enable:
	VIDEO_V4L2_SUBDEV_API=y
	VIDEO_BCM2835_UNICAM=m
	VIDEO_TC358743=m
	VIDEO_ADV7180=m
	VIDEO_OV5647=m
so that we can receive CSI data from these devices.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
media: adv7180: Nasty hack to allow input selection.
Whilst the adv7180 driver support s_routing, nothing else
does, and there is a missing lump of framework code to
define the mapping from connectors on a board to the inputs
they represent on the ADV7180.

Add a nasty hack to take a module parameter that is passed in
to s_routing on any call to G_STD, or S_STD (or subdev
g_input_status call).

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
media: adv7180: Add YPrPb support for ADV7282M
The ADV7282M can support YPbPr on AIN1-3, but this was
not selectable from the driver. Add it to the list of
supported input modes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>

@6by9 6by9 force-pushed the 6by9:unicam_4_14_rebased branch from a355eff to 086e909 Jun 29, 2018

@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

I think that addresses your review comments - thank you :-)

@pelwell pelwell merged commit 1e72a58 into raspberrypi:rpi-4.14.y Jun 29, 2018

@pelwell

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

Ship it!

@6by9

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

:-o It's finally merged (even if not upstream).

I will kick all those driver patches upstream (except the debug hack on adv7180) when I have the energy.

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jul 9, 2018

kernel: Bump to 4.14.54
kernel: vc4 backports
See: raspberrypi/linux#2605

kernel: Unicam simplified DT overlays
See: raspberrypi/linux#2607

kernel: Unicam driver
See: raspberrypi/linux#2513

firmware: arm_dt: Protect against seg-fault when dt failed to load

firmware: isp: Alter logging level for buffer size errors
firmware: isp: Return output buffers that are too small
firmware: video_encode: Integrate the ISP for format conversion
firmware: mmal: Populate buffer->type->video.flags field
firmware: image_fx: Apply interlacing flags from the plugin to output frames

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Jul 9, 2018

kernel: Bump to 4.14.54
kernel: vc4 backports
See: raspberrypi/linux#2605

kernel: Unicam simplified DT overlays
See: raspberrypi/linux#2607

kernel: Unicam driver
See: raspberrypi/linux#2513

firmware: arm_dt: Protect against seg-fault when dt failed to load

firmware: isp: Alter logging level for buffer size errors
firmware: isp: Return output buffers that are too small
firmware: video_encode: Integrate the ISP for format conversion
firmware: mmal: Populate buffer->type->video.flags field
firmware: image_fx: Apply interlacing flags from the plugin to output frames

@6by9 6by9 referenced this pull request Jul 11, 2018

Closed

ISL79987 Support #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.