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

Alderalke branch to development. #12060

Merged
merged 49 commits into from
Aug 29, 2023
Merged

Alderalke branch to development. #12060

merged 49 commits into from
Aug 29, 2023

Conversation

dmipx
Copy link
Contributor

@dmipx dmipx commented Aug 1, 2023

All ipu6 & jetson enhancements to backend.
Support for multiple DFU devices.

dmipx and others added 30 commits January 5, 2023 17:04
backend-v4l2: MPLANE supported for 1 plane
IPU6 currently supports only one [1] plane
backend-v4l2: Metadata for IPU6 with MPLANE [1] support.
IPU6 currently supports only MPLANE streaming metadata as video stream resolution
[ADL-P] LRS V4L2 Backend Enumerate video nodes by name [LRS-573]

rs-enum.sh:
 - Create symbolic links for video nodes and for metadata nodes - /dev/video-rs-[<sensor>|<sensor>-md]-[camera-index]
 - Enable ipu6 link enumeration

backend-v4l2.cpp:
 - scan for rs-enum links

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
rs-enum: add comments, change rgb to color.
backend-v4l2: change rgb to color, comments, try/catch for throwing module.

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
rs_sensor: RS2_CAMERA_INFO_DFU_DEVICE_PATH

Tracked-by: [DSO-18802] [ADL-P] D4xx DFU support for LRS

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
IPU6 d4xx udev rules for binding and enumeration

[DSO-18804] Tracking by

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Metadata structure will be changed in future kernel releases,
expected from version > 6.4.
This patch helps avoid system header file change with Sakari metadata patch.

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
@dmipx dmipx requested review from remibettan and Nir-Az August 1, 2023 10:43
@@ -16,14 +16,30 @@
#
# Alderlake:
#$ ./rs-enum.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some comment with explanation of the commands sent in this script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are not commenting c++ code for every magic you do.
This is simple filtering, nothing specific can be commented. No magic involved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Remi meant command line parameters
i.e.

# parse command line parameters
# for '-i' parameter, print links only

@@ -0,0 +1,78 @@
#!/bin/bash
# Dependency: v4l-utils
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some comment with explanation of the commands sent in this script

_offset = buf.m.offset;
if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
_original_length = buf.m.planes[0].length;
_offset = buf.m.planes[0].m.mem_offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

could a const value be used here instead of planes[0]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as soon we support only one plane in multi-plane buffer we have to leave that like so.
We had no use case for multiple planes and no system to verify on.


if (_type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
buf.m.planes = planes;
buf.length = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

please consider replacing by some const, and add some comment to explain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is for single buffer on multiplane, we have no other cases to verify what can be passsed to it for now.

@@ -1362,6 +1397,11 @@ namespace librealsense
// Drop partial and overflow frames (assumes D4XX metadata only)
bool partial_frame = (!compressed_format && (buf.bytesused < buffer->get_full_length() - MAX_META_DATA_SIZE));
bool overflow_frame = (buf.bytesused == buffer->get_length_frame_only() + MAX_META_DATA_SIZE);
if (_dev.buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
/* metadata size is one line of profile, temporary disable validation */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you implement a better solution for that, that will still detect partial frame and overflow frame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in ipu6 case we will always have full frame as it padding it by itself.
We can work on this in future

for (const uint32_t& request : { V4L2_META_FMT_D4XX, V4L2_META_FMT_UVC})
{
// Configure metadata format - try d4xx, then fallback to currently retrieve UVC default header of 12 bytes
memcpy(fmt.fmt.raw_data,&request,sizeof(request));
memcpy(fmt.fmt.raw_data, &request, sizeof(request));
// use only for IPU6?
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct the comment - "?" should be solved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will append line for submitted kernel patch

enum v4l2_buf_type buf_type;
unsigned char num_planes;
struct v4l2_capability cap;
struct v4l2_cropcap cropcap;
Copy link
Contributor

Choose a reason for hiding this comment

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

please add some comment to explain the meaning of cropcap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no function for LRS but it was polled this device capability in past. This is "crop capability" for frame cropping.

Copy link
Contributor

@remibettan remibettan left a comment

Choose a reason for hiding this comment

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

Few comments - impressive work!

# DFU rules
SUBSYSTEM=="d4xx-class",KERNEL=="d4xx-dfu*", GROUP="video", MODE="0660"
# video links for SDK, binding for ipu6
SUBSYSTEM=="video4linux", ATTR{name}=="DS5 mux *", RUN+="/bin/bash -c 'rs_ipu6_d457_bind.sh > /dev/kmsg; rs-enum.sh -q > /dev/kmsg'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to identify we are running on ADL-P and only run this script if needed?
Not sure jetson users want to see IPU6 script call..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after looking on debian package for udev rules, we have no way to get that information on debian installation stage.
I decided to merge all paltforms to one script. If it is so crucial - we can add post installation step and remove unnecessary files and modify rules but it will be a mess..

if [[ -n "${is_ipu6}" ]] || [[ -n "${is_tegra}" ]]; then
sudo cp config/99-realsense-d4xx-mipi-dfu.rules /etc/udev/rules.d/
sudo cp scripts/rs-enum.sh /usr/local/bin/rs-enum.sh
sudo cp scripts/rs_ipu6_d457_bind.sh /usr/local/bin/rs_ipu6_d457_bind.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not copy only relevant script based on is_tegra/is_ipu6?

Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trying to keep similarity for debian package (udev-rules)

@@ -789,8 +805,9 @@ namespace librealsense
// assign unique id for mipi by appending camera id to bus_info (bus_info is same for each mipi port)
// Note - jetson can use only bus_info, as card is different for each sensor and metadata node.
info.unique_id = bus_info + "-" + std::to_string(cam_id); // use bus_info as per camera unique id for mipi
info.dfu_device_path = "/dev/d4xx-dfu504"; // Use legacy DFU device node used in firmware_update_manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this hard coded path say we dont support multi camera DFU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it comes form fw-update-helper.cpp

if ((_dev.cap.capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) && !is_platform_jetson())
{
/* Sakari patch for videodev2.h. This structure will be within kernel > 6.4 */
struct v4l2_meta_format {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we share the patch link instead of writing "Sakari patch"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will append line for submitted kernel patch

Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

Great work overall..
A few comment.

Since this goes public to all users, lets clean it up according to the comments and run CI builds to see no reggression on USB / Mipi sku's
Thanks
P.S any change need ls to be in debian repo? regarding udev rules / rs_enum?

@Nir-Az Nir-Az merged commit 6514916 into development Aug 29, 2023
@Nir-Az Nir-Az deleted the adl-p branch January 9, 2024 12:20
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.

3 participants