Hello Laurent, > -----Original Message----- > From: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開) > Sent: Monday, November 27, 2023 9:47 AM > To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; Hans Verkuil > <hverkuil@xxxxxxxxx> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; > iwamatsu nobuhiro(岩松 信洋 ○DITC□DIT○OST) > <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > specific control handlers > > Hello Laurent, > > Thank you for your comment. > > > -----Original Message----- > > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > Sent: Tuesday, November 14, 2023 6:54 PM > > To: Hans Verkuil <hverkuil@xxxxxxxxx> > > Cc: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開) > > <yuji2.ishikawa@xxxxxxxxxxxxx>; Mauro Carvalho Chehab > > <mchehab@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof > > Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley > > <conor+dt@xxxxxxxxxx>; iwamatsu nobuhiro(岩松 信洋 ○DITC□DIT○ > > OST) <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; > linux-media@xxxxxxxxxxxxxxx; > > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > linux-kernel@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > > specific control handlers > > > > On Tue, Nov 14, 2023 at 10:10:50AM +0100, Hans Verkuil wrote: > > > On 14/11/2023 10:02, Hans Verkuil wrote: > > > > On 12/10/2023 09:13, Yuji Ishikawa wrote: > > > >> Add support to Image Signal Processors of Visconti's Video Input > > Interface. > > > >> This patch adds vendor specific compound controls to configure > > > >> the image signal processor. > > > >> > > > >> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@xxxxxxxxxxxxx> > > > >> --- > > > >> Changelog v2: > > > >> - Resend v1 because a patch exceeds size limit. > > > >> > > > >> Changelog v3: > > > >> - Adapted to media control framework > > > >> - Introduced ISP subdevice, capture device > > > >> - Remove private IOCTLs and add vendor specific V4L2 controls > > > >> - Change function name avoiding camelcase and uppercase letters > > > >> > > > >> Changelog v4: > > > >> - Split patches because the v3 patch exceeds size limit > > > >> - Stop using ID number to identify driver instance: > > > >> - Use dynamically allocated structure to hold HW specific context, > > > >> instead of static one. > > > >> - Call HW layer functions with the context structure instead of > > > >> ID number > > > >> > > > >> Changelog v5: > > > >> - no change > > > >> > > > >> Changelog v6: > > > >> - remove unused macros > > > >> - removed hwd_ and HWD_ prefix > > > >> - update source code documentation > > > >> - Suggestion from Hans Verkuil > > > >> - pointer to userland memory is removed from uAPI arguments > > > >> - style of structure is now "nested" instead of "chained by pointer"; > > > >> - use div64_u64 for 64bit division > > > >> - vendor specific controls support TRY_EXT_CTRLS > > > >> - add READ_ONLY flag to GET_CALIBRATION_STATUS control and > > similar ones > > > >> - human friendry control names for vendor specific controls > > > >> - add initial value to each vendor specific control > > > >> - GET_LAST_CAPTURE_STATUS control is updated asyncnously > from > > workqueue > > > >> - remove EXECUTE_ON_WRITE flag of vendor specific control > > > >> - uAPI: return value of GET_CALIBRATION_STATUS follows common > > rules of error codes > > > >> - applied v4l2-compliance > > > >> - Suggestion from Sakari Ailus > > > >> - use div64_u64 for 64bit division > > > >> - update copyright's year > > > >> - remove redandunt cast > > > >> - use bool instead of HWD_VIIF_ENABLE/DISABLE > > > >> - simplify comparison to 0 > > > >> - simplify statements with trigram operator > > > >> - remove redundant local variables > > > >> - use general integer types instead of u32/s32 > > > >> - Suggestion from Laurent Pinchart > > > >> - moved VIIF driver to driver/platform/toshiba/visconti > > > >> - change register access: struct-style to macro-style > > > >> - remove unused type definitions > > > >> - define enums instead of successive macro constants > > > >> - remove redundant parenthesis of macro constant > > > >> - embed struct hwd_res into struct viif_device > > > >> - use xxx_dma instead of xxx_paddr for variable names of IOVA > > > >> - literal value: just 0 instead of 0x0 > > > >> - use literal 1 or 0 instead of HWD_VIIF_ENABLE, DISABLE for > > > >> register > > access > > > >> - use true or false instead of HWD_VIIF_ENABLE, DISABLE for > > > >> function > > calls > > > >> - uAPI: return value of GET_CALIBRATION_STATUS follows common > > > >> rules of error codes > > > >> > > > >> Changelog v7: > > > >> - remove unused variables > > > >> - split long statements which have multiple logical-OR and > > > >> trigram operators > > > >> > > > >> Changelog v8: > > > >> - define constant V4L2_CTRL_TYPE_VISCONTI_ISP for datatype > > > >> of Visconti specific controls > > > >> - Suggestion from Hans Verkuil > > > >> - remove pr_info() > > > >> - use pm_runtime_get_if_in_use() to get power status > > > >> > > > >> Changelog v9: > > > >> - fix warning for cast between ptr and dma_addr_t > > > >> > > > >> .../media/platform/toshiba/visconti/Makefile | 2 +- > > > >> .../media/platform/toshiba/visconti/viif.c | 10 +- > > > >> .../platform/toshiba/visconti/viif_controls.c | 3395 > > +++++++++++++++++ > > > >> .../platform/toshiba/visconti/viif_controls.h | 18 + > > > >> .../platform/toshiba/visconti/viif_isp.c | 15 +- > > > >> drivers/media/v4l2-core/v4l2-ctrls-core.c | 7 +- > > > >> include/uapi/linux/videodev2.h | 2 + > > > >> 7 files changed, 3431 insertions(+), 18 deletions(-) create > > > >> mode > > > >> 100644 drivers/media/platform/toshiba/visconti/viif_controls.c > > > >> create mode 100644 > > > >> drivers/media/platform/toshiba/visconti/viif_controls.h > > > >> > > > > > > > > <snip> > > > > > > > > These core changes below should be in a separate patch, not mixed > > > > in with the driver. > > > > > > > >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c > > > >> b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > > >> index a662fb60f73f..0c4df9fffbe0 100644 > > > >> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > > > >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > > >> @@ -367,7 +367,9 @@ void v4l2_ctrl_type_op_log(const struct > > > >> v4l2_ctrl > > *ctrl) > > > >> case V4L2_CTRL_TYPE_AV1_FILM_GRAIN: > > > >> pr_cont("AV1_FILM_GRAIN"); > > > >> break; > > > >> - > > > >> + case V4L2_CTRL_TYPE_VISCONTI_ISP: > > > >> + pr_cont("VISCONTI_ISP"); > > > >> + break; > > > >> default: > > > >> pr_cont("unknown type %d", ctrl->type); > > > >> break; > > > >> @@ -1163,6 +1165,9 @@ static int std_validate_compound(const > > > >> struct > > v4l2_ctrl *ctrl, u32 idx, > > > >> case V4L2_CTRL_TYPE_AV1_FILM_GRAIN: > > > >> return validate_av1_film_grain(p); > > > >> > > > >> + case V4L2_CTRL_TYPE_VISCONTI_ISP: > > > >> + break; > > > >> + > > > >> case V4L2_CTRL_TYPE_AREA: > > > >> area = p; > > > >> if (!area->width || !area->height) diff --git > > > >> a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > > >> index c3d4e490ce7c..bbc3cd3efa65 100644 > > > >> --- a/include/uapi/linux/videodev2.h > > > >> +++ b/include/uapi/linux/videodev2.h > > > >> @@ -1915,6 +1915,8 @@ enum v4l2_ctrl_type { > > > >> V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281, > > > >> V4L2_CTRL_TYPE_AV1_FRAME = 0x282, > > > >> V4L2_CTRL_TYPE_AV1_FILM_GRAIN = 0x283, > > > >> + > > > >> + V4L2_CTRL_TYPE_VISCONTI_ISP = 0x290, > > > > > > > > I see you are using the same V4L2_CTRL_TYPE_VISCONTI_ISP for all > > > > the compound controls. But that's not allowed: the V4L2_CTRL_TYPE_ > > > > defines determine the control type, so each struct used by a > > > > control needs > > its own type. > > > > > > Actually, you don't want to add such a type at all. This is all > > > driver specific, so support like this belongs in the driver. > > > > > > A good example of that is > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP in > > > drivers/media/platform/nxp/dw100/dw100.c: there all the handling is > > > done in the driver, and it adds init/validate/log/equal ops as well. > > > > Actually, I think a better option is to use parameters buffers instead > > of controls, like other ISP driver do. > > > > I'll quickly make an experimental impl of parameters buffers of the VIIF driver > and check if it works well. > I'm still not for sure passing parameter via parameters buffers satisfies the > timing constraint of the HW. I made an experimental implementation of parameters buffers based on the RKISP1 driver. The experimental driver seems working well, but there's a concern. Due to HW spec, there is a minium 2-frame delay before the enqueued parameters are reflected in the resulting image, while most ISPs should reflect it with a minium 1-frame delay. Our in-house AE/AG middleware will be affected by increased latency. Does libcamera have specific requirements for control latency?, or it can handle the problem by properly using parameter interface and statistics interface? ===== The Visconti5's ISP captures register values automatically at VSync, not manually triggered by register operations (as most ISPs do). The extra 1 frame occurs as shown below. [Visconti5 ISP requires 2 frames to reflect] 0: user application issues ioctl(QBUF) of new parameters 1: VSYNC ISR pops parameters and sets them to ISP registers 2: ISP captures register values at VSYNC. ISP reflects. [Most ISPs require 1 frame to reflect] 0: user application issues ioctl(QBUF) of new parameters 1: VSYNC ISR pops parameters, sets them to ISP registers and notify the change. ISP reflects. > > > > I also noticed looking through include/uapi/linux/visconti_viif.h > > > > that some of the struct have holes. I really want to avoid holes > > > > in structs used by controls, it is bad practice. > > > > > > > > The pahole utility is very useful for testing this. It is also > > > > highly recommended to check for both 32 and 64 bit compilation: > > > > the struct layout must be the same, otherwise you would run into > > > > problems if a 32 bit application is used with a 64 bit kernel. > > > > > > > > Finally, Laurent and/or Sakari will also take a look at this > > > > driver, for some reason this driver has been mostly reviewed by > > > > me, but I am not really the expert on ISPs. > > > > > > > >> }; > > > >> > > > >> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */ > > > > -- > > Regards, > > > > Laurent Pinchart > > > Regards, > Yuji Regards, Yuji