-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
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>
@@ -16,14 +16,30 @@ | |||
# | |||
# Alderlake: | |||
#$ ./rs-enum.sh |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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]?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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'" |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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
There was a problem hiding this 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?
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
All ipu6 & jetson enhancements to backend.
Support for multiple DFU devices.