RE: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor specific control handlers

[yuji2.ishikawa]

Hello Hans,

> -----Original Message-----
> From: Hans Verkuil <hverkuil@xxxxxxxxx>
> Sent: Tuesday, November 14, 2023 6:11 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; 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>
> Cc: 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 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.

I checked drivers/media/platform/nxp/dw100/dw100.c and found that
V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP handles a 2D array of U32 parameters,
which is covered by standard v4l2_ctrl_type_op_xxxx() APIs.

I suppose that controls of the VIIF driver, which handle control specific structs,
would not be implemented in the same way and something else may be needed.

> Regards,
> 
> 	Hans
> 
> >
> > 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.
> >
> > Regards,
> >
> > 	Hans
> >
> >>  };
> >>
> >>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> >
> >

Regards,
Yuji