Message-ID: <178d5426-9761-4790-a488-a0009b0c9ba0@xs4all.nl>
Date:   Wed, 6 Dec 2023 12:03:32 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     yuji2.ishikawa@...hiba.co.jp, 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

On 27/11/2023 01:45, yuji2.ishikawa@...hiba.co.jp wrote:
> Hello Hans,
> 
>> -----Original Message-----
>> From: Hans Verkuil <hverkuil@...all.nl>
>> Sent: Tuesday, November 14, 2023 6:11 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 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@...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.
>>>
>>>> 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.

The key takeaway here is that you don't want to have to add support for a
driver-specific type to the core control framework. It has to remain
driver specific. In this case that means you need to provide your own
custom type ops in the driver, just like dw100 does. The actual code there
will of course be different, since you are dealing with a compound type,
not an array. But the principle is the same.

Regards,

	Hans

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

Powered by blists - more mailing lists