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

Rpi/embedded data support/rpi 6.6.y #6437

Open
wants to merge 159 commits into
base: rpi-6.6.y
Choose a base branch
from

Conversation

jmondi
Copy link
Contributor

@jmondi jmondi commented Oct 24, 2024

This branch backports embedded data support to rpi-6.6.y

It includes framework changes to support embedded data and

  • CFE from mainline
  • Unicam from mainline
  • Aligns imx219 with mainline
  • Sensor patches from Naush to support embedded data

The branch compiles with rpi defconfig but fails for other drivers not part of defconfig. I'm working to backport the necessary changes there too.

The associated libcamera branch will be pushed soon.


/* Set vsync trigger mode: 0=standalone, 1=source, 2=sink */
tm = (imx477->trigger_mode_of >= 0) ? imx477->trigger_mode_of : trigger_mode;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naushir Naush I haven't changed this, but this hunk seems to revert 074f41b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably an error on my part!

@jmondi jmondi force-pushed the rpi/embedded-data-support/rpi-6.6.y branch from aa125fe to 31804e2 Compare October 24, 2024 16:31
@jmondi
Copy link
Contributor Author

jmondi commented Oct 24, 2024

Refreshed the pull request, compared to v1:

  • Test compiling with allmodconfig
  • Compact backports at the beginning of the series
  • Drop ov2740 multistream support
  • Backport 2 BSP patches on mainline unicam
  • Backport CFE from the patches merged in media-stage and not from the mailing list
  • Fix imx477 multi-stream support reverting a BSP commit

@jmondi jmondi force-pushed the rpi/embedded-data-support/rpi-6.6.y branch from 31804e2 to e002d10 Compare October 25, 2024 09:01
@jmondi
Copy link
Contributor Author

jmondi commented Oct 25, 2024

Added three patches from @jailuthra on top of Naush's one for sensors to add support for enable/disable_streams.

We kept them separate to ease review as they apply on top of Naush's ones. They can be squashed in later.

/* TODO: Set rate_factor and binning mode together */
if (((crop->height / format->height) == 2) ||
format->height == 480)
imx219->rate_factor = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed the check for bpp == 8 here.

I will rework f54e586 and also see if it can be posted upstream.

@jmondi jmondi force-pushed the rpi/embedded-data-support/rpi-6.6.y branch from e002d10 to 83d0d4f Compare October 28, 2024 14:15
@jmondi
Copy link
Contributor Author

jmondi commented Oct 28, 2024

Re-established the correct handling of analog binning mode for imx219 by applying ef737ab

@naushir
Copy link
Contributor

naushir commented Nov 1, 2024

Starting to test this now together with raspberrypi/libcamera#200 and all looks ok so far...

@jmondi jmondi force-pushed the rpi/embedded-data-support/rpi-6.6.y branch from 83d0d4f to 739ce93 Compare November 8, 2024 10:52
@jmondi
Copy link
Contributor Author

jmondi commented Nov 8, 2024

Rebased on most recent rpi-6.6.y and apply the following change

@ -1025,7 +1025,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
	 * embedded data stream.
	 */
	ed_format = v4l2_subdev_state_get_format(state, IMX219_PAD_EDATA);
-	ed_format->code = imx219_format_edata(format->code);
+	ed_format->code = MEDIA_BUS_FMT_CCS_EMBEDDED;
	ed_format->width = format->width;
	ed_format->height = IMX219_EMBEDDED_DATA_HEIGHT;
	ed_format->field = V4L2_FIELD_NONE;
@ -1033,6 +1033,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
	format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE,
					      IMX219_STREAM_EDATA);
	*format = *ed_format;
+	format->code = imx219_format_edata(format->code);

	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
		u32 prev_hts = format->width + imx219->hblank->val;

To imx219, imx277, imx519 and imx708

@naushir
Copy link
Contributor

naushir commented Nov 12, 2024

Been testing on VC4 platforms, the one bit of functionality seems missing is allowing unpacked formats to be used. I think the following fixes it:

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index 1a6a0eacd49d..dce1047c3baa 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -550,7 +550,8 @@ unicam_find_format_by_fourcc(u32 fourcc, u32 pad)
        }

        for (i = 0; i < num_formats; ++i) {
-               if (formats[i].fourcc == fourcc)
+               if (formats[i].fourcc == fourcc ||
+                   formats[i].unpacked_fourcc == fourcc)
                        return &formats[i];
        }

@6by9, our D/S unicam driver seems to do something with a check_variant flag in this bit of code that I can't quite understand. Do we want to add it back here?

@naushir
Copy link
Contributor

naushir commented Nov 12, 2024

Also PDAF does not seem to work correctly on Unicam + IMX708 - will need to check this next.

@6by9
Copy link
Contributor

6by9 commented Nov 12, 2024

MEDIA_BUS_FMT_YUYV8_2X8 and MEDIA_BUS_FMT_YUYV8_1X16 both mapped to V4L2_PIX_FMT_YUYV (and the other component orderings).

Mainline try to say that 2X.. shouldn't be used for serial interfaces (only parallel where you can have multiple transfers per pixel). However that isn't true in all drivers, including adv7180. adv748x afe got fixed up recently.

I have fixed adv7180 up downstream in 4f9944c, and that's the only device we really care about that needed this workaround. (Yes, another patch I ought to send upstream)

@naushir
Copy link
Contributor

naushir commented Nov 13, 2024

Also PDAF does not seem to work correctly on Unicam + IMX708 - will need to check this next.

For some reason, the first frame has invalid PDAF/HDR metadata when running with Unicam, subsequent frames are fine. CFE/Pi 5 does not seem to show this problem. @njhollinghurst any thoughts as to why this may be? The following test in the helper fails:

if (bpp < 10 || bpp > 14 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {

because ptr[0] != 0.

@njhollinghurst
Copy link
Contributor

njhollinghurst commented Nov 13, 2024

Of course it means the PDAF data packet was corrupt (or the packet at that position was not PDAF). It's a pretty weak validation -- only 2 bytes are checked -- and since the driver doesn't say how many bytes were received, we can't tell if we're checking current or stale buffer contents. EDIT: The nonzero value does at least suggest we received something...

Is it possible that this always happens (in which case the warning should maybe be suppressed), but in other cases we drop the entire first metadata buffer so the check is never performed?

@naushir
Copy link
Contributor

naushir commented Nov 14, 2024

In the old (i.e. existing) codebase, the first frame coming out of the sensor/Unicam goes through to the helper and gets parsed without errors. I'll trace this again with the new code.

@jmondi
Copy link
Contributor Author

jmondi commented Nov 18, 2024

MEDIA_BUS_FMT_YUYV8_2X8 and MEDIA_BUS_FMT_YUYV8_1X16 both mapped to V4L2_PIX_FMT_YUYV (and the other component orderings).

Mainline try to say that 2X.. shouldn't be used for serial interfaces (only parallel where you can have multiple transfers per pixel). However that isn't true in all drivers, including adv7180. adv748x afe got fixed up recently.

I have fixed adv7180 up downstream in 4f9944c, and that's the only device we really care about that needed this workaround. (Yes, another patch I ought to send upstream)

Hi Dave
do you think you will need to keep the support for the 2X8 formats once [4f9944c] is upstreamed ?

@6by9
Copy link
Contributor

6by9 commented Nov 18, 2024

Hi Dave do you think you will need to keep the support for the 2X8 formats once [4f9944c] is upstreamed ?

No, it's officially the wrong thing to do, and doesn't impact any of the source drivers that we care about.
If someone tries adding support for a driver that "does the wrong thing", they get told to fix up the source driver.

At the time I originally wrote the Unicam driver I don't think 2X formats had been officially been stated as not applicable for CSI, hence working around the source driver quirks.

@naushir
Copy link
Contributor

naushir commented Nov 19, 2024

Wow, this was hard to track down, but I think I've fixed the Unicam PDAF/HDR metadata problem. In fact, I had fixed it 2 years ago with 59df4fc. With this change applied things seem to behave consistently across the existing codebase and this new set of changes.

But I probably ought to check that all our other downstream fixes have also made it into the driver.

@naushir
Copy link
Contributor

naushir commented Nov 19, 2024

Sadly there are a couple more ISR fixes missing from the upstream driver. No way to cherry pick single commits, I've got a commit that fixes everything here: naushir@26b3a09

@jmondi
Copy link
Contributor Author

jmondi commented Nov 19, 2024

Sadly there are a couple more ISR fixes missing from the upstream driver. No way to cherry pick single commits, I've got a commit that fixes everything here: naushir@26b3a09

Thank you for your hard work in chasing down issues with the mainline unicam driver.

Would you like me to check if changes from naushir@26b3a09 can be broken down to single commits and applied to the mainline version ?

@naushir
Copy link
Contributor

naushir commented Nov 19, 2024

Would you like me to check if changes from naushir@26b3a09 can be broken down to single commits and applied to the mainline version ?

No need, I'm in the process of doing that right now. Basically 3 commits - 1) Dummy buffer 0 size, 2) format change 3) ISR robustness improvements. I'll post the patches shortly.

@naushir
Copy link
Contributor

naushir commented Nov 19, 2024

I think the top 4 commit from https://github.com/naushir/linux/commits/streams/ should be added to this and it looks to be passing all my tests on VC4.

Sakari Ailus and others added 5 commits November 21, 2024 17:34
The v4l2_pipeline_pm_get() and v4l2_pipeline_pm_put() functions were
needed to control sub-devices' power states before runtime PM. These
functions should no longer be used, and instead sub-device drivers should
use runtime PM.

Signed-off-by: Sakari Ailus <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
(cherry picked from commit 6d7ca3c9c1de48c627769602af9cf841459c1828)
Drop an unneeded double colon, and use 'shall' instead of 'must' for
consistency with the rest of the file.

Signed-off-by: Laurent Pinchart <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
(cherry picked from commit 5adf57f8a9ddf1540a27c132af47bb1349bfe170)
Signed-off-by: Jacopo Mondi <[email protected]>
The Documentation/devicetree/bindings/clock/clock-bindings.txt file is
deprecated and points to clock-bindings.yaml, which is not hosted in the
kernel source tree. Use an HTTPS link to refer to the YAML binding
document.

While at it, drop "currently" from the paragraph, as the whole file
refers to the current recommended practices except where explicitly
noted.

Signed-off-by: Laurent Pinchart <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
(cherry picked from commit a7acee9965d914202386ff438799feed74bd504c)
Signed-off-by: Jacopo Mondi <[email protected]>
Move the power management section up, just after clocks, as it relates
to internal system resources and not features exposed to applications.
The text itself is otherwise unchanged.

Signed-off-by: Laurent Pinchart <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
(cherry picked from commit 3a8b77f735ce0c17a57025426d79764d74c41047)
Signed-off-by: Jacopo Mondi <[email protected]>
…tation

Camera sensor drivers are highly subject to cargo cult programming, with
back practices being copied from old to new drivers. In particular, many
drivers implement system and runtime PM incorrectly. As a first step
towards fixing this situation, refactor and expand the power management
documentation to detail correct usage of system and runtime PM.

Signed-off-by: Laurent Pinchart <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
(cherry picked from commit 072278810ca27421fb75d5ad8ce5b310ad09a267)
Signed-off-by: Jacopo Mondi <[email protected]>
pinchartl and others added 27 commits November 21, 2024 18:54
Calculate the crop rectangle size and location dynamically when setting
the format, instead of storing it in the imx219_mode structure. This
removes duplicated information from the mode, to guarantee consistency.

Signed-off-by: Laurent Pinchart <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
Subdev state variables are named with a mix of 'state' and 'sd_state'
through the driver. To improve consistency, name them all 'state'.

Signed-off-by: Laurent Pinchart <[email protected]>
Reviewed-by: Dave Stevenson <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
The exposure_max, exposure_def and hblank variables are only used in an
inner scope in the imx219_set_pad_format() function. Move them to that
scope to keep them closer to their usage and improve readability.

Signed-off-by: Laurent Pinchart <[email protected]>
Reviewed-by: Dave Stevenson <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
The imx219_update_pad_format() is short and called from a single place,
in imx219_set_pad_format(). Inline the code in the caller to keep all
format adjustments grouped in a single place and improve readability.

Signed-off-by: Laurent Pinchart <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
Reviewed-by: Dave Stevenson <[email protected]>
Use the newly added internal pad API to expose the internal
configuration of the sensor to userspace in a standard manner. This also
paves the way for adding support for embedded data with an additional
internal pad.

To maintain compatibility with existing userspace that may operate on
pad 0 unconditionally, keep the source pad numbered 0 and number the
internal image pad 1.

Signed-off-by: Laurent Pinchart <[email protected]>
---
Changes since v7:

- Set IMX219_PAD_SOURCE to 0 explicitly
- Update comment to clarify the whole image pad format is fixed
- Compare code->index with 0
- Drop unneeded parentheses
In preparation for embedded data stream support, introduce a new
imx219_stream_ids enumeration for stream IDs, with a single value,
IMX219_STREAM_IMAGE for the image data stream. Use it when accessing the
formats, crop and compose rectangles on the source pad. This is meant to
reduce the size of further commits, and doesn't introduce any functional
change.

Signed-off-by: Laurent Pinchart <[email protected]>
The imx219 driver implements the .s_stream() operation, which is not
compatible with streams. In preparation for the addition of streams
support in the driver, switch to the .enable_streams() and
.disable_streams() operations.

Signed-off-by: Laurent Pinchart <[email protected]>
Usage of internal pads creates a route internal to the subdev, and the
V4L2 camera sensor API requires such routes to be reported to userspace.
Create the route in the .init_state() operation.

Internal routing support requires stream support, so set the
V4L2_SUBDEV_FL_STREAMS flag. As the route is immutable, there's no need
to implement the .set_routing() operation.

Signed-off-by: Laurent Pinchart <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
---
Changes since v7:

- Update commit message to match the changes in v6
- Fix name of STREAMS flag in commit message
- Use IMX219_STREAM_IMAGE

Changes since v6:

- Drop change to get format API in imx219_set_ctrl()
- Fix function name in commit message
- Set V4L2_SUBDEV_ROUTE_FL_IMMUTABLE flag on route
Implement the .get_frame_desc() subdev operation to report information
about streams to the connected CSI-2 receiver. This is required to let
the CSI-2 receiver driver know about virtual channels and data types for
each stream.

Signed-off-by: Laurent Pinchart <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
---
Changes since v7:

- Use IMX219_STREAM_IMAGE
- Drop unneeded memset()

Changes since v6:

- Replace v4l2_subdev_state_get_stream_format() with
  v4l2_subdev_state_get_format()
- Make imx219_format_bpp() return an unsigned int
The IMX219 generates embedded data unconditionally. Report it as an
additional stream, with a new internal embedded data pad, and update
subdev operations accordingly.

Signed-off-by: Laurent Pinchart <[email protected]>
---
Changes since v7:

- Compare fse->index with 0 in imx219_enum_frame_size()
- Don't set route array size explicitly in imx219_init_state()
- Implement imx219_format_edata() on top of imx219_format_bpp()
- Handle streams in .{enable,disable}_streams()

Changes since v6:

- Get format from IMX219_STREAM_IMAGE in imx219_set_ctrl()
- Fix mbus code for second stream in imx219_get_frame_desc()
- Set V4L2_SUBDEV_ROUTE_FL_IMMUTABLE flag on route
The datasheet for this sensor documents the minimum vblanking as being
32 lines. It does fix some problems with occasional black lines at the
bottom of images (tested on Raspberry Pi).

Signed-off-by: David Plowman <[email protected]>
The HBLANK control was read-only, and always configured such
that the sensor HTS register was 3448. This limited the maximum
exposure time that could be achieved to around 1.26 secs.

Make HBLANK read/write so that the line time can be extended,
and thereby allow longer exposures (and slower frame rates).
Retain the overall HTS setting when changing modes rather than
resetting it to a default.

Signed-off-by: Dave Stevenson <[email protected]>
Signed-off-by: Jai Luthra <[email protected]>
The 640x480 mode uses a special binning mode for high framerate operation where
the pixel rate is effectively doubled. Account for this when setting up the
pixel clock rate, and applying the vblank and exposure controls.

Signed-off-by: Naushir Patuck <[email protected]>
Signed-off-by: Jai Luthra <[email protected]>
HTS was still using the raw register ID.

Fixes: dd26d43 ("media: i2c: imx219: make HBLANK r/w to allow longer exposures")
Signed-off-by: Dave Stevenson <[email protected]>
At a high FPS with RAW10, there is frame corruption for 480p because the
rate_factor of 2 is used with the normal 2x2 bining [1]. This commit
ties the rate_factor to the selected binning mode. For the 480p mode,
analog 2x2 binning mode with a rate_factor of 2 is always used. For the
1232p mode the normal 2x2 binning mode is used for RAW10 while analog
2x2 binning mode is used for RAW8.

[1] raspberrypi#5493

Signed-off-by: Vinay Varma <[email protected]>
Reworked due to upstream changes
Signed-off-by: Dave Stevenson <[email protected]>
Reworked again due to upstream changes
Signed-off-by: Jai Luthra <[email protected]>
Add the V4L2 streams API support for embedded data in the IMX477 device
driver. This also updates the drier to use the V4L2 subdev state API.

Signed-off-by: Naushir Patuck <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
The .s_stream() operation is not compatible with streams, so switch to
the .{enable,disable}_streams() operations.

Signed-off-by: Jai Luthra <[email protected]>
Add the V4L2 streams API support for embedded/pdaf/hdr data in the
IMX708 device driver. This also updates the drier to use the V4L2 subdev
state API.

Signed-off-by: Naushir Patuck <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
The .s_stream() operation is not compatible with streams, so switch to
the .{enable,disable}_streams() operations.

Signed-off-by: Jai Luthra <[email protected]>
Add the V4L2 streams API support for embedded data in the IMX519 device
driver. This also updates the drier to use the V4L2 subdev state API.

Signed-off-by: Naushir Patuck <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
The .s_stream() operation is not compatible with streams, so switch to
the .{enable,disable}_streams() operations.

Signed-off-by: Jai Luthra <[email protected]>
With the introduction of the streams API, the legacy embedded data
support has been removed. Pull out all code associated with the legacy
support.

Note that this driver has not been converted to use the streams API, so
embedded data will be unavailable.

Signed-off-by: Naushir Patuck <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
Remove v4l2_subdev_enable_streams_api variable that was used to easily
enable streams API for development, and conditions that use the variable.

This patch enables the streams API for V4L2 sub-device interface which
allows transporting multiple streams on a single MC link.

Signed-off-by: Sakari Ailus <[email protected]>
When matching formats via try_fmt/set_fmt ioctls, test for the unpacked
formats as well as packed formats. This allows userland clients setup
unpacking to 16-bits from the 10/12/14-packed CSI2 formats.

Signed-off-by: Naushir Patuck <[email protected]>
The imx219/imx708 sensors frequently generate a single corrupt frame
(image or embedded data) when the sensor first starts. This can either
be a missing line, or invalid samples within the line. This only occurrs
using the upstream Unicam kernel driver.

Disabling trigger mode elimiates this corruption. Since trigger mode is
a legacy feature copied from the firmware driver and not expected to be
needed, remove it. Tested on the Raspberry Pi cameras and shows no ill
effects.

Signed-off-by: Naushir Patuck <[email protected]>
The Unicam hardware has been observed to cause a buffer overrun when
using the dummy buffer as a circular buffer. The conditions that cause
the overrun are not fully known, but it seems to occur when the memory
bus is heavily loaded.

To avoid the overrun, program the hardware with a buffer size of 0 when
using the dummy buffer. This will cause overrun into the allocated dummy
buffer, but avoid out of bounds writes.

Signed-off-by: Naushir Patuck <[email protected]>
This change aligns the FS/FE interrupt handling with the Raspberry Pi
kernel downstream Unicam driver.

If we get a simultaneous FS + FE interrupt for the same frame, it cannot
be marked as completed and returned to userland as the framebuffer will
be refilled by Unicam on the next sensor frame. Additionally, the
timestamp will be set to 0 as the FS interrupt handling code will not
have run yet.

To avoid these problems, the frame is considered dropped in the FE
handler, and will be returned to userland on the subsequent sensor frame.

Signed-off-by: Naushir Patuck <[email protected]>
@jmondi jmondi force-pushed the rpi/embedded-data-support/rpi-6.6.y branch from 739ce93 to c48f001 Compare November 22, 2024 09:16
@jmondi
Copy link
Contributor Author

jmondi commented Nov 22, 2024

Rebased on latest v6.6.y with @naushir patches on top.

Tested on rpi4 with imx708

[0:21:30.109693327] [1825] DEBUG V4L2 v4l2_videodevice.cpp:1731 /dev/video1[19:cap]: Dequeuing buffer 0
[0:21:30.109772196] [1825] DEBUG RPI vc4.cpp:894 Stream Unicam Embedded buffer dequeue, buffer id 1, timestamp: 1290097911000
[0:21:30.109993951] [1825] DEBUG RPI vc4.cpp:1078 Signalling prepareIsp: Bayer buffer id: 1
[0:21:30.110038061] [1825] DEBUG RPI vc4.cpp:1093 Signalling prepareIsp: Embedded buffer id: 1
[0:21:30.110208650] [1825] DEBUG Event event_dispatcher_poll.cpp:215 next timer 0xf38278e8 expires in 0.998666639
[0:21:30.110830674] [1828] DEBUG IPARPI ipa_base.cpp:1299 Metadata - Exposure: 994.58us Frame length: 3619 Line length: 9.21us Gain: 1.12281 Lens: 1

Sorry for not noticing before, I didn't have an imx708 until a few days ago

@njhollinghurst
Copy link
Contributor

FWIW, it was probably not imx708 specific -- it's just that the imx708 PDAF/HDR metadata parser prints these warnings. Other camera helpers might silently fail (or succeed, if the required metadata came before the data loss).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants