Message-ID: <5595805b-8017-4bcd-bcbc-fd0a75f9c3dc@xs4all.nl>
Date:   Wed, 6 Dec 2023 12:08:43 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     yuji2.ishikawa@...hiba.co.jp, laurent.pinchart@...asonboard.com
Cc:     mchehab@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        nobuhiro1.iwamatsu@...hiba.co.jp, 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:46, yuji2.ishikawa@...hiba.co.jp wrote:
> Hello Laurent,
> 
> Thank you for your comment.
> 
>> -----Original Message-----
>> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>> Sent: Tuesday, November 14, 2023 6:54 PM
>> To: Hans Verkuil <hverkuil@...all.nl>
>> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
>> <yuji2.ishikawa@...hiba.co.jp>; 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>; 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 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@...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.
>>
>> 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.

This is the preferred method over using controls.

Note that the 32 bit vs 64 bit requirement still stands, so use pahole to make
sure that whatever data you exchange over the parameter buffer is the same for
both architectures.

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

Powered by blists - more mailing lists