Message ID | 20231008085154.6757-1-jack.zhu@starfivetech.com |
---|---|
Headers | show |
Series | Add StarFive Camera Subsystem driver | expand |
On 2023/10/8 16:51, Jack Zhu wrote: > Hi, > > This series is the v10 series that attempts to support the Camera Subsystem > found on StarFive JH7110 SoC. > Hi Hans, Could you please help to review and give your comments? Thanks a lot!
On 2023/10/18 10:37, Jack Zhu wrote: > Hi Hans, > > Thank you for your comments. > > On 2023/10/16 19:40, Hans Verkuil wrote: >> Hi Jack, >> >> On 08/10/2023 10:51, Jack Zhu wrote: >>> Hi, >>> >>> This series is the v10 series that attempts to support the Camera Subsystem >>> found on StarFive JH7110 SoC. >>> >>> This series is based on top of the master branch of media_stage repository, >>> which is tested with a v4l2-compliance compiled from the git repo >>> (git://linuxtv.org/v4l-utils.git). >> >> I get one smatch warning: >> >> drivers/staging/media/starfive/camss/stf-isp.c:122 isp_enum_mbus_code() warn: unsigned 'code->index' is never less than zero. >> >> And I also notice that there is no TODO file: staging drivers should have a >> TODO file explaining what needs to be done to get them out of staging. >> >> I'm curious to know that as well :-) >> >> It looks like there is a lot of additional development that can be done, since >> most of the ISP parameters appear to be hardcoded. >> >> I also notice something weird in the compliance test output for v4l-subdev0 vs >> v4l-subdev1: >> >>> Compliance test for starfive-camss device /dev/v4l-subdev0: >>> >>> Driver Info: >>> Driver version : 6.6.0 >>> Capabilities : 0x00000000 >>> Media Driver Info: >>> Driver name : starfive-camss >>> Model : Starfive Camera Subsystem >>> Serial : >>> Bus info : platform:19840000.camss >>> Media version : 6.6.0 >>> Hardware revision: 0x00000000 (0) >>> Driver version : 6.6.0 >>> Interface Info: >>> ID : 0x0300001c >>> Type : V4L Sub-Device >>> Entity Info: >>> ID : 0x00000001 (1) >>> Name : stf_isp >>> Function : Image Signal Processor >>> Pad 0x01000002 : 0: Sink >>> Link 0x02000014: from remote pad 0x1000010 of entity 'cdns_csi2rx.19800000.csi-bridge' (Video Interface Bridge): Data, Enabled >>> Pad 0x01000003 : 1: Source >>> Link 0x0200000c: to remote pad 0x1000009 of entity 'capture_yuv' (V4L2 I/O): Data, Enabled >> >> Here it shows the Media Driver Info for v4l-subdev0. >> >> <snip> >> >>> -------------------------------------------------------------------------------- >>> Compliance test for device /dev/v4l-subdev1: >>> >>> Driver Info: >>> Driver version : 6.6.0 >>> Capabilities : 0x00000000 >> >> But this does not appear for v4l-subdev1. >> >> I can't really tell why it doesn't show that. Can you debug a little bit? >> The code is in v4l2-compliance.cpp, line 1086: >> >> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >> >> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >> and that fails for some reason. It could be that media_fd is invalid (would be weird). >> >> This could well be a v4l2-compliance bug that you hit with this driver. >> > > On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is > /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd > obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. > > I don't know why media0 does not exist in /sys/dev/char/81:3/device? > Hi Hans, Could you please comment on this issue? imx219 directly uses the driver file in the media_stage repository. Thank you for your time!
Hi Jack, On 18/10/2023 04:37, Jack Zhu wrote: <snip> >>> -------------------------------------------------------------------------------- >>> Compliance test for device /dev/v4l-subdev1: >>> >>> Driver Info: >>> Driver version : 6.6.0 >>> Capabilities : 0x00000000 >> >> But this does not appear for v4l-subdev1. >> >> I can't really tell why it doesn't show that. Can you debug a little bit? >> The code is in v4l2-compliance.cpp, line 1086: >> >> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >> >> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >> and that fails for some reason. It could be that media_fd is invalid (would be weird). >> >> This could well be a v4l2-compliance bug that you hit with this driver. >> > > On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is > /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd > obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. > > I don't know why media0 does not exist in /sys/dev/char/81:3/device? > Can you try again with this v4l2-compliance patch? I need to dig a bit deeper as to why media0 is missing, but for now try this. Regards, Hans diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp index 7169eefe..29475d6b 100644 --- a/utils/v4l2-compliance/v4l2-compliance.cpp +++ b/utils/v4l2-compliance/v4l2-compliance.cpp @@ -968,7 +968,7 @@ err: } void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, - unsigned frame_count, unsigned all_fmt_frame_count) + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd) { struct node node2; struct v4l2_capability vcap = {}; @@ -997,8 +997,12 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ memset(&vcap, 0, sizeof(vcap)); } - if (!node.is_media()) - media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); + if (!node.is_media()) { + if (parent_media_fd >= 0) + media_fd = parent_media_fd; + else + media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); + } int fd = node.is_media() ? node.g_fd() : media_fd; if (fd >= 0) { diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h index 7caf254b..c47f25f5 100644 --- a/utils/v4l2-compliance/v4l2-compliance.h +++ b/utils/v4l2-compliance/v4l2-compliance.h @@ -308,7 +308,7 @@ int check_ustring(const __u8 *s, int len); int check_0(const void *p, int len); int restoreFormat(struct node *node); void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, - unsigned frame_count, unsigned all_fmt_frame_count); + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd = -1); std::string stream_from(const std::string &pixelformat, bool &use_hdr); // Media Controller ioctl tests
On 2023/10/18 16:50, Hans Verkuil wrote: > Hi Jack, > > On 18/10/2023 04:37, Jack Zhu wrote: > > <snip> > >>>> -------------------------------------------------------------------------------- >>>> Compliance test for device /dev/v4l-subdev1: >>>> >>>> Driver Info: >>>> Driver version : 6.6.0 >>>> Capabilities : 0x00000000 >>> >>> But this does not appear for v4l-subdev1. >>> >>> I can't really tell why it doesn't show that. Can you debug a little bit? >>> The code is in v4l2-compliance.cpp, line 1086: >>> >>> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >>> >>> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >>> and that fails for some reason. It could be that media_fd is invalid (would be weird). >>> >>> This could well be a v4l2-compliance bug that you hit with this driver. >>> >> >> On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is >> /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd >> obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. >> >> I don't know why media0 does not exist in /sys/dev/char/81:3/device? >> > > Can you try again with this v4l2-compliance patch? > > I need to dig a bit deeper as to why media0 is missing, but for now try this. > > Regards, > > Hans > > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp > index 7169eefe..29475d6b 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.cpp > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp > @@ -968,7 +968,7 @@ err: > } > > void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, > - unsigned frame_count, unsigned all_fmt_frame_count) > + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd) > { > struct node node2; > struct v4l2_capability vcap = {}; > @@ -997,8 +997,12 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > memset(&vcap, 0, sizeof(vcap)); > } > > - if (!node.is_media()) > - media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); > + if (!node.is_media()) { > + if (parent_media_fd >= 0) > + media_fd = parent_media_fd; > + else > + media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); > + } > > int fd = node.is_media() ? node.g_fd() : media_fd; > if (fd >= 0) { > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > index 7caf254b..c47f25f5 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.h > +++ b/utils/v4l2-compliance/v4l2-compliance.h > @@ -308,7 +308,7 @@ int check_ustring(const __u8 *s, int len); > int check_0(const void *p, int len); > int restoreFormat(struct node *node); > void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, > - unsigned frame_count, unsigned all_fmt_frame_count); > + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd = -1); > std::string stream_from(const std::string &pixelformat, bool &use_hdr); > > // Media Controller ioctl tests >
On 18/10/2023 11:25, Jack Zhu wrote: > > > On 2023/10/18 16:50, Hans Verkuil wrote: >> Hi Jack, >> >> On 18/10/2023 04:37, Jack Zhu wrote: >> >> <snip> >> >>>>> -------------------------------------------------------------------------------- >>>>> Compliance test for device /dev/v4l-subdev1: >>>>> >>>>> Driver Info: >>>>> Driver version : 6.6.0 >>>>> Capabilities : 0x00000000 >>>> >>>> But this does not appear for v4l-subdev1. >>>> >>>> I can't really tell why it doesn't show that. Can you debug a little bit? >>>> The code is in v4l2-compliance.cpp, line 1086: >>>> >>>> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >>>> >>>> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >>>> and that fails for some reason. It could be that media_fd is invalid (would be weird). >>>> >>>> This could well be a v4l2-compliance bug that you hit with this driver. >>>> >>> >>> On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is >>> /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd >>> obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. >>> >>> I don't know why media0 does not exist in /sys/dev/char/81:3/device? >>> >> >> Can you try again with this v4l2-compliance patch? >> >> I need to dig a bit deeper as to why media0 is missing, but for now try this. >> >> Regards, >> >> Hans >> >> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >> index 7169eefe..29475d6b 100644 >> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >> @@ -968,7 +968,7 @@ err: >> } >> >> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >> - unsigned frame_count, unsigned all_fmt_frame_count) >> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd) >> { >> struct node node2; >> struct v4l2_capability vcap = {}; >> @@ -997,8 +997,12 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >> memset(&vcap, 0, sizeof(vcap)); >> } >> >> - if (!node.is_media()) >> - media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >> + if (!node.is_media()) { >> + if (parent_media_fd >= 0) >> + media_fd = parent_media_fd; >> + else >> + media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >> + } >> >> int fd = node.is_media() ? node.g_fd() : media_fd; >> if (fd >= 0) { >> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >> index 7caf254b..c47f25f5 100644 >> --- a/utils/v4l2-compliance/v4l2-compliance.h >> +++ b/utils/v4l2-compliance/v4l2-compliance.h >> @@ -308,7 +308,7 @@ int check_ustring(const __u8 *s, int len); >> int check_0(const void *p, int len); >> int restoreFormat(struct node *node); >> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >> - unsigned frame_count, unsigned all_fmt_frame_count); >> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd = -1); >> std::string stream_from(const std::string &pixelformat, bool &use_hdr); >> >> // Media Controller ioctl tests >> > > From the log, there is no change. Oops, my mistake. Also apply this change: diff --git a/utils/v4l2-compliance/v4l2-test-media.cpp b/utils/v4l2-compliance/v4l2-test-media.cpp index 0195ac58..52ab7fb8 100644 --- a/utils/v4l2-compliance/v4l2-test-media.cpp +++ b/utils/v4l2-compliance/v4l2-test-media.cpp @@ -612,7 +612,7 @@ void walkTopology(struct node &node, struct node &expbuf_node, } testNode(test_node, test_node, expbuf_node, type, - frame_count, all_fmt_frame_count); + frame_count, all_fmt_frame_count, node.g_fd()); test_node.close(); } } Regards, Hans
On 2023/10/18 17:31, Hans Verkuil wrote: > On 18/10/2023 11:25, Jack Zhu wrote: >> >> >> On 2023/10/18 16:50, Hans Verkuil wrote: >>> Hi Jack, >>> >>> On 18/10/2023 04:37, Jack Zhu wrote: >>> >>> <snip> >>> >>>>>> -------------------------------------------------------------------------------- >>>>>> Compliance test for device /dev/v4l-subdev1: >>>>>> >>>>>> Driver Info: >>>>>> Driver version : 6.6.0 >>>>>> Capabilities : 0x00000000 >>>>> >>>>> But this does not appear for v4l-subdev1. >>>>> >>>>> I can't really tell why it doesn't show that. Can you debug a little bit? >>>>> The code is in v4l2-compliance.cpp, line 1086: >>>>> >>>>> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >>>>> >>>>> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >>>>> and that fails for some reason. It could be that media_fd is invalid (would be weird). >>>>> >>>>> This could well be a v4l2-compliance bug that you hit with this driver. >>>>> >>>> >>>> On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is >>>> /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd >>>> obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. >>>> >>>> I don't know why media0 does not exist in /sys/dev/char/81:3/device? >>>> >>> >>> Can you try again with this v4l2-compliance patch? >>> >>> I need to dig a bit deeper as to why media0 is missing, but for now try this. >>> >>> Regards, >>> >>> Hans >>> >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >>> index 7169eefe..29475d6b 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >>> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >>> @@ -968,7 +968,7 @@ err: >>> } >>> >>> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >>> - unsigned frame_count, unsigned all_fmt_frame_count) >>> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd) >>> { >>> struct node node2; >>> struct v4l2_capability vcap = {}; >>> @@ -997,8 +997,12 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >>> memset(&vcap, 0, sizeof(vcap)); >>> } >>> >>> - if (!node.is_media()) >>> - media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >>> + if (!node.is_media()) { >>> + if (parent_media_fd >= 0) >>> + media_fd = parent_media_fd; >>> + else >>> + media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >>> + } >>> >>> int fd = node.is_media() ? node.g_fd() : media_fd; >>> if (fd >= 0) { >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>> index 7caf254b..c47f25f5 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>> @@ -308,7 +308,7 @@ int check_ustring(const __u8 *s, int len); >>> int check_0(const void *p, int len); >>> int restoreFormat(struct node *node); >>> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >>> - unsigned frame_count, unsigned all_fmt_frame_count); >>> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd = -1); >>> std::string stream_from(const std::string &pixelformat, bool &use_hdr); >>> >>> // Media Controller ioctl tests >>> >> >> From the log, there is no change. > > Oops, my mistake. Also apply this change: > > diff --git a/utils/v4l2-compliance/v4l2-test-media.cpp b/utils/v4l2-compliance/v4l2-test-media.cpp > index 0195ac58..52ab7fb8 100644 > --- a/utils/v4l2-compliance/v4l2-test-media.cpp > +++ b/utils/v4l2-compliance/v4l2-test-media.cpp > @@ -612,7 +612,7 @@ void walkTopology(struct node &node, struct node &expbuf_node, > } > > testNode(test_node, test_node, expbuf_node, type, > - frame_count, all_fmt_frame_count); > + frame_count, all_fmt_frame_count, node.g_fd()); > test_node.close(); > } > } > Can see relevant Info in the log. test log: -------------------------------------------------------------------------------- Compliance test for starfive-camss device /dev/v4l-subdev1: Driver Info: Driver version : 6.6.0 Capabilities : 0x00000000 Media Driver Info: Driver name : starfive-camss Model : Starfive Camera Subsystem Serial : Bus info : platform:19840000.camss Media version : 6.6.0 Hardware revision: 0x00000000 (0) Driver version : 6.6.0 Interface Info: ID : 0x0300001e Type : V4L Sub-Device Entity Info: ID : 0x00000018 (24) Name : imx219 6-0010 Function : Camera Sensor Pad 0x01000019 : 0: Source Link 0x0200001a: to remote pad 0x100000f of entity 'cdns_csi2rx.19800000.csi-bridge' (Video Interface Bridge): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev1 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Sub-Device ioctls (Source Pad 0): Try Stream 0 test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK test Try VIDIOC_SUBDEV_G/S_FMT: OK warn: ../utils/v4l2-compliance/v4l2-test-subdevs.cpp(541): VIDIOC_SUBDEV_G_SELECTION is supported for target 0 but not VIDIOC_SUBDEV_S_SELECTION test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK Active Stream 0 test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK test Active VIDIOC_SUBDEV_G/S_FMT: OK warn: ../utils/v4l2-compliance/v4l2-test-subdevs.cpp(541): VIDIOC_SUBDEV_G_SELECTION is supported for target 0 but not VIDIOC_SUBDEV_S_SELECTION test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 20 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) Total for starfive-camss device /dev/v4l-subdev1: 51, Succeeded: 51, Failed: 0, Warnings: 2 Grand Total for starfive-camss device /dev/media0: 209, Succeeded: 209, Failed: 0, Warnings: 2 #
On 18/10/2023 11:52, Jack Zhu wrote: > > > On 2023/10/18 17:31, Hans Verkuil wrote: >> On 18/10/2023 11:25, Jack Zhu wrote: >>> >>> >>> On 2023/10/18 16:50, Hans Verkuil wrote: >>>> Hi Jack, >>>> >>>> On 18/10/2023 04:37, Jack Zhu wrote: >>>> >>>> <snip> >>>> >>>>>>> -------------------------------------------------------------------------------- >>>>>>> Compliance test for device /dev/v4l-subdev1: >>>>>>> >>>>>>> Driver Info: >>>>>>> Driver version : 6.6.0 >>>>>>> Capabilities : 0x00000000 >>>>>> >>>>>> But this does not appear for v4l-subdev1. >>>>>> >>>>>> I can't really tell why it doesn't show that. Can you debug a little bit? >>>>>> The code is in v4l2-compliance.cpp, line 1086: >>>>>> >>>>>> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >>>>>> >>>>>> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >>>>>> and that fails for some reason. It could be that media_fd is invalid (would be weird). >>>>>> >>>>>> This could well be a v4l2-compliance bug that you hit with this driver. >>>>>> >>>>> >>>>> On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is >>>>> /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd >>>>> obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. >>>>> >>>>> I don't know why media0 does not exist in /sys/dev/char/81:3/device? >>>>> >>>> >>>> Can you try again with this v4l2-compliance patch? >>>> >>>> I need to dig a bit deeper as to why media0 is missing, but for now try this. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >>>> index 7169eefe..29475d6b 100644 >>>> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >>>> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >>>> @@ -968,7 +968,7 @@ err: >>>> } >>>> >>>> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >>>> - unsigned frame_count, unsigned all_fmt_frame_count) >>>> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd) >>>> { >>>> struct node node2; >>>> struct v4l2_capability vcap = {}; >>>> @@ -997,8 +997,12 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >>>> memset(&vcap, 0, sizeof(vcap)); >>>> } >>>> >>>> - if (!node.is_media()) >>>> - media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >>>> + if (!node.is_media()) { >>>> + if (parent_media_fd >= 0) >>>> + media_fd = parent_media_fd; >>>> + else >>>> + media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >>>> + } >>>> >>>> int fd = node.is_media() ? node.g_fd() : media_fd; >>>> if (fd >= 0) { >>>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>>> index 7caf254b..c47f25f5 100644 >>>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>>> @@ -308,7 +308,7 @@ int check_ustring(const __u8 *s, int len); >>>> int check_0(const void *p, int len); >>>> int restoreFormat(struct node *node); >>>> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >>>> - unsigned frame_count, unsigned all_fmt_frame_count); >>>> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd = -1); >>>> std::string stream_from(const std::string &pixelformat, bool &use_hdr); >>>> >>>> // Media Controller ioctl tests >>>> >>> >>> From the log, there is no change. >> >> Oops, my mistake. Also apply this change: >> >> diff --git a/utils/v4l2-compliance/v4l2-test-media.cpp b/utils/v4l2-compliance/v4l2-test-media.cpp >> index 0195ac58..52ab7fb8 100644 >> --- a/utils/v4l2-compliance/v4l2-test-media.cpp >> +++ b/utils/v4l2-compliance/v4l2-test-media.cpp >> @@ -612,7 +612,7 @@ void walkTopology(struct node &node, struct node &expbuf_node, >> } >> >> testNode(test_node, test_node, expbuf_node, type, >> - frame_count, all_fmt_frame_count); >> + frame_count, all_fmt_frame_count, node.g_fd()); >> test_node.close(); >> } >> } >> > > Can see relevant Info in the log. Great! Can you do one more thing? Please run 'v4l2-compliance -m /dev/media0 --verbose' and mail the output to me. It's pretty big, so just email it to me, without CCs. I want to take a closer look at the output to see why this patch is needed. Regards, Hans
On Wed, Oct 18, 2023 at 11:11:40AM +0800, Jack Zhu wrote: > On 2023/10/16 19:40, Hans Verkuil wrote: > > On 08/10/2023 10:51, Jack Zhu wrote: > >> Hi, > >> > >> This series is the v10 series that attempts to support the Camera Subsystem > >> found on StarFive JH7110 SoC. > >> > >> This series is based on top of the master branch of media_stage repository, > >> which is tested with a v4l2-compliance compiled from the git repo > >> (git://linuxtv.org/v4l-utils.git). > > > > I get one smatch warning: > > > > drivers/staging/media/starfive/camss/stf-isp.c:122 isp_enum_mbus_code() warn: unsigned 'code->index' is never less than zero. > > Could you please tell me the code check command? This way I can use it to check > my next commit. > > > And I also notice that there is no TODO file: staging drivers should have a > > TODO file explaining what needs to be done to get them out of staging. > > OK, I'll add it to my next commit. I previously misunderstood that it was submitted > when moving out of staging. > > > I'm curious to know that as well :-) > > > > It looks like there is a lot of additional development that can be done, since > > most of the ISP parameters appear to be hardcoded. > > Part is the module initialization configuration. In the next stage, we will use > incremental development to implement 3A functions. I'm really looking forward to that part :-)
On 18/10/2023 11:56, Hans Verkuil wrote: > On 18/10/2023 11:52, Jack Zhu wrote: >> >> >> On 2023/10/18 17:31, Hans Verkuil wrote: >>> On 18/10/2023 11:25, Jack Zhu wrote: >>>> >>>> >>>> On 2023/10/18 16:50, Hans Verkuil wrote: >>>>> Hi Jack, >>>>> >>>>> On 18/10/2023 04:37, Jack Zhu wrote: >>>>> >>>>> <snip> >>>>> >>>>>>>> -------------------------------------------------------------------------------- >>>>>>>> Compliance test for device /dev/v4l-subdev1: >>>>>>>> >>>>>>>> Driver Info: >>>>>>>> Driver version : 6.6.0 >>>>>>>> Capabilities : 0x00000000 >>>>>>> >>>>>>> But this does not appear for v4l-subdev1. >>>>>>> >>>>>>> I can't really tell why it doesn't show that. Can you debug a little bit? >>>>>>> The code is in v4l2-compliance.cpp, line 1086: >>>>>>> >>>>>>> ent_id = mi_media_info_for_fd(media_fd, node.g_fd(), &is_invalid, &node.function); >>>>>>> >>>>>>> The mi_media_info_for_fd() function calls ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &mdinfo), >>>>>>> and that fails for some reason. It could be that media_fd is invalid (would be weird). >>>>>>> >>>>>>> This could well be a v4l2-compliance bug that you hit with this driver. >>>>>>> >>>>>> >>>>>> On the test board, /dev/v4l-subdev1 is imx219, and the corresponding directory is >>>>>> /sys/dev/char/81:3/device. Media0 does not exist in this directory. Therefore, the media_fd >>>>>> obtained through mi_get_media_fd(node.g_fd(), node.bus_info) is invalid. >>>>>> >>>>>> I don't know why media0 does not exist in /sys/dev/char/81:3/device? >>>>>> >>>>> >>>>> Can you try again with this v4l2-compliance patch? >>>>> >>>>> I need to dig a bit deeper as to why media0 is missing, but for now try this. >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >>>>> index 7169eefe..29475d6b 100644 >>>>> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >>>>> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >>>>> @@ -968,7 +968,7 @@ err: >>>>> } >>>>> >>>>> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >>>>> - unsigned frame_count, unsigned all_fmt_frame_count) >>>>> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd) >>>>> { >>>>> struct node node2; >>>>> struct v4l2_capability vcap = {}; >>>>> @@ -997,8 +997,12 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >>>>> memset(&vcap, 0, sizeof(vcap)); >>>>> } >>>>> >>>>> - if (!node.is_media()) >>>>> - media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >>>>> + if (!node.is_media()) { >>>>> + if (parent_media_fd >= 0) >>>>> + media_fd = parent_media_fd; >>>>> + else >>>>> + media_fd = mi_get_media_fd(node.g_fd(), node.bus_info); >>>>> + } >>>>> >>>>> int fd = node.is_media() ? node.g_fd() : media_fd; >>>>> if (fd >= 0) { >>>>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>>>> index 7caf254b..c47f25f5 100644 >>>>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>>>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>>>> @@ -308,7 +308,7 @@ int check_ustring(const __u8 *s, int len); >>>>> int check_0(const void *p, int len); >>>>> int restoreFormat(struct node *node); >>>>> void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_node, media_type type, >>>>> - unsigned frame_count, unsigned all_fmt_frame_count); >>>>> + unsigned frame_count, unsigned all_fmt_frame_count, int parent_media_fd = -1); >>>>> std::string stream_from(const std::string &pixelformat, bool &use_hdr); >>>>> >>>>> // Media Controller ioctl tests >>>>> >>>> >>>> From the log, there is no change. >>> >>> Oops, my mistake. Also apply this change: >>> >>> diff --git a/utils/v4l2-compliance/v4l2-test-media.cpp b/utils/v4l2-compliance/v4l2-test-media.cpp >>> index 0195ac58..52ab7fb8 100644 >>> --- a/utils/v4l2-compliance/v4l2-test-media.cpp >>> +++ b/utils/v4l2-compliance/v4l2-test-media.cpp >>> @@ -612,7 +612,7 @@ void walkTopology(struct node &node, struct node &expbuf_node, >>> } >>> >>> testNode(test_node, test_node, expbuf_node, type, >>> - frame_count, all_fmt_frame_count); >>> + frame_count, all_fmt_frame_count, node.g_fd()); >>> test_node.close(); >>> } >>> } >>> >> >> Can see relevant Info in the log. > > Great! Can you do one more thing? Please run 'v4l2-compliance -m /dev/media0 --verbose' > and mail the output to me. It's pretty big, so just email it to me, without CCs. > > I want to take a closer look at the output to see why this patch is needed. Thank you for your help. This v4l2-compliance patch is in fact needed, and I have just pushed the fix. Regards, Hans