Message-ID: <TYAPR01MB620109D13BA981F8F6BBA49092BDA@TYAPR01MB6201.jpnprd01.prod.outlook.com>
Date:   Mon, 27 Nov 2023 00:43:21 +0000
From:   <yuji2.ishikawa@...hiba.co.jp>
To:     <hverkuil@...all.nl>, <laurent.pinchart@...asonboard.com>,
        <mchehab@...nel.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <nobuhiro1.iwamatsu@...hiba.co.jp>
CC:     <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor
 specific control handlers

Hello Hans,

Thank you for the review

> -----Original Message-----
> From: Hans Verkuil <hverkuil@...all.nl>
> Sent: Tuesday, November 14, 2023 6:02 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>; Laurent Pinchart
> <laurent.pinchart@...asonboard.com>; Mauro Carvalho Chehab
> <mchehab@...nel.org>; Rob Herring <robh+dt@...nel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> <conor+dt@...nel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○
> ＯＳＴ) <nobuhiro1.iwamatsu@...hiba.co.jp>
> Cc: linux-media@...r.kernel.org; devicetree@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor
> specific control handlers
> 
> 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@...hiba.co.jp>
> > ---
> > 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.
> 

I'll put changes for the core library into a separate patch.

> > 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.

I understand.

> 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.

I'll apply pahole tool.

> 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

Powered by blists - more mailing lists