Hello Laurent,
> -----Original Message-----
> From: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> 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(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ)
> <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(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> > <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(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○
> > ＯＳＴ) <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
